430695 - add backoff algorithm to gethash requests

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Marria Nazif]

Currently, we don't cache the result from failed gethash requests and there is no backoff for these requests.  Thus, gethash failures on a popular site that is blacklisted will raise our qps.

I'd like to add the same backoff that we have for regular downloads requests to the gethash requests.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Absolutely not. When we're in backoff mode we're not allowed to use our cached data beyond 30 minutes. If we also backoff from that, then we're basically saying that backoff mode = zero protection.

Strongly suggest WONTFIX.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

(see bug 427862 for details on how we avoid our caches and rely on gethash when we're in backoff mode)

Comment 3

[Marria Nazif]

Mike - I understand your concern, but it's important to realize that backoff
for gethash should only kick in if the client is receiving errors already on
gethash requests.  That means you won't be getting any protection at that point
anyway, since no warning is shown when there is a gethash error.  It's
basically a last ditch kind of thing, that shouldn't ever happen in the normal
case.  The idea is that if the server is already returning errors, continuing
to ping regularly won't help (and could possibly hurt).

I've also discussed some options with Dave that are more conservative for parse
errors (rather than 5xx 4xx errors).  For example, we could cache the error for
an individual expression if it is a parse error and extend the cache time for
each additional error.

I'm also open to a different backoff schedule.

However, I do really think we need some kind of backoff.

Thoughts? 

Comment 4

[Marria Nazif]

renominating just to make sure you notice the new comment

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Ian Fette poked me on IM (he's having trouble connecting to Bugzilla from WWW in China) to say:

Mike - just to clarify on bug 430695 (re backoff for gethash) - this is not saying "if you're being backed off from updates then don't do gethash requests" - it's saying that if the gethash request itself returns an error, then implement some sort of backoff for subsequent gethash requests for that hash. Not sure if there was some confusion there

I hadn't understood that from comment 0, sorry. I withdraw the strenuous objection from comment 1.

This makes sense to me, dunno cost of implementation, though, at this stage. dcamp?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Dave, is this the sort of thing we could take on the branch?

Marria, do you *know* that this will cause problems, or is this an enhancement? I'd really rather not delay our ship date any further, so I'm going to minus this for now, but if new information comes to light, you know how to set it back to a "?" :)

Comment 7

[Marria Nazif]

Hi Mike - this is something that I think is important to implement before the launch.  In a worst case scenario, it could cause problems, and there are people on our end who will likely require it for launch.

I've talked with Dave about this bug offline.  He said he would update the bug with his thoughts.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Tony: do you have time to put together a patch for this? Dave's said that it's obvious where it should go in nsURLClassifierHashCompleter, find him on IM or IRC to help.

Comment 9

[Tony Chang (Google)]

I doubt I'll have time.  Definitely not this week, maybe during the weekend.

Comment 10

[Dave Camp (:dcamp)]

This should be pretty easy, putting this here in case I don't get to it today:

Basically in
toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp, we need
to check for errors in nsUrlClassifierHashCompleterRequest::OnStopRequest(),
apply whatever backoff logic we want here, set up a "next allowed request
time", and check that next-allowed-request-time in
nsUrlClassifierHashCompleterRequest::Begin().  If there's a backoff, just call
NotifyFailure().

Marria, can you guys detail the backoff algorithm you guys want here?

Comment 11

[Dave Camp (:dcamp)]

Honza, do you have time to do this?

Comment 12

[Marria Nazif]

From our perspective, it would be just fine to have the same backoff behavior as the regular download request, and it might be easier to start with that for the first cut.  We can also play with those parameters if you think that backoff will be too aggressive.

Comment 13

[Marria Nazif]

Let me be more precise in describing the details here.  Specifically, this is my suggestion for the backoff (although the details can be tweaked).

If a client receives 2 gethash errors within 5 minutes, we enter backoff mode.  After this point, if the client receives one non-error response, or the last error occurred at least 8 hours ago, we exit backoff mode.

While in backoff mode, we will make sure not to ping for at least a certain amount of time from the last error.  This time changes exponentially until a max of 2 hours.  Specifically, first we don't ping for at least 30 minutes from the last error.  If receiving another error, we don't ping for at least 1 hour.  If receiving another error, we don't ping for at least 2 hours.  After that, we wait at least 2 hours between pings.

Comment 14

[Dave Camp (:dcamp)]

Attachment: v1

Comment 15

[Tony Chung [:tchung]]

how can this fix be tested?

Comment 16

[Tony Chang (Google)]

Comment on attachment 319710 [details] [diff] [review]
v1

+             PR_IntervalToSeconds(mErrorTimes[0] - now) >= gBackoffTime) {

This looks backwards, isn't now always >= mErrorTimes[0]?

Comment 17

[Dave Camp (:dcamp)]

Attachment: v2

Indeed, fixed that line up.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319731 [details] [diff] [review]
v2

a1.9=beltzner

Comment 19

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp 	1.7
mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h 	1.6