1029240 - exponential backoff in safebrowsing code is completely broken

Status: RESOLVED INVALID

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Caller in listmanager.js:
  if (!status) {
    status = 500;
  }
  status = parseInt(status, 10);
  this.requestBackoffs_[updateUrl].noteServerResponse(status);

Callee in nsUrlClassifierHashCompleter.js:
  noteServerResponse: function HC_noteServerResponse(aSuccess) {
    if (aSuccess) {
      this._backoff = false;
      this._nextRequestTime = 0;
      this._backoffTime = 0;
      return;
    }

So this code backs off when the HTTP status code is 0 (which is never because the caller ensures that). Also the caller calls nextRequestDelay instead of nextRequestTime.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Sigh, the callee is actually request-backoff.js, not the hash completer. Delightfully they have interfaces with the same name with different semantics.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/request-backoff.js

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Well, it actually is completely broken, but for a different reason.     

In the downloadError callback, these lines schedule a non-repeating alarm that never gets upgraded to a repeating one, even if the download succeeds. This will be fixed in bug 1021419 so there's no need to keep this one open.

this.currentUpdateChecker_ =
445       new G_Alarm(BindToObject(this.checkForUpdates, this),
446                   this.requestBackoff_.nextRequestDelay());