Bug 1012924 - Experiments should cancel their XMLHttpRequest on shutdown, and should also set a reasonable timeout on them, r=gfritzsche
authorBenjamin Smedberg <benjamin@smedbergs.us>
Mon, 01 Sep 2014 15:26:07 +0200
changeset 224518 820c379866126e404c8bbf7b906a92e4e4d2e655
parent 224517 62b491425fa2f7beb0071b5d18eb843bc7a52c92
child 224519 8106c19ed3b064bb99bef163a4b7b91c262cfeaa
push id3979
push userraliiev@mozilla.com
push dateMon, 13 Oct 2014 16:35:44 +0000
treeherdermozilla-beta@30f2cc610691 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1012924
milestone34.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1012924 - Experiments should cancel their XMLHttpRequest on shutdown, and should also set a reasonable timeout on them, r=gfritzsche
browser/experiments/Experiments.jsm
browser/experiments/test/xpcshell/test_nethang_bug1012924.js
browser/experiments/test/xpcshell/xpcshell.ini
--- a/browser/experiments/Experiments.jsm
+++ b/browser/experiments/Experiments.jsm
@@ -60,16 +60,17 @@ const PREF_HEALTHREPORT_ENABLED = "datar
 
 const PREF_BRANCH_TELEMETRY     = "toolkit.telemetry.";
 const PREF_TELEMETRY_ENABLED    = "enabled";
 
 const URI_EXTENSION_STRINGS     = "chrome://mozapps/locale/extensions/extensions.properties";
 const STRING_TYPE_NAME          = "type.%ID%.name";
 
 const CACHE_WRITE_RETRY_DELAY_SEC = 60 * 3;
+const MANIFEST_FETCH_TIMEOUT_MSEC = 60 * 3 * 1000; // 3 minutes
 
 const TELEMETRY_LOG = {
   // log(key, [kind, experimentId, details])
   ACTIVATION_KEY: "EXPERIMENT_ACTIVATION",
   ACTIVATION: {
     // Successfully activated.
     ACTIVATED: "ACTIVATED",
     // Failed to install the add-on.
@@ -401,16 +402,17 @@ Experiments.Experiments = function (poli
   // * disabling/enabling experiments
   // * saving the cache (if _dirty)
   this._mainTask = null;
 
   // Timer for re-evaluating experiment status.
   this._timer = null;
 
   this._shutdown = false;
+  this._networkRequest = null;
 
   // We need to tell when we first evaluated the experiments to fire an
   // experiments-changed notification when we only loaded completed experiments.
   this._firstEvaluate = true;
 
   this.init();
 };
 
@@ -481,16 +483,24 @@ Experiments.Experiments.prototype = {
 
       if (this._timer) {
         this._timer.clear();
       }
     }
 
     this._shutdown = true;
     if (this._mainTask) {
+      if (this._networkRequest) {
+	try {
+	  this._log.trace("Aborting pending network request: " + this._networkRequest);
+	  this._networkRequest.abort();
+	} catch (e) {
+	  // pass
+	}
+      }
       try {
         this._log.trace("uninit: waiting on _mainTask");
         yield this._mainTask;
       } catch (e if e instanceof AlreadyShutdownError) {
         // We error out of tasks after shutdown via that exception.
       } catch (e) {
         this._latestError = e;
         throw e;
@@ -955,38 +965,46 @@ Experiments.Experiments.prototype = {
     let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
     try {
       xhr.open("GET", url);
     } catch (e) {
       this._log.error("httpGetRequest() - Error opening request to " + url + ": " + e);
       return Promise.reject(new Error("Experiments - Error opening XHR for " + url));
     }
 
+    this._networkRequest = xhr;
     let deferred = Promise.defer();
 
     let log = this._log;
-    xhr.onerror = function (e) {
-      log.error("httpGetRequest::onError() - Error making request to " + url + ": " + e.error);
-      deferred.reject(new Error("Experiments - XHR error for " + url + " - " + e.error));
+    let errorhandler = (evt) => {
+      log.error("httpGetRequest::onError() - Error making request to " + url + ": " + evt.type);
+      deferred.reject(new Error("Experiments - XHR error for " + url + " - " + evt.type));
+      this._networkRequest = null;
     };
+    xhr.onerror = errorhandler;
+    xhr.ontimeout = errorhandler;
+    xhr.onabort = errorhandler;
 
-    xhr.onload = function (event) {
+    xhr.onload = (event) => {
       if (xhr.status !== 200 && xhr.state !== 0) {
         log.error("httpGetRequest::onLoad() - Request to " + url + " returned status " + xhr.status);
         deferred.reject(new Error("Experiments - XHR status for " + url + " is " + xhr.status));
+	this._networkRequest = null;
         return;
       }
 
       deferred.resolve(xhr.responseText);
+      this._networkRequest = null;
     };
 
     if (xhr.channel instanceof Ci.nsISupportsPriority) {
       xhr.channel.priority = Ci.nsISupportsPriority.PRIORITY_LOWEST;
     }
 
+    xhr.timeout = MANIFEST_FETCH_TIMEOUT_MSEC;
     xhr.send(null);
     return deferred.promise;
   },
 
   /*
    * Path of the cache file we use in the profile.
    */
   get _cacheFilePath() {
new file mode 100644
--- /dev/null
+++ b/browser/experiments/test/xpcshell/test_nethang_bug1012924.js
@@ -0,0 +1,47 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+Cu.import("resource://testing-common/httpd.js");
+Cu.import("resource:///modules/experiments/Experiments.jsm");
+
+const MANIFEST_HANDLER         = "manifests/handler";
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* test_setup() {
+  loadAddonManager();
+  do_get_profile();
+
+  let httpServer = new HttpServer();
+  httpServer.start(-1);
+  let port = httpServer.identity.primaryPort;
+  let httpRoot = "http://localhost:" + port + "/";
+  let handlerURI = httpRoot + MANIFEST_HANDLER;
+  httpServer.registerPathHandler("/" + MANIFEST_HANDLER,
+    (request, response) => {
+      response.processAsync();
+      response.setStatus(null, 200, "OK");
+      response.write("["); // never finish!
+    });
+
+  do_register_cleanup(() => httpServer.stop(() => {}));
+  Services.prefs.setBoolPref(PREF_EXPERIMENTS_ENABLED, true);
+  Services.prefs.setIntPref(PREF_LOGGING_LEVEL, 0);
+  Services.prefs.setBoolPref(PREF_LOGGING_DUMP, true);
+  Services.prefs.setCharPref(PREF_MANIFEST_URI, handlerURI);
+  Services.prefs.setIntPref(PREF_FETCHINTERVAL, 0);
+
+  let experiments = Experiments.instance();
+  experiments.updateManifest().then(
+    () => {
+      Assert.ok(true, "updateManifest finished successfully");
+    },
+    (e) => {
+      do_throw("updateManifest should not have failed: got error " + e);
+    });
+  yield experiments.uninit();
+});
--- a/browser/experiments/test/xpcshell/xpcshell.ini
+++ b/browser/experiments/test/xpcshell/xpcshell.ini
@@ -20,8 +20,9 @@ generated-files =
 [test_cacherace.js]
 [test_conditions.js]
 [test_disableExperiments.js]
 [test_fetch.js]
 [test_telemetry.js]
 [test_healthreport.js]
 [test_previous_provider.js]
 [test_upgrade.js]
+[test_nethang_bug1012924.js]