Closed Bug 428548 Opened 12 years ago Closed 12 years ago

Limit total number of safebrowsing update requests per hour

Categories

(Toolkit :: Safe Browsing, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: marria, Assigned: dcamp)

References

Details

(Whiteboard: [has patch][has reviews])

Attachments

(1 file)

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
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.
We'd like to see at least #3 done before release, as a last-ditch means to prevent high ping rates.
Flags: blocking-firefox3?
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).
Attachment #315676 - Flags: review?(tony)
Attachment #315676 - Attachment is patch: true
Attachment #315676 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #315676 - Flags: review?(tony) → review+
(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.

Oh, you're right.  I misread it as an && instead of an ||.  It's fine how it is.
Whiteboard: [has patch][has reviews]
Flags: blocking-firefox3? → blocking-firefox3+
Comment on attachment 315676 [details] [diff] [review]
Allow a max of 4 requests in any one-hour period

Dave, please land ASAP.
Attachment #315676 - Flags: approval1.9+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Tweaks to safebrowsing backoff behavior → Limit total number of safebrowsing update requests per hour
Product: Firefox → Toolkit
See Also: → 429394
You need to log in before you can comment on or make changes to this bug.