Bug 985670 - Telemetry experiments: Decouple experiment and addon ids. r=felipe

browser/experiments/Experiments.jsm

@@ -505,23 +505,34 @@ Experiments.Experiments.prototype = {
   },
 
   onDisabled: function (addon) {
-    this._checkForShutdown();
-    let experiment = this._experiments.get(addon.id);
-    if (!experiment) {
-      return;
-    }
-
+    gLogger.trace("Experiments::onDisabled() - addon id: " + addon.id)
     this.disableExperiment(addon.id);
   },
 
   onUninstalled: function (addon) {
+    gLogger.trace("Experiments::onUninstalled() - addon id: " + addon.id);
+    this.disableExperiment(addon.id);
+  },
+
+  _getExperimentByAddonId: function (addonId) {
+    for (let [, entry] of this._experiments) {
+      if (entry._addonId === addonId) {
+        return entry;
+      }
+    }
+
+    return null;
+  },
+
+  _disableExperimentByAddonId: function (addonId) {
     this._checkForShutdown();
-    let experiment = this._experiments.get(addon.id);
+    gLogger.trace("Experiments::disableExperimentByAddonId() - addon id: " + addonId);
+    let experiment = this._getExperimentByAddonId(addonId);
     if (!experiment) {
       return;
     }
 
-    this.disableExperiment(addon.id);
+    this.disableExperiment(experiment.id);
   },
 
   /*
@@ -950,6 +961,7 @@ Experiments.ExperimentEntry = function (policy) {
   this._name = null;
   this._description = null;
   this._homepageURL = null;
+  this._addonId = null;
 };
 
 Experiments.ExperimentEntry.prototype = {
@@ -1293,13 +1305,6 @@ Experiments.ExperimentEntry.prototype = {
       let listener = {
         onDownloadEnded: install => {
           gLogger.trace("ExperimentEntry::start() - onDownloadEnded for " + this.id);
-          let addon = install.addon;
-
-          if (addon.id !== this.id) {
-            let message = "id mismatch: '" + this.id + "' vs. '" + addon.id + "'";
-            gLogger.error("ExperimentEntry::start() - " + message);
-            install.cancel();
-          }
         },
 
         onInstallStarted: install => {
@@ -1312,6 +1317,7 @@ Experiments.ExperimentEntry.prototype = {
 
           let addon = install.addon;
           this._name = addon.name;
+          this._addonId = addon.id;
           this._description = addon.description || "";
           this._homepageURL = addon.homepageURL || "";
         },
@@ -1329,7 +1335,9 @@ Experiments.ExperimentEntry.prototype = {
       };
 
       ["onDownloadCancelled", "onDownloadFailed", "onInstallCancelled", "onInstallFailed"]
-        .forEach(what => listener[what] = install => failureHandler(install, what));
+        .forEach(what => {
+          listener[what] = install => failureHandler(install, what)
+        });
 
       install.addListener(listener);
       install.install();
@@ -1362,7 +1370,7 @@ Experiments.ExperimentEntry.prototype = {
       this._endDate = now;
     };
 
-    AddonManager.getAddonByID(this.id, addon => {
+    AddonManager.getAddonByID(this._addonId, addon => {
       if (!addon) {
         let message = "could not get Addon for " + this.id;
         gLogger.warn("ExperimentEntry::stop() - " + message);
@@ -1373,7 +1381,7 @@ Experiments.ExperimentEntry.prototype = {
 
       let listener = {};
       let handler = addon => {
-        if (addon.id !== this.id) {
+        if (addon.id !== this._addonId) {
           return;
         }
 

browser/experiments/test/xpcshell/head.js

@@ -100,6 +100,61 @@ function dateToSeconds(date) {
   return date.getTime() / 1000;
 }
 
+// Install addon and return a Promise<boolean> that is
+// resolve with true on success, false otherwise.
+function installAddon(url, hash) {
+  let deferred = Promise.defer();
+  let success = () => deferred.resolve(true);
+  let fail = () => deferred.resolve(false);
+  let listener = {
+    onDownloadCancelled: fail,
+    onDownloadFailed: fail,
+    onInstallCancelled: fail,
+    onInstallFailed: fail,
+    onInstallEnded: success,
+  };
+
+  let installCallback = install => {
+    install.addListener(listener);
+    install.install();
+  };
+
+  AddonManager.getInstallForURL(url, installCallback,
+                     "application/x-xpinstall", hash);
+
+  return deferred.promise;
+}
+
+// Uninstall addon and return a Promise<boolean> that is
+// resolve with true on success, false otherwise.
+function uninstallAddon(id) {
+  let deferred = Promise.defer();
+
+  AddonManager.getAddonByID(id, addon => {
+    if (!addon) {
+      deferred.resolve(false);
+    }
+
+    let listener = {};
+    let handler = addon => {
+      if (addon.id !== id) {
+        return;
+      }
+
+      AddonManager.removeAddonListener(listener);
+      deferred.resolve(true);
+    };
+
+    listener.onUninstalled = handler;
+    listener.onDisabled = handler;
+
+    AddonManager.addAddonListener(listener);
+    addon.uninstall();
+  });
+
+  return deferred.promise;
+}
+
 function createAppInfo(options) {
   const XULAPPINFO_CONTRACTID = "@mozilla.org/xre/app-info;1";
   const XULAPPINFO_CID = Components.ID("{c763b610-9d49-455a-bbd2-ede71682a1ac}");

browser/experiments/test/xpcshell/test_api.js

@@ -1200,6 +1200,84 @@ add_task(function* test_invalidUrl() {
   yield removeCacheFile();
 });
 
+// Test that we handle it properly when active experiment addons are being
+// uninstalled.
+
+add_task(function* test_unexpectedUninstall() {
+  const OBSERVER_TOPIC = "experiments-changed";
+  let observerFireCount = 0;
+  let expectedObserverFireCount = 0;
+  let observer = () => ++observerFireCount;
+  Services.obs.addObserver(observer, OBSERVER_TOPIC, false);
+
+  // Dates the following tests are based on.
+
+  let baseDate   = new Date(2014, 5, 1, 12);
+  let startDate  = futureDate(baseDate,   100 * MS_IN_ONE_DAY);
+  let endDate    = futureDate(baseDate, 10000 * MS_IN_ONE_DAY);
+
+  // The manifest data we test with.
+
+  gManifestObject = {
+    "version": 1,
+    experiments: [
+      {
+        id:               EXPERIMENT1_ID,
+        xpiURL:           gDataRoot + EXPERIMENT1_XPI_NAME,
+        xpiHash:          EXPERIMENT1_XPI_SHA1,
+        startTime:        dateToSeconds(startDate),
+        endTime:          dateToSeconds(endDate),
+        maxActiveSeconds: 10 * SEC_IN_ONE_DAY,
+        appName:          ["XPCShell"],
+        channel:          ["nightly"],
+      },
+    ],
+  };
+
+  let experiments = new Experiments.Experiments(gPolicy);
+
+  // Trigger update, clock set to before any activation.
+
+  let now = baseDate;
+  defineNow(gPolicy, now);
+  yield experiments.updateManifest();
+  Assert.equal(observerFireCount, 0,
+               "Experiments observer should not have been called yet.");
+  let list = yield experiments.getExperiments();
+  Assert.equal(list.length, 0, "Experiment list should be empty.");
+
+  // Trigger update, clock set for the experiment to start.
+
+  now = futureDate(startDate, 10 * MS_IN_ONE_DAY);
+  defineNow(gPolicy, now);
+  yield experiments.updateManifest();
+  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, true, "Experiment 1 should be active.");
+
+  // Uninstall the addon through the addon manager instead of stopping it through
+  // the experiments API.
+
+  let success = yield uninstallAddon(EXPERIMENT1_ID);
+  Assert.ok(success, "Addon should have been uninstalled.");
+
+  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 1 should not be active anymore.");
+
+  // Cleanup.
+
+  Services.obs.removeObserver(observer, OBSERVER_TOPIC);
+  yield experiments.uninit();
+  yield removeCacheFile();
+});
+
+
 add_task(function* shutdown() {
   yield gReporter._shutdown();
   yield removeCacheFile();