From 957a9ff37abf1fc44ea21b58cb6049d74c3ba1d9 Mon Sep 17 00:00:00 2001 From: "dcamp%mozilla.com" Date: Wed, 23 Apr 2008 16:57:56 +0000 Subject: [PATCH] Bug 429755: Apply some random fuzz to safebrowsing backoff intervals. r=tony, a=beltzner git-svn-id: svn://10.0.0.236/trunk@250706 18797224-902f-48f8-a5cc-f745e15eee43 --- .../safebrowsing/content/globalstore.js | 8 ++- .../url-classifier/content/listmanager.js | 17 ++++-- .../url-classifier/content/request-backoff.js | 55 +++++++---------- .../url-classifier/tests/unit/test_backoff.js | 61 +++++++++++-------- 4 files changed, 76 insertions(+), 65 deletions(-) diff --git a/mozilla/browser/components/safebrowsing/content/globalstore.js b/mozilla/browser/components/safebrowsing/content/globalstore.js index 821df84ef9c..c3cc7072cc8 100644 --- a/mozilla/browser/components/safebrowsing/content/globalstore.js +++ b/mozilla/browser/components/safebrowsing/content/globalstore.js @@ -150,13 +150,17 @@ PROT_DataProvider.prototype.getUrlPref_ = function(prefName) { var appInfo = Components.classes["@mozilla.org/xre/app-info;1"] .getService(Components.interfaces.nsIXULAppInfo); - var mozClientStr = MOZ_OFFICIAL_BUILD ? 'navclient-auto-ffox' : appInfo.name; + var mozClientStr = this.prefs_.getPref("browser.safebrowsing.clientid", + MOZ_OFFICIAL_BUILD ? 'navclient-auto-ffox' : appInfo.name); + + var versionStr = this.prefs_.getPref("browser.safebrowsing.clientver", + appInfo.version); // Parameter substitution url = url.replace(MOZ_PARAM_LOCALE, this.getLocale_()); url = url.replace(MOZ_PARAM_CLIENT, mozClientStr); url = url.replace(MOZ_PARAM_BUILDID, appInfo.appBuildID); - url = url.replace(MOZ_PARAM_VERSION, appInfo.version); + url = url.replace(MOZ_PARAM_VERSION, versionStr); return url; } diff --git a/mozilla/toolkit/components/url-classifier/content/listmanager.js b/mozilla/toolkit/components/url-classifier/content/listmanager.js index a7150526f43..018d53f2b82 100644 --- a/mozilla/toolkit/components/url-classifier/content/listmanager.js +++ b/mozilla/toolkit/components/url-classifier/content/listmanager.js @@ -96,12 +96,16 @@ function PROT_ListManager() { BindToObject(this.cookieChanged_, this), false); - this.requestBackoff_ = new RequestBackoff(3 /* num errors */, - 10*60*1000 /* error time, 10min */, + /* Backoff interval should be between 30 and 60 minutes. */ + var backoffInterval = 30 * 60 * 1000; + backoffInterval += Math.floor(Math.random() * (30 * 60 * 1000)); + + this.requestBackoff_ = new RequestBackoff(2 /* max errors */, + 60*1000 /* retry interval, 1 min */, 4 /* num requests */, 60*60*1000 /* request time, 60 min */, - 60*60*1000 /* backoff interval, 60min */, - 6*60*60*1000 /* max backoff, 6hr */); + backoffInterval /* backoff interval, 60 min */, + 8*60*60*1000 /* max backoff, 8hr */); this.dbService_ = Cc["@mozilla.org/url-classifier/dbservice;1"] .getService(Ci.nsIUrlClassifierDBService); @@ -507,9 +511,10 @@ PROT_ListManager.prototype.downloadError_ = function(status) { this.requestBackoff_.noteServerResponse(status); if (this.requestBackoff_.isErrorStatus(status)) { - // Try again in a minute + // Schedule an update for when our backoff is complete this.currentUpdateChecker_ = - new G_Alarm(BindToObject(this.checkForUpdates, this), 60000); + new G_Alarm(BindToObject(this.checkForUpdates, this), + this.requestBackoff_.nextRequestDelay()); } } diff --git a/mozilla/toolkit/components/url-classifier/content/request-backoff.js b/mozilla/toolkit/components/url-classifier/content/request-backoff.js index 60daba34632..86adbf4bc8b 100644 --- a/mozilla/toolkit/components/url-classifier/content/request-backoff.js +++ b/mozilla/toolkit/components/url-classifier/content/request-backoff.js @@ -49,9 +49,8 @@ const HTTP_SEE_OTHER = 303; const HTTP_TEMPORARY_REDIRECT = 307; /** - * @param maxErrors Number the number of errors needed to trigger backoff - * @param errorPeriod Number time (ms) in which maxErros have to occur to - * trigger the backoff behavior + * @param maxErrors Number of times to request before backing off. + * @param retryIncrement Time (ms) for each retry before backing off. * @param maxRequests Number the number of requests needed to trigger backoff * @param requestPeriod Number time (ms) in which maxRequests have to occur to * trigger the backoff behavior @@ -59,11 +58,11 @@ const HTTP_TEMPORARY_REDIRECT = 307; * we double this time for consecutive errors * @param maxTimeout Number time (ms) maximum timeout period */ -function RequestBackoff(maxErrors, errorPeriod, +function RequestBackoff(maxErrors, retryIncrement, maxRequests, requestPeriod, timeoutIncrement, maxTimeout) { this.MAX_ERRORS_ = maxErrors; - this.ERROR_PERIOD_ = errorPeriod; + this.RETRY_INCREMENT_ = retryIncrement; this.MAX_REQUESTS_ = maxRequests; this.REQUEST_PERIOD_ = requestPeriod; this.TIMEOUT_INCREMENT_ = timeoutIncrement; @@ -72,21 +71,18 @@ function RequestBackoff(maxErrors, errorPeriod, // Queue of ints keeping the time of all requests this.requestTimes_ = []; - // Queue of ints keeping the time of errors. - this.errorTimes_ = []; + this.numErrors_ = 0; this.errorTimeout_ = 0; this.nextRequestTime_ = 0; - this.backoffTriggered_ = false; } /** * Reset the object for reuse. */ RequestBackoff.prototype.reset = function() { - this.errorTimes_ = []; + this.numErrors_ = 0; this.errorTimeout_ = 0; this.nextRequestTime_ = 0; - this.backoffTriggered_ = false; } /** @@ -94,7 +90,7 @@ RequestBackoff.prototype.reset = function() { */ RequestBackoff.prototype.canMakeRequest = function() { var now = Date.now(); - if (now <= this.nextRequestTime_) { + if (now < this.nextRequestTime_) { return false; } @@ -111,36 +107,29 @@ RequestBackoff.prototype.noteRequest = function() { this.requestTimes_.shift(); } +RequestBackoff.prototype.nextRequestDelay = function() { + return Math.max(0, this.nextRequestTime_ - Date.now()); +} + /** * Notify this object of the last server response. If it's an error, */ RequestBackoff.prototype.noteServerResponse = function(status) { if (this.isErrorStatus(status)) { - var now = Date.now(); - this.errorTimes_.push(now); + this.numErrors_++; - // We only care about keeping track of MAX_ERRORS - if (this.errorTimes_.length > this.MAX_ERRORS_) - this.errorTimes_.shift(); + if (this.numErrors_ < this.MAX_ERRORS_) + this.errorTimeout_ = this.RETRY_INCREMENT_; + else if (this.numErrors_ == this.MAX_ERRORS_) + this.errorTimeout_ = this.TIMEOUT_INCREMENT_; + else + this.errorTimeout_ *= 2; - // See if we hit the backoff case - // This either means we hit MAX_ERRORS in ERROR_PERIOD - // *or* we were already in a backoff state, in which case we - // increase our timeout. - if ((this.errorTimes_.length == this.MAX_ERRORS_ && - now - this.errorTimes_[0] < this.ERROR_PERIOD_) - || this.backoffTriggered_) { - this.errorTimeout_ = (this.errorTimeout_ * 2) + this.TIMEOUT_INCREMENT_; - this.errorTimeout_ = Math.min(this.errorTimeout_, this.MAX_TIMEOUT_); - this.nextRequestTime_ = now + this.errorTimeout_; - this.backoffTriggered_ = true; - } + this.errorTimeout_ = Math.min(this.errorTimeout_, this.MAX_TIMEOUT_); + this.nextRequestTime_ = Date.now() + this.errorTimeout_; } else { - // Reset error timeout, allow requests to go through, and switch out - // of backoff state. - this.errorTimeout_ = 0; - this.nextRequestTime_ = 0; - this.backoffTriggered_ = false; + // Reset error timeout, allow requests to go through. + this.reset(); } } diff --git a/mozilla/toolkit/components/url-classifier/tests/unit/test_backoff.js b/mozilla/toolkit/components/url-classifier/tests/unit/test_backoff.js index 3a44b0645ca..365568c479e 100644 --- a/mozilla/toolkit/components/url-classifier/tests/unit/test_backoff.js +++ b/mozilla/toolkit/components/url-classifier/tests/unit/test_backoff.js @@ -9,56 +9,69 @@ function setNow(time) { } function run_test() { - // 2 errors, 5ms time period, max 3 requests per ten milliseconds, - // 5ms backoff interval, 20ms max delay - var rb = new jslib.RequestBackoff(2, 5, 3, 10, 5, 20); + // 3 errors, 1ms retry period, max 3 requests per ten milliseconds, + // 5ms backoff interval, 19ms max delay + var rb = new jslib.RequestBackoff(3, 1, 3, 10, 5, 19); setNow(1); rb.noteServerResponse(200); do_check_true(rb.canMakeRequest()); - setNow(2); + do_check_true(rb.canMakeRequest()); + + // First error should trigger a 1ms delay rb.noteServerResponse(500); - do_check_true(rb.canMakeRequest()); - + do_check_false(rb.canMakeRequest()); + do_check_eq(rb.nextRequestTime_, 3); setNow(3); - rb.noteServerResponse(200); do_check_true(rb.canMakeRequest()); - // Trigger backoff + // Second error should also trigger a 1ms delay + rb.noteServerResponse(500); + do_check_false(rb.canMakeRequest()); + do_check_eq(rb.nextRequestTime_, 4); setNow(4); - rb.noteServerResponse(502); + do_check_true(rb.canMakeRequest()); + + // Third error should trigger a 5ms backoff + rb.noteServerResponse(500); do_check_false(rb.canMakeRequest()); do_check_eq(rb.nextRequestTime_, 9); + setNow(9); + do_check_true(rb.canMakeRequest()); // Trigger backoff again - setNow(10); - do_check_true(rb.canMakeRequest()); rb.noteServerResponse(503); do_check_false(rb.canMakeRequest()); - do_check_eq(rb.nextRequestTime_, 25); + do_check_eq(rb.nextRequestTime_, 19); + setNow(19); + do_check_true(rb.canMakeRequest()); // Trigger backoff a third time and hit max timeout - setNow(30); - do_check_true(rb.canMakeRequest()); rb.noteServerResponse(302); do_check_false(rb.canMakeRequest()); - do_check_eq(rb.nextRequestTime_, 50); + do_check_eq(rb.nextRequestTime_, 38); + setNow(38); + do_check_true(rb.canMakeRequest()); + + // One more backoff, should still be at the max timeout + rb.noteServerResponse(400); + do_check_false(rb.canMakeRequest()); + do_check_eq(rb.nextRequestTime_, 57); + setNow(57); + do_check_true(rb.canMakeRequest()); // Request goes through - setNow(100); - do_check_true(rb.canMakeRequest()); rb.noteServerResponse(200); do_check_true(rb.canMakeRequest()); do_check_eq(rb.nextRequestTime_, 0); - - // Another error (shouldn't trigger backoff) - setNow(101); + setNow(58); rb.noteServerResponse(500); - do_check_true(rb.canMakeRequest()); - // Another error, but not in ERROR_PERIOD, so it should be ok - setNow(107); - rb.noteServerResponse(500); + // Another error, should trigger a 1ms backoff + do_check_false(rb.canMakeRequest()); + do_check_eq(rb.nextRequestTime_, 59); + + setNow(59); do_check_true(rb.canMakeRequest()); setNow(200);