428548 - Limit total number of safebrowsing update requests per hour

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Marria Nazif]

We'd like to make some tweaks to the backoff behavior for safebrowsing to make it more reliable:

1. Currently it only activates backoff if 3 errors occur within 10 minutes.  This 10 minute interval seems largely irrelevant now that we've taken out lookup mode, so we should probably remove this check.

2. We'd like backoff to persist across restarts.  So, if a user restarts frequently they would still ping just as often as someone who keeps their browser.

3. We should keep track of the last ping time and add a sanity check when the timer fires that we're not pinging sooner than we are intending to.  This is to prevent a situation where we have clients that always ping frequently (see bug https://bugzilla.mozilla.org/show_bug.cgi?id=402971

Comment 1

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

Dave: does this block, or are there changes here could take on the branch? None of these, save perhaps #3, look like hard blockers to me, but if you think this needs to block, please nominate it.

Comment 2

[Brian Ryner (not reading)]

We'd like to see at least #3 done before release, as a last-ditch means to prevent high ping rates.

Comment 3

[Dave Camp (:dcamp)]

Attachment: Allow a max of 4 requests in any one-hour period

This patch limits the total number of requests in any given one-hour period to four.  This should be enough for the worst case (one successful request, then three error requests causing a one-hour backoff).

I also pulled out the cut-n-paste backoff tests into an automated unit test (and added tests for this behavior).

Comment 4

[Tony Chang (Google)]

Comment on attachment 315676 [details] [diff] [review]
Allow a max of 4 requests in any one-hour period

> RequestBackoff.prototype.canMakeRequest = function() {
>-  return Date.now() > this.nextRequestTime_;
>+  var now = Date.now();
>+  if (now <= this.nextRequestTime_) {
>+    return false;
>+  }
>+
>+  return (this.requestTimes_.length < this.MAX_REQUESTS_ ||
>+          (now - this.requestTimes_[0]) > this.REQUEST_PERIOD_);
>+}

Don't you need to check that this.requestTimes_.length is non-zero before checking this.requestTimes_[0]?

The rest looks good.

Comment 5

[Dave Camp (:dcamp)]

(In reply to comment #4)
> >+  return (this.requestTimes_.length < this.MAX_REQUESTS_ ||
> >+          (now - this.requestTimes_[0]) > this.REQUEST_PERIOD_);
> >+}
> 
> Don't you need to check that this.requestTimes_.length is non-zero before
> checking this.requestTimes_[0]?

If this.requestTimes_.length is 0, the first clause of the || will eval true and the second part won't be evaluated.

I don't mind changing this if you think it's confusing, but it shouldn't be necessary.

Comment 6

[Tony Chang (Google)]

Oh, you're right.  I misread it as an && instead of an ||.  It's fine how it is.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 315676 [details] [diff] [review]
Allow a max of 4 requests in any one-hour period

Dave, please land ASAP.

Comment 8

[Dave Camp (:dcamp)]

Closing and retopic'ing this bug to #3.  Opening separate bugs for 1 and 2.

Checking in toolkit/components/url-classifier/content/listmanager.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/listmanager.js,v  <--  listmanager.js
new revision: 1.30; previous revision: 1.29
done
Checking in toolkit/components/url-classifier/content/request-backoff.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/request-backoff.js,v  <--  request-backoff.js
new revision: 1.3; previous revision: 1.2
done
Checking in test_backoff.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_backoff.js,v  <--  test_backoff.js
initial revision: 1.1
done