Bug 989137 - Part 12: Refactor experiment and add-on state management. r=gfritzsche
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Thu, 17 Apr 2014 15:47:36 +0200
changeset 179569 cb9a5809f6737c3ce4a0f1d63adf93b9f4aaac89
parent 179568 b960c2026325596fe588bc8ff7666ca588298aab
child 179570 9d5d5da02bf259b6301d81e9c4e752c65cfa81e8
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersgfritzsche
bugs989137
milestone31.0a1
Bug 989137 - Part 12: Refactor experiment and add-on state management. r=gfritzsche
browser/experiments/Experiments.jsm
browser/experiments/moz.build
browser/experiments/test/xpcshell/test_activate.js
browser/experiments/test/xpcshell/test_api.js
browser/experiments/test/xpcshell/test_cache.js
browser/experiments/test/xpcshell/test_telemetry.js
--- a/browser/experiments/Experiments.jsm
+++ b/browser/experiments/Experiments.jsm
@@ -37,21 +37,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 // would.
 XPCOMUtils.defineLazyGetter(this, "CertUtils",
   function() {
     var mod = {};
     Cu.import("resource://gre/modules/CertUtils.jsm", mod);
     return mod;
   });
 
-#ifdef MOZ_CRASHREPORTER
 XPCOMUtils.defineLazyServiceGetter(this, "gCrashReporter",
                                    "@mozilla.org/xre/app-info;1",
                                    "nsICrashReporter");
-#endif
 
 const FILE_CACHE                = "experiments.json";
 const OBSERVER_TOPIC            = "experiments-changed";
 const MANIFEST_VERSION          = 1;
 const CACHE_VERSION             = 1;
 
 const KEEP_HISTORY_N_DAYS       = 180;
 const MIN_EXPERIMENT_ACTIVE_SECONDS = 60;
@@ -71,46 +69,59 @@ const PREF_HEALTHREPORT_ENABLED = "datar
 
 const PREF_BRANCH_TELEMETRY     = "toolkit.telemetry.";
 const PREF_TELEMETRY_ENABLED    = "enabled";
 
 const TELEMETRY_LOG = {
   // log(key, [kind, experimentId, details])
   ACTIVATION_KEY: "EXPERIMENT_ACTIVATION",
   ACTIVATION: {
-    ACTIVATED: "ACTIVATED",             // successfully activated
-    INSTALL_FAILURE: "INSTALL_FAILURE", // failed to install the extension
-    REJECTED: "REJECTED",               // experiment was rejected because of it's conditions,
-                                        // provides details on which
+    // Successfully activated.
+    ACTIVATED: "ACTIVATED",
+    // Failed to install the add-on.
+    INSTALL_FAILURE: "INSTALL_FAILURE",
+    // Experiment does not meet activation requirements. Details will
+    // be provided.
+    REJECTED: "REJECTED",
   },
 
   // log(key, [kind, experimentId, optionalDetails...])
   TERMINATION_KEY: "EXPERIMENT_TERMINATION",
   TERMINATION: {
-    USERDISABLED: "USERDISABLED", // the user disabled this experiment
-    FROM_API: "FROM_API",         // the experiment disabled itself
-    EXPIRED: "EXPIRED",           // experiment expired e.g. by exceeding the end-date
-    RECHECK: "RECHECK",           // disabled after re-evaluating conditions,
-                                  // provides details on which
+    // The Experiments service was disabled.
+    SERVICE_DISABLED: "SERVICE_DISABLED",
+    // Add-on uninstalled.
+    ADDON_UNINSTALLED: "ADDON_UNINSTALLED",
+    // The experiment disabled itself.
+    FROM_API: "FROM_API",
+    // The experiment expired (e.g. by exceeding the end date).
+    EXPIRED: "EXPIRED",
+    // Disabled after re-evaluating conditions. If this is specified,
+    // details will be provided.
+    RECHECK: "RECHECK",
   },
 };
 
 const gPrefs = new Preferences(PREF_BRANCH);
 const gPrefsTelemetry = new Preferences(PREF_BRANCH_TELEMETRY);
 let gExperimentsEnabled = false;
 let gExperiments = null;
 let gLogAppenderDump = null;
 let gPolicyCounter = 0;
 let gExperimentsCounter = 0;
 let gExperimentEntryCounter = 0;
 
 // Tracks active AddonInstall we know about so we can deny external
 // installs.
 let gActiveInstallURLs = new Set();
 
+// Tracks add-on IDs that are being uninstalled by us. This allows us
+// to differentiate between expected uninstalled and user-driven uninstalls.
+let gActiveUninstallAddonIDs = new Set();
+
 let gLogger;
 let gLogDumping = false;
 
 function configureLogging() {
   if (!gLogger) {
     gLogger = Log.repository.getLogger("Browser.Experiments");
     gLogger.addAppender(new Log.ConsoleAppender(new Log.BasicFormatter()));
   }
@@ -331,19 +342,16 @@ Experiments.Experiments = function (poli
   this._experiments = null;
   this._refresh = false;
   this._terminateReason = null; // or TELEMETRY_LOG.TERMINATION....
   this._dirty = false;
 
   // Loading the cache happens once asynchronously on startup
   this._loadTask = null;
 
-  // Ignore addon-manager notifications for addons that we are uninstalling ourself
-  this._pendingUninstall = null;
-
   // The _main task handles all other actions:
   // * refreshing the manifest off the network (if _refresh)
   // * disabling/enabling experiments
   // * saving the cache (if _dirty)
   this._mainTask = null;
 
   // Timer for re-evaluating experiment status.
   this._timer = null;
@@ -452,34 +460,34 @@ Experiments.Experiments.prototype = {
   /**
    * Toggle whether the experiments feature is enabled or not.
    */
   set enabled(enabled) {
     this._log.trace("set enabled(" + enabled + ")");
     gPrefs.set(PREF_ENABLED, enabled);
   },
 
-  _toggleExperimentsEnabled: function (enabled) {
+  _toggleExperimentsEnabled: Task.async(function* (enabled) {
     this._log.trace("_toggleExperimentsEnabled(" + enabled + ")");
     let wasEnabled = gExperimentsEnabled;
     gExperimentsEnabled = enabled && telemetryEnabled();
 
     if (wasEnabled == gExperimentsEnabled) {
       return;
     }
 
     if (gExperimentsEnabled) {
-      this.updateManifest();
+      yield this.updateManifest();
     } else {
-      this.disableExperiment();
+      yield this.disableExperiment(TELEMETRY_LOG.TERMINATION.SERVICE_DISABLED);
       if (this._timer) {
         this._timer.clear();
       }
     }
-  },
+  }),
 
   _telemetryStatusChanged: function () {
     this._toggleExperimentsEnabled(gExperimentsEnabled);
   },
 
   /**
    * Returns a promise that is resolved with an array of `ExperimentInfo` objects,
    * which provide info on the currently and recently active experiments.
@@ -651,39 +659,28 @@ Experiments.Experiments.prototype = {
   notify: function (timer) {
     this._log.trace("notify()");
     this._checkForShutdown();
     return this._run();
   },
 
   // START OF ADD-ON LISTENERS
 
-  onDisabled: function (addon) {
-    this._log.trace("onDisabled() - addon id: " + addon.id);
-    if (addon.id == this._pendingUninstall) {
-      return;
-    }
-    let activeExperiment = this._getActiveExperiment();
-    if (!activeExperiment || activeExperiment._addonId != addon.id) {
-      return;
-    }
-    this.disableExperiment();
-  },
-
   onUninstalled: function (addon) {
     this._log.trace("onUninstalled() - addon id: " + addon.id);
-    if (addon.id == this._pendingUninstall) {
+    if (gActiveUninstallAddonIDs.has(addon.id)) {
       this._log.trace("matches pending uninstall");
       return;
     }
     let activeExperiment = this._getActiveExperiment();
     if (!activeExperiment || activeExperiment._addonId != addon.id) {
       return;
     }
-    this.disableExperiment();
+
+    this.disableExperiment(TELEMETRY_LOG.TERMINATION.ADDON_UNINSTALLED);
   },
 
   onInstallStarted: function (install) {
     if (install.addon.type != "experiment") {
       return;
     }
 
     // We want to be in control of all experiment add-ons: reject installs
@@ -911,25 +908,27 @@ Experiments.Experiments.prototype = {
       this._log.error("getActiveExperimentId() - should not have more than 1 active experiment");
       throw new Error("have more than 1 active experiment");
     }
 
     return null;
   },
 
   /**
-   * Disable an experiment by id.
-   * @param experimentId The id of the experiment.
-   * @param userDisabled (optional) Whether this is disabled as a result of a user action.
+   * Disables all active experiments.
+   *
    * @return Promise<> Promise that will get resolved once the task is done or failed.
    */
-  disableExperiment: function (userDisabled=true) {
+  disableExperiment: function (reason) {
+    if (!reason) {
+      throw new Error("Must specify a termination reason.");
+    }
+
     this._log.trace("disableExperiment()");
-
-    this._terminateReason = userDisabled ? TELEMETRY_LOG.TERMINATION.USERDISABLED : TELEMETRY_LOG.TERMINATION.FROM_API;
+    this._terminateReason = reason;
     return this._run();
   },
 
   /**
    * The Set of add-on IDs that we know about from manifests.
    */
   get _trackedAddonIds() {
     if (!this._experiments) {
@@ -976,54 +975,53 @@ Experiments.Experiments.prototype = {
     let activeChanged = false;
     let now = this._policy.now();
 
     if (!activeExperiment) {
       // Avoid this pref staying out of sync if there were e.g. crashes.
       gPrefs.set(PREF_ACTIVE_EXPERIMENT, false);
     }
 
+    // Ensure the active experiment is in the proper state. This may install,
+    // uninstall, upgrade, or enable the experiment add-on. What exactly is
+    // abstracted away from us by design.
     if (activeExperiment) {
-      this._pendingUninstall = activeExperiment._addonId;
-      try {
-        let wasStopped;
-        if (this._terminateReason) {
-          yield activeExperiment.stop(this._terminateReason);
-          wasStopped = true;
+      let changes;
+      let shouldStopResult = yield activeExperiment.shouldStop();
+      if (shouldStopResult.shouldStop) {
+        let expireReasons = ["endTime", "maxActiveSeconds"];
+        let kind, reason;
+
+        if (expireReasons.indexOf(shouldStopResult.reason[0]) != -1) {
+          kind = TELEMETRY_LOG.TERMINATION.EXPIRED;
+          reason = null;
         } else {
-          wasStopped = yield activeExperiment.maybeStop();
+          kind = TELEMETRY_LOG.TERMINATION.RECHECK;
+          reason = shouldStopResult.reason;
         }
-        if (wasStopped) {
-          this._dirty = true;
-          this._log.debug("evaluateExperiments() - stopped experiment "
-                        + activeExperiment.id);
-          activeExperiment = null;
-          activeChanged = true;
-        } else if (!gExperimentsEnabled) {
-          // No further actions if the feature is disabled.
-        } else if (activeExperiment.needsUpdate) {
-          this._log.debug("evaluateExperiments() - updating experiment "
-                        + activeExperiment.id);
-          try {
-            yield activeExperiment.stop();
-            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;
+        changes = yield activeExperiment.stop(kind, reason);
+      }
+      else if (this._terminateReason) {
+        changes = yield activeExperiment.stop(this._terminateReason);
+      }
+      else {
+        changes = yield activeExperiment.reconcileAddonState();
+      }
+
+      if (changes) {
+        this._dirty = true;
+        activeChanged = true;
+      }
+
+      if (!activeExperiment._enabled) {
+        activeExperiment = null;
+        activeChanged = true;
       }
     }
+
     this._terminateReason = null;
 
     if (!activeExperiment && gExperimentsEnabled) {
       for (let [id, experiment] of this._experiments) {
         let applicable;
         let reason = null;
         try {
           applicable = yield experiment.isApplicable();
@@ -1036,40 +1034,45 @@ Experiments.Experiments.prototype = {
         if (!applicable && reason && reason[0] != "was-active") {
           // Report this from here to avoid over-reporting.
           let desc = TELEMETRY_LOG.ACTIVATION;
           let data = [TELEMETRY_LOG.ACTIVATION.REJECTED, id];
           data = data.concat(reason);
           TelemetryLog.log(TELEMETRY_LOG.ACTIVATION_KEY, data);
         }
 
-        if (applicable) {
-          this._log.debug("evaluateExperiments() - activating experiment " + id);
-          try {
-            yield experiment.start();
-            activeChanged = true;
-            activeExperiment = experiment;
-            this._dirty = true;
-            break;
-          } catch (e) {
-            // On failure try the next experiment.
-          }
+        if (!applicable) {
+          continue;
+        }
+
+        this._log.debug("evaluateExperiments() - activating experiment " + id);
+        try {
+          yield experiment.start();
+          activeChanged = true;
+          activeExperiment = experiment;
+          this._dirty = true;
+          break;
+        } catch (e) {
+          // On failure, clean up the best we can and try the next experiment.
+          this._log.error("evaluateExperiments() - Unable to start experiment: " + e.message);
+          experiment._enabled = false;
+          yield experiment.reconcileAddonState();
         }
       }
     }
 
+    gPrefs.set(PREF_ACTIVE_EXPERIMENT, activeExperiment != null);
+
     if (activeChanged) {
       Services.obs.notifyObservers(null, OBSERVER_TOPIC, null);
     }
 
-#ifdef MOZ_CRASHREPORTER
-    if (activeExperiment) {
+    if ("@mozilla.org/toolkit/crash-reporter;1" in Cc && activeExperiment) {
       gCrashReporter.annotateCrashReport("ActiveExperiment", activeExperiment.id);
     }
-#endif
   },
 
   /*
    * Schedule the soonest re-check of experiment applicability that is needed.
    */
   _scheduleNextRun: function () {
     this._checkForShutdown();
 
@@ -1111,17 +1114,17 @@ Experiments.Experiments.prototype = {
  */
 
 Experiments.ExperimentEntry = function (policy) {
   this._policy = policy || new Experiments.Policy();
   this._log = Log.repository.getLoggerWithMessagePrefix(
     "Browser.Experiments.Experiments",
     "ExperimentEntry #" + gExperimentEntryCounter++ + "::");
 
-  // Is this experiment running?
+  // Is the experiment supposed to be running.
   this._enabled = false;
   // When this experiment was started, if ever.
   this._startDate = null;
   // When this experiment was ended, if ever.
   this._endDate = null;
   // The condition data from the manifest.
   this._manifestData = null;
   // For an active experiment, signifies whether we need to update the xpi.
@@ -1182,16 +1185,21 @@ Experiments.ExperimentEntry.prototype = 
     "_endDate",
   ]),
 
   DATE_KEYS: new Set([
     "_startDate",
     "_endDate",
   ]),
 
+  ADDON_CHANGE_NONE: 0,
+  ADDON_CHANGE_INSTALL: 1,
+  ADDON_CHANGE_UNINSTALL: 2,
+  ADDON_CHANGE_ENABLE: 4,
+
   /*
    * Initialize entry from the manifest.
    * @param data The experiment data from the manifest.
    * @return boolean Whether initialization succeeded.
    */
   initFromManifestData: function (data) {
     if (!this._isManifestDataValid(data)) {
       return false;
@@ -1471,36 +1479,28 @@ Experiments.ExperimentEntry.prototype = 
       }
 
       throw new Task.Result(true);
     }.bind(this));
   },
 
   /*
    * Start running the experiment.
+   *
    * @return Promise<> Resolved when the operation is complete.
    */
-  start: function () {
+  start: Task.async(function* () {
     this._log.trace("start() for " + this.id);
 
-    return Task.spawn(function* ExperimentEntry_start_task() {
-      let addons = yield installedExperimentAddons();
-      if (addons.length > 0) {
-        this._log.error("start() - there are already "
-                        + addons.length + " experiment addons installed");
-        yield uninstallAddons(addons);
-      }
-
-      yield this._installAddon();
-      gPrefs.set(PREF_ACTIVE_EXPERIMENT, true);
-    }.bind(this));
-  },
+    this._enabled = true;
+    return yield this.reconcileAddonState();
+  }),
 
   // Async install of the addon for this experiment, part of the start task above.
-  _installAddon: function* () {
+  _installAddon: Task.async(function* () {
     let deferred = Promise.defer();
 
     let install = yield addonInstallForURL(this._manifestData.xpiURL,
                                            this._manifestData.xpiHash);
     gActiveInstallURLs.add(install.sourceURI.spec);
 
     let failureHandler = (install, handler) => {
       let message = "AddonInstall " + handler + " for " + this.id + ", state=" +
@@ -1588,108 +1588,146 @@ Experiments.ExperimentEntry.prototype = 
     ["onDownloadCancelled", "onDownloadFailed", "onInstallCancelled", "onInstallFailed"]
       .forEach(what => {
         listener[what] = install => failureHandler(install, what)
       });
 
     install.addListener(listener);
     install.install();
 
-    return deferred.promise;
-  },
+    return yield deferred.promise;
+  }),
 
-  /*
+  /**
    * Stop running the experiment if it is active.
-   * @param terminationKind (optional) The termination kind, e.g. USERDISABLED or EXPIRED.
-   * @param terminationReason (optional) The termination reason details for
-   *                          termination kind RECHECK.
+   *
+   * @param terminationKind (optional)
+   *        The termination kind, e.g. ADDON_UNINSTALLED or EXPIRED.
+   * @param terminationReason (optional)
+   *        The termination reason details for termination kind RECHECK.
    * @return Promise<> Resolved when the operation is complete.
    */
-  stop: function (terminationKind, terminationReason) {
+  stop: Task.async(function* (terminationKind, terminationReason) {
     this._log.trace("stop() - id=" + this.id + ", terminationKind=" + terminationKind);
     if (!this._enabled) {
-      this._log.warning("stop() - experiment not enabled: " + id);
-      return Promise.reject();
+      throw new Error("Must not call stop() on an inactive experiment.");
     }
 
     this._enabled = false;
-    gPrefs.set(PREF_ACTIVE_EXPERIMENT, false);
+
+    let changes = yield this.reconcileAddonState();
+    let now = this._policy.now();
+    this._lastChangedDate = now;
+    this._endDate = now;
+    this._logTermination(terminationKind, terminationReason);
+
+    return changes;
+  }),
 
-    let deferred = Promise.defer();
-    let updateDates = () => {
-      let now = this._policy.now();
-      this._lastChangedDate = now;
-      this._endDate = now;
-    };
+  /**
+   * Reconcile the state of the add-on against what it's supposed to be.
+   *
+   * If we are active, ensure the add-on is enabled and up to date.
+   *
+   * If we are inactive, ensure the add-on is not installed.
+   */
+  reconcileAddonState: Task.async(function* () {
+    this._log.trace("reconcileAddonState()");
 
-    this._getAddon().then((addon) => {
-      if (!addon) {
-        let message = "could not get Addon for " + this.id;
-        this._log.warn("stop() - " + message);
-        updateDates();
-        deferred.resolve();
-        return;
+    if (!this._enabled) {
+      if (!this._addonId) {
+        this._log.trace("reconcileAddonState() - Experiment is not enabled and " +
+                        "has no add-on. Doing nothing.");
+        return this.ADDON_CHANGE_NONE;
       }
 
-      updateDates();
-      this._logTermination(terminationKind, terminationReason);
-      deferred.resolve(uninstallAddons([addon]));
-    });
+      let addon = yield this._getAddon();
+      if (!addon) {
+        this._log.trace("reconcileAddonState() - Inactive experiment has no " +
+                        "add-on. Doing nothing.");
+        return this.ADDON_CHANGE_NONE;
+      }
 
-    return deferred.promise;
-  },
+      this._log.info("reconcileAddonState() - Uninstalling add-on for inactive " +
+                     "experiment: " + addon.id);
+      gActiveUninstallAddonIDs.add(addon.id);
+      yield uninstallAddons([addon]);
+      gActiveUninstallAddonIDs.delete(addon.id);
+      return this.ADDON_CHANGE_UNINSTALL;
+    }
+
+    // If we get here, we're supposed to be active.
+
+    let changes = 0;
 
-  /**
-   * 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);
+    // That requires an add-on.
+    let currentAddon = yield this._getAddon();
+
+    // If we have an add-on but it isn't up to date, uninstall it
+    // (to prepare for reinstall).
+    if (currentAddon && this._needsUpdate) {
+      this._log.info("reconcileAddonState() - Uninstalling add-on because update " +
+                     "needed: " + currentAddon.id);
+      gActiveUninstallAddonIDs.add(currentAddon.id);
+      yield uninstallAddons([currentAddon]);
+      gActiveUninstallAddonIDs.delete(currentAddon.id);
+      changes |= this.ADDON_CHANGE_UNINSTALL;
+    }
+
+    if (!currentAddon || this._needsUpdate) {
+      this._log.info("reconcileAddonState() - Installing add-on.");
+      yield this._installAddon();
+      changes |= this.ADDON_CHANGE_INSTALL;
+    }
 
     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);
+      throw new Error("Could not obtain add-on for experiment that should be " +
+                      "enabled.");
     }
 
-    // User disabled likely means the experiment is disabled at startup,
-    // since the permissions don't allow it to be disabled by the user.
+    // If we have the add-on and it is enabled, we are done.
     if (!addon.userDisabled) {
-      return;
+      return changes;
     }
 
     let deferred = Promise.defer();
 
+    // Else we need to enable it.
     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;
-  }),
+    changes |= this.ADDON_CHANGE_ENABLE;
+
+    this._log.info("Add-on has been enabled: " + addon.id);
+    return changes;
+   }),
 
   /**
    * Obtain the underlying Addon from the Addon Manager.
    *
    * @return Promise<Addon|null>
    */
   _getAddon: function () {
+    if (!this._addonId) {
+      return Promise.resolve(null);
+    }
+
     let deferred = Promise.defer();
 
     AddonManager.getAddonByID(this._addonId, deferred.resolve);
 
     return deferred.promise;
   },
 
   _logTermination: function (terminationKind, terminationReason) {
@@ -1705,53 +1743,28 @@ Experiments.ExperimentEntry.prototype = 
     let data = [terminationKind, this.id];
     if (terminationReason) {
       data = data.concat(terminationReason);
     }
 
     TelemetryLog.log(TELEMETRY_LOG.TERMINATION_KEY, data);
   },
 
-  /*
-   * Stop if experiment stop criteria are met.
-   * @return Promise<boolean> Resolved when done stopping or checking,
-   *                          the value indicates whether it was stopped.
+  /**
+   * Determine whether an active experiment should be stopped.
    */
-  maybeStop: function () {
-    this._log.trace("maybeStop()");
-    return Task.spawn(function* ExperimentEntry_maybeStop_task() {
-      if (!gExperimentsEnabled) {
-        this._log.warn("maybeStop() - should not get here");
-        yield this.stop(TELEMETRY_LOG.TERMINATION.FROM_API);
-        return true;
-      }
+  shouldStop: function () {
+    if (!this._enabled) {
+      throw new Error("shouldStop must not be called on disabled experiments.");
+    }
 
-      let result = yield this._shouldStop();
-      if (result.shouldStop) {
-        let expireReasons = ["endTime", "maxActiveSeconds"];
-        if (expireReasons.indexOf(result.reason[0]) != -1) {
-          yield this.stop(TELEMETRY_LOG.TERMINATION.EXPIRED);
-        } else {
-          yield this.stop(TELEMETRY_LOG.TERMINATION.RECHECK, result.reason);
-        }
-      }
-
-      return result.shouldStop;
-    }.bind(this));
-  },
-
-  _shouldStop: function () {
     let data = this._manifestData;
     let now = this._policy.now() / 1000; // The manifest times are in seconds.
     let maxActiveSec = data.maxActiveSeconds || 0;
 
-    if (!this._enabled) {
-      return Promise.resolve({shouldStop: false});
-    }
-
     let deferred = Promise.defer();
     this.isApplicable().then(
       () => deferred.resolve({shouldStop: false}),
       reason => deferred.resolve({shouldStop: true, reason: reason})
     );
 
     return deferred.promise;
   },
--- a/browser/experiments/moz.build
+++ b/browser/experiments/moz.build
@@ -4,15 +4,15 @@
 
 EXTRA_COMPONENTS += [
     'Experiments.manifest',
     'ExperimentsService.js',
 ]
 
 JS_MODULES_PATH = 'modules/experiments'
 
-EXTRA_PP_JS_MODULES += [
+EXTRA_JS_MODULES += [
   'Experiments.jsm',
 ]
 
 XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini']
 
 SPHINX_TREES['experiments'] = 'docs'
--- a/browser/experiments/test/xpcshell/test_activate.js
+++ b/browser/experiments/test/xpcshell/test_activate.js
@@ -97,47 +97,48 @@ add_task(function* test_startStop() {
   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();
+  let changes = yield experiment.start();
+  Assert.equal(changes, experiment.ADDON_CHANGE_INSTALL, "Add-on was installed.");
   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();
+  changes = yield experiment.stop();
+  Assert.equal(changes, experiment.ADDON_CHANGE_UNINSTALL, "Add-on was uninstalled.");
   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();
+  changes = yield experiment.start();
+  Assert.equal(changes, experiment.ADDON_CHANGE_INSTALL, "Add-on was installed.");
   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();
+  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();
+  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.");
+  changes = yield experiment.stop();
+  Assert.equal(changes, experiment.ADDON_CHANGE_UNINSTALL, "Add-on should be uninstalled.");
   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
@@ -419,17 +419,17 @@ add_task(function* test_disableExperimen
     Assert.equal(experimentInfo[k], list[0][k],
                  "Property " + k + " should match reference data.");
   }
 
   // Test disabling the experiment.
 
   now = futureDate(now, 1 * MS_IN_ONE_DAY);
   defineNow(gPolicy, now);
-  yield experiments.disableExperiment();
+  yield experiments.disableExperiment("foo");
 
   list = yield experiments.getExperiments();
   Assert.equal(list.length, 1, "Experiment list should have 1 entry.");
 
   experimentInfo.active = false;
   experimentInfo.endDate = now.getTime();
   for (let k of Object.keys(experimentInfo)) {
     Assert.equal(experimentInfo[k], list[0][k],
@@ -744,17 +744,17 @@ add_task(function* test_userDisabledAndU
   let todayActive = yield experiments.lastActiveToday();
   Assert.ok(todayActive, "Last active for today reports a value.");
   Assert.equal(todayActive.id, list[0].id, "The entry is what we expect.");
 
   // Explicitly disable an experiment.
 
   now = futureDate(now, 20 * MS_IN_ONE_DAY);
   defineNow(gPolicy, now);
-  yield experiments.disableExperiment();
+  yield experiments.disableExperiment("foo");
   Assert.equal(observerFireCount, ++expectedObserverFireCount,
                "Experiments observer should have been called.");
 
   list = yield experiments.getExperiments();
   Assert.equal(list.length, 1, "Experiment list should have 1 entry now.");
   Assert.equal(list[0].id, EXPERIMENT1_ID, "Experiment 1 should be the sole entry.");
   Assert.equal(list[0].active, false, "Experiment should not be active anymore.");
   todayActive = yield experiments.lastActiveToday();
@@ -1400,17 +1400,17 @@ add_task(function* testForeignExperiment
     experiments: [],
   };
 
   yield experiments.init();
 
   let addons = yield getExperimentAddons();
   Assert.equal(addons.length, 0, "Precondition: No experiment add-ons present.");
 
-  let failed;
+  let failed = false;
   try {
     yield AddonTestUtils.installXPIFromURL(gDataRoot + EXPERIMENT1_XPI_NAME, EXPERIMENT1_XPI_SHA1);
   } catch (ex) {
     failed = true;
   }
   Assert.ok(failed, "Add-on install should not have completed successfully");
   addons = yield getExperimentAddons();
   Assert.equal(addons.length, 0, "Add-on install should have been cancelled.");
--- a/browser/experiments/test/xpcshell/test_cache.js
+++ b/browser/experiments/test/xpcshell/test_cache.js
@@ -238,12 +238,12 @@ add_task(function* test_cache() {
 
   experimentListData[0].active = false;
   experimentListData[0].endDate = now.getTime();
   checkExperimentListsEqual(experimentListData, list);
   checkExperimentSerializations(experiments._experiments.values());
 
   // Cleanup.
 
-  yield experiments.disableExperiment();
+  yield experiments._toggleExperimentsEnabled(false);
   yield experiments.uninit();
   yield removeCacheFile();
 });
--- a/browser/experiments/test/xpcshell/test_telemetry.js
+++ b/browser/experiments/test/xpcshell/test_telemetry.js
@@ -1,17 +1,17 @@
 /* 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://gre/modules/TelemetryLog.jsm");
 Cu.import("resource://gre/modules/TelemetryPing.jsm");
-Cu.import("resource:///modules/experiments/Experiments.jsm");
+let bsp = Cu.import("resource:///modules/experiments/Experiments.jsm");
 
 
 const FILE_MANIFEST            = "experiments.manifest";
 const MANIFEST_HANDLER         = "manifests/handler";
 
 const SEC_IN_ONE_DAY  = 24 * 60 * 60;
 const MS_IN_ONE_DAY   = SEC_IN_ONE_DAY * 1000;
 
@@ -20,35 +20,17 @@ let gProfileDir          = null;
 let gHttpServer          = null;
 let gHttpRoot            = null;
 let gDataRoot            = null;
 let gReporter            = null;
 let gPolicy              = null;
 let gManifestObject      = null;
 let gManifestHandlerURI  = null;
 
-const TLOG = {
-  // log(key, [kind, experimentId, details])
-  ACTIVATION_KEY: "EXPERIMENT_ACTIVATION",
-  ACTIVATION: {
-    ACTIVATED: "ACTIVATED",
-    INSTALL_FAILURE: "INSTALL_FAILURE",
-    REJECTED: "REJECTED",
-  },
-
-  // log(key, [kind, experimentId, optionalDetails...])
-  TERMINATION_KEY: "EXPERIMENT_TERMINATION",
-  TERMINATION: {
-    USERDISABLED: "USERDISABLED",
-    FROM_API: "FROM_API",
-    EXPIRED: "EXPIRED",
-    RECHECK: "RECHECK",
-  },
-};
-
+const TLOG = bsp.TELEMETRY_LOG;
 
 let gGlobalScope = this;
 function loadAddonManager() {
   let ns = {};
   Cu.import("resource://gre/modules/Services.jsm", ns);
   let head = "../../../../toolkit/mozapps/extensions/test/xpcshell/head_addons.js";
   let file = do_get_file(head);
   let uri = ns.Services.io.newFileURI(file);
@@ -258,30 +240,30 @@ add_task(function* test_telemetryBasics(
   Assert.equal(list.length, 2, "Experiment list should have 2 entries.");
 
   expectedLogLength += 1;
   log = TelemetryPing.getPayload().log;
   Assert.equal(log.length, expectedLogLength, "Telemetry log should have " + expectedLogLength + " entries.");
   checkEvent(log[log.length-1], TLOG.ACTIVATION_KEY,
              [TLOG.ACTIVATION.ACTIVATED, EXPERIMENT2_ID]);
 
-  // Fake user-disable of an experiment.
+  // Fake user uninstall of experiment via add-on manager.
 
   now = futureDate(now, MS_IN_ONE_DAY);
   defineNow(gPolicy, now);
 
-  yield experiments.disableExperiment();
+  yield experiments.disableExperiment(TLOG.TERMINATION.ADDON_UNINSTALLED);
   list = yield experiments.getExperiments();
   Assert.equal(list.length, 2, "Experiment list should have 2 entries.");
 
   expectedLogLength += 1;
   log = TelemetryPing.getPayload().log;
   Assert.equal(log.length, expectedLogLength, "Telemetry log should have " + expectedLogLength + " entries.");
   checkEvent(log[log.length-1], TLOG.TERMINATION_KEY,
-             [TLOG.TERMINATION.USERDISABLED, EXPERIMENT2_ID]);
+             [TLOG.TERMINATION.ADDON_UNINSTALLED, EXPERIMENT2_ID]);
 
   // Trigger update with experiment 1a ready to start.
 
   now = futureDate(now, MS_IN_ONE_DAY);
   defineNow(gPolicy, now);
   gManifestObject.experiments[0].id      = EXPERIMENT3_ID;
   gManifestObject.experiments[0].endTime = dateToSeconds(futureDate(now, 50 * MS_IN_ONE_DAY));
 
@@ -290,22 +272,22 @@ add_task(function* test_telemetryBasics(
   Assert.equal(list.length, 3, "Experiment list should have 3 entries.");
 
   expectedLogLength += 1;
   log = TelemetryPing.getPayload().log;
   Assert.equal(log.length, expectedLogLength, "Telemetry log should have " + expectedLogLength + " entries.");
   checkEvent(log[log.length-1], TLOG.ACTIVATION_KEY,
              [TLOG.ACTIVATION.ACTIVATED, EXPERIMENT3_ID]);
 
-  // Trigger non-user-disable of an experiment via the API
+  // Trigger disable of an experiment via the API.
 
   now = futureDate(now, MS_IN_ONE_DAY);
   defineNow(gPolicy, now);
 
-  yield experiments.disableExperiment(false);
+  yield experiments.disableExperiment(TLOG.TERMINATION.FROM_API);
   list = yield experiments.getExperiments();
   Assert.equal(list.length, 3, "Experiment list should have 3 entries.");
 
   expectedLogLength += 1;
   log = TelemetryPing.getPayload().log;
   Assert.equal(log.length, expectedLogLength, "Telemetry log should have " + expectedLogLength + " entries.");
   checkEvent(log[log.length-1], TLOG.TERMINATION_KEY,
              [TLOG.TERMINATION.FROM_API, EXPERIMENT3_ID]);