Bug 1012924 - Experiments should cancel their XMLHttpRequest on shutdown and should also set a reasonable timeout on them. r=gfritzsche, a=lmandel
authorBenjamin Smedberg <benjamin@smedbergs.us>
Mon, 01 Sep 2014 15:26:07 +0200
changeset 216677 db5539e42eb5
parent 216676 70930f30da0e
child 216678 e0ad01b2e26e
push id3872
push userryanvm@gmail.com
push date2014-09-08 19:43 +0000
treeherdermozilla-beta@d820ef3b256d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche, lmandel
bugs1012924
milestone33.0
Bug 1012924 - Experiments should cancel their XMLHttpRequest on shutdown and should also set a reasonable timeout on them. r=gfritzsche, a=lmandel
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
@@ -71,16 +71,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.
@@ -404,16 +405,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();
 };
 
@@ -484,16 +486,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;
@@ -958,28 +968,34 @@ 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.onload = function (event) {
+    xhr.onerror = errorhandler;
+    xhr.ontimeout = errorhandler;
+    xhr.onabort = errorhandler;
+
+    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;
       }
 
       let certs = null;
       if (gPrefs.get(PREF_MANIFEST_CHECKCERT, true)) {
         certs = CertUtils.readCertPrefs(PREF_BRANCH + "manifest.certs.");
       }
       try {
@@ -988,22 +1004,24 @@ Experiments.Experiments.prototype = {
       }
       catch (e) {
         log.error("manifest fetch failed certificate checks", [e]);
         deferred.reject(new Error("Experiments - manifest fetch failed certificate checks: " + e));
         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
@@ -17,8 +17,9 @@ generated-files =
 [test_cache.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]