Bug 989137 - Part 9: Ensure add-ons are enabled when they should be; r=Unfocused, r=gfritzsche
☠☠ backed out by 317a809acdc8 ☠ ☠
authorGregory Szorc <gps@mozilla.com>
Fri, 04 Apr 2014 15:58:27 -0700
changeset 177139 d3053c4e4c5143360df21f4c965e9f21ab8e3f2e
parent 177138 7e410c1d61e6e8eb6690634933d978fdb1e675e6
child 177140 831a3ccce1000eec241d443dc16d7c72f7a9499a
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersUnfocused, gfritzsche
bugs989137
milestone31.0a1
Bug 989137 - Part 9: Ensure add-ons are enabled when they should be; r=Unfocused, r=gfritzsche Experiment add-ons are disabled by default in the Addon Manager. This patch will manually enable them if necessary.
browser/experiments/Experiments.jsm
browser/experiments/test/xpcshell/test_activate.js
browser/experiments/test/xpcshell/test_api.js
--- a/browser/experiments/Experiments.jsm
+++ b/browser/experiments/Experiments.jsm
@@ -207,16 +207,20 @@ function uninstallAddons(addons) {
       AddonManager.removeAddonListener(listener);
       deferred.resolve();
     }
   };
 
   AddonManager.addAddonListener(listener);
 
   for (let addon of addons) {
+    // Disabling the add-on before uninstalling is necessary to cause tests to
+    // pass. This might be indicative of a bug in XPIProvider.
+    // TODO follow up in bug 992396.
+    addon.userDisabled = true;
     addon.uninstall();
   }
 
   return deferred.promise;
 }
 
 /**
  * The experiments module.
@@ -967,16 +971,18 @@ Experiments.Experiments.prototype = {
             yield activeExperiment.start();
           } catch (e) {
             this._log.error(e);
             // On failure try the next experiment.
             activeExperiment = null;
           }
           this._dirty = true;
           activeChanged = true;
+        } else {
+          yield activeExperiment.ensureActive();
         }
       } finally {
         this._pendingUninstall = null;
       }
     }
     this._terminateReason = null;
 
     if (!activeExperiment) {
@@ -1468,16 +1474,18 @@ Experiments.ExperimentEntry.prototype = 
 
       TelemetryLog.log(TELEMETRY_LOG.ACTIVATION_KEY,
                       [TELEMETRY_LOG.ACTIVATION.INSTALL_FAILURE, this.id]);
 
       deferred.reject(new Error(message));
     };
 
     let listener = {
+      _expectedID: null,
+
       onDownloadEnded: install => {
         this._log.trace("_installAddon() - onDownloadEnded for " + this.id);
 
         if (install.existingAddon) {
           this._log.warn("_installAddon() - onDownloadEnded, addon already installed");
         }
 
         if (install.addon.type !== "experiment") {
@@ -1492,19 +1500,16 @@ Experiments.ExperimentEntry.prototype = 
         if (install.existingAddon) {
           this._log.warn("_installAddon() - onInstallStarted, addon already installed");
         }
 
         if (install.addon.type !== "experiment") {
           this._log.error("_installAddon() - onInstallStarted, wrong addon type");
           return false;
         }
-
-        // Experiment add-ons default to userDisabled = true.
-        install.addon.userDisabled = false;
       },
 
       onInstallEnded: install => {
         this._log.trace("_installAddon() - install ended for " + this.id);
         gActiveInstallURLs.delete(install.sourceURI.spec);
 
         this._lastChangedDate = this._policy.now();
         this._startDate = this._policy.now();
@@ -1514,16 +1519,36 @@ Experiments.ExperimentEntry.prototype = 
                        [TELEMETRY_LOG.ACTIVATION.ACTIVATED, this.id]);
 
         let addon = install.addon;
         this._name = addon.name;
         this._addonId = addon.id;
         this._description = addon.description || "";
         this._homepageURL = addon.homepageURL || "";
 
+        // Experiment add-ons default to userDisabled=true. Enable if needed.
+        if (addon.userDisabled) {
+          this._log.trace("Add-on is disabled. Enabling.");
+          listener._expectedID = addon.id;
+          AddonManager.addAddonListener(listener);
+          addon.userDisabled = false;
+        } else {
+          this._log.trace("Add-on is enabled. start() completed.");
+          deferred.resolve();
+        }
+      },
+
+      onEnabled: addon => {
+        this._log.info("onEnabled() for " + addon.id);
+
+        if (addon.id != listener._expectedID) {
+          return;
+        }
+
+        AddonManager.removeAddonListener(listener);
         deferred.resolve();
       },
     };
 
     ["onDownloadCancelled", "onDownloadFailed", "onInstallCancelled", "onInstallFailed"]
       .forEach(what => {
         listener[what] = install => failureHandler(install, what)
       });
@@ -1551,33 +1576,88 @@ Experiments.ExperimentEntry.prototype = 
     this._enabled = false;
     let deferred = Promise.defer();
     let updateDates = () => {
       let now = this._policy.now();
       this._lastChangedDate = now;
       this._endDate = now;
     };
 
-    AddonManager.getAddonByID(this._addonId, addon => {
+    this._getAddon().then((addon) => {
       if (!addon) {
         let message = "could not get Addon for " + this.id;
         this._log.warn("stop() - " + message);
         updateDates();
         deferred.resolve();
         return;
       }
 
       updateDates();
       this._logTermination(terminationKind, terminationReason);
       deferred.resolve(uninstallAddons([addon]));
     });
 
     return deferred.promise;
   },
 
+  /**
+   * Try to ensure this experiment is active.
+   *
+   * The returned promise will be resolved if the experiment is active
+   * in the Addon Manager or rejected if it isn't.
+   *
+   * @return Promise<>
+   */
+  ensureActive: Task.async(function* () {
+    this._log.trace("ensureActive() for " + this.id);
+
+    let addon = yield this._getAddon();
+    if (!addon) {
+      this._log.warn("Experiment is not installed: " + this._addonId);
+      throw new Error("Experiment is not installed: " + this._addonId);
+    }
+
+    // User disabled likely means the experiment is disabled at startup,
+    // since the permissions don't allow it to be disabled by the user.
+    if (!addon.userDisabled) {
+      return;
+    }
+
+    let deferred = Promise.defer();
+
+    let listener = {
+      onEnabled: enabledAddon => {
+        if (enabledAddon.id != addon.id) {
+          return;
+        }
+
+        AddonManager.removeAddonListener(listener);
+        deferred.resolve();
+      },
+    };
+
+    this._log.info("Activating add-on: " + addon.id);
+    AddonManager.addAddonListener(listener);
+    addon.userDisabled = false;
+    yield deferred.promise;
+  }),
+
+  /**
+   * Obtain the underlying Addon from the Addon Manager.
+   *
+   * @return Promise<Addon|null>
+   */
+  _getAddon: function () {
+    let deferred = Promise.defer();
+
+    AddonManager.getAddonByID(this._addonId, deferred.resolve);
+
+    return deferred.promise;
+  },
+
   _logTermination: function (terminationKind, terminationReason) {
     if (terminationKind === undefined) {
       return;
     }
 
     if (!(terminationKind in TELEMETRY_LOG.TERMINATION)) {
       this._log.warn("stop() - unknown terminationKind " + terminationKind);
       return;
--- a/browser/experiments/test/xpcshell/test_activate.js
+++ b/browser/experiments/test/xpcshell/test_activate.js
@@ -94,35 +94,55 @@ add_task(function* test_startStop() {
 
   let result;
 
   defineNow(gPolicy, baseDate);
   result = yield isApplicable(experiment);
   Assert.equal(result.applicable, false, "Experiment should not be applicable.");
   Assert.equal(experiment.enabled, false, "Experiment should not be enabled.");
 
+  let addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 0, "No experiment add-ons are installed.");
+
   defineNow(gPolicy, futureDate(startDate, 5 * MS_IN_ONE_DAY));
   result = yield isApplicable(experiment);
   Assert.equal(result.applicable, true, "Experiment should now be applicable.");
   Assert.equal(experiment.enabled, false, "Experiment should not be enabled.");
 
   yield experiment.start();
+  addons = yield getExperimentAddons();
   Assert.equal(experiment.enabled, true, "Experiment should now be enabled.");
+  Assert.equal(addons.length, 1, "1 experiment add-on is installed.");
+  Assert.equal(addons[0].id, experiment._addonId, "The add-on is the one we expect.");
+  Assert.equal(addons[0].userDisabled, false, "The add-on is not userDisabled.");
+  Assert.ok(addons[0].isActive, "The add-on is active.");
 
   yield experiment.stop();
+  addons = yield getExperimentAddons();
   Assert.equal(experiment.enabled, false, "Experiment should not be enabled.");
+  Assert.equal(addons.length, 0, "Experiment should be uninstalled from the Addon Manager.");
 
   yield experiment.start();
+  addons = yield getExperimentAddons();
   Assert.equal(experiment.enabled, true, "Experiment should now be enabled.");
+  Assert.equal(addons.length, 1, "1 experiment add-on is installed.");
+  Assert.equal(addons[0].id, experiment._addonId, "The add-on is the one we expect.");
+  Assert.equal(addons[0].userDisabled, false, "The add-on is not userDisabled.");
+  Assert.ok(addons[0].isActive, "The add-on is active.");
 
   let result = yield experiment._shouldStop();
   Assert.equal(result.shouldStop, false, "shouldStop should be false.");
   let maybeStop = yield experiment.maybeStop();
   Assert.equal(maybeStop, false, "Experiment should not have been stopped.");
   Assert.equal(experiment.enabled, true, "Experiment should be enabled.");
+  addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 1, "Experiment still in add-ons manager.");
+  Assert.ok(addons[0].isActive, "The add-on is still active.");
 
   defineNow(gPolicy, futureDate(endDate, MS_IN_ONE_DAY));
   result = yield experiment._shouldStop();
   Assert.equal(result.shouldStop, true, "shouldStop should now be true.");
   maybeStop = yield experiment.maybeStop();
   Assert.equal(maybeStop, true, "Experiment should have been stopped.");
   Assert.equal(experiment.enabled, false, "Experiment should be disabled.");
+  addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 0, "Experiment add-on is uninstalled.");
 });
--- a/browser/experiments/test/xpcshell/test_api.js
+++ b/browser/experiments/test/xpcshell/test_api.js
@@ -1412,8 +1412,57 @@ add_task(function* testForeignExperiment
   Assert.equal(addons.length, 0, "Precondition: No experiment add-ons present.");
   yield installAddon(gDataRoot + EXPERIMENT1_XPI_NAME, EXPERIMENT1_XPI_SHA1);
   addons = yield getExperimentAddons();
   Assert.equal(addons.length, 0, "Add-on install should have been cancelled.");
 
   yield experiments.uninit();
   yield removeCacheFile();
 });
+
+// Experiment add-ons will be disabled after Addon Manager restarts. Ensure
+// we enable them automatically.
+add_task(function* testEnabledAfterRestart() {
+  let experiments = new Experiments.Experiments(gPolicy);
+
+  gManifestObject = {
+    "version": 1,
+    experiments: [
+      {
+        id: EXPERIMENT1_ID,
+        xpiURL: gDataRoot + EXPERIMENT1_XPI_NAME,
+        xpiHash: EXPERIMENT1_XPI_SHA1,
+        startTime: gPolicy.now().getTime() / 1000 - 60,
+        endTime: gPolicy.now().getTime() / 1000 + 60,
+        maxActiveSeconds: 10 * SEC_IN_ONE_DAY,
+        appName: ["XPCShell"],
+        channel: ["nightly"],
+      },
+    ],
+  };
+
+  let addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 0, "Precondition: No experimenta add-ons installed.");
+
+  yield experiments.updateManifest();
+  let fromManifest = yield experiments.getExperiments();
+  Assert.equal(fromManifest.length, 1, "A single experiment is known.");
+
+  addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 1, "A single experiment add-on is installed.");
+  Assert.ok(addons[0].isActive, "That experiment is active.");
+
+  dump("Restarting Addon Manager\n");
+  experiments._stopWatchingAddons();
+  restartManager();
+  experiments._startWatchingAddons();
+
+  addons = yield getExperimentAddons();
+  Assert.equal(addons.length, 1, "The experiment is still there after restart.");
+  Assert.ok(addons[0].userDisabled, "But it is disabled.");
+  Assert.equal(addons[0].isActive, false, "And not active.");
+
+  yield experiments.updateManifest();
+  Assert.ok(addons[0].isActive, "It activates when the manifest is evaluated.");
+
+  yield experiments.uninit();
+  yield removeCacheFile();
+});