From 7f61a84118a50ffeb6483de7ba2c964280902801 Mon Sep 17 00:00:00 2001 From: Gregory Szorc Date: Fri, 4 Apr 2014 15:58:26 -0700 Subject: [PATCH] Bug 989137 - Part 8: Prevent unknown experiments from being installed; r=Unfocused The experiments service insists on being in control of experiments. Before, it wasn't being as assertive as it needed to be: other browser components could install experiments behind its back. This patch reasserts the experiments service as king of experiments add-on management. --HG-- extra : rebase_source : 91439622e47419fae7675d83890e83cf37db453b --- browser/experiments/Experiments.jsm | 72 ++++++++++++++++++- .../test/xpcshell/test_activate.js | 6 ++ browser/experiments/test/xpcshell/test_api.js | 55 ++++++++++++-- .../test/browser/browser_experiments.js | 17 ++++- 4 files changed, 141 insertions(+), 9 deletions(-) diff --git a/browser/experiments/Experiments.jsm b/browser/experiments/Experiments.jsm index f2d2220596e..d52359d55b9 100644 --- a/browser/experiments/Experiments.jsm +++ b/browser/experiments/Experiments.jsm @@ -98,6 +98,10 @@ 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(); + let gLogger; let gLogDumping = false; @@ -359,7 +363,7 @@ Experiments.Experiments.prototype = { AsyncShutdown.profileBeforeChange.addBlocker("Experiments.jsm shutdown", this.uninit.bind(this)); - AddonManager.addAddonListener(this); + this._startWatchingAddons(); this._loadTask = Task.spawn(this._loadFromCache.bind(this)); this._loadTask.then( @@ -380,7 +384,7 @@ Experiments.Experiments.prototype = { */ uninit: function () { if (!this._shutdown) { - AddonManager.removeAddonListener(this); + this._stopWatchingAddons(); gPrefs.ignore(PREF_LOGGING, configureLogging); gPrefs.ignore(PREF_MANIFEST_URI, this.updateManifest, this); @@ -400,6 +404,16 @@ Experiments.Experiments.prototype = { return Promise.resolve(); }, + _startWatchingAddons: function () { + AddonManager.addAddonListener(this); + AddonManager.addInstallListener(this); + }, + + _stopWatchingAddons: function () { + AddonManager.removeInstallListener(this); + AddonManager.removeAddonListener(this); + }, + /** * Throws an exception if we've already shut down. */ @@ -644,6 +658,42 @@ Experiments.Experiments.prototype = { this.disableExperiment(); }, + onInstallStarted: function (install) { + if (install.addon.type != "experiment") { + return; + } + + // We want to be in control of all experiment add-ons: reject installs + // for add-ons that we don't know about. + + // We have a race condition of sorts to worry about here. We have 2 + // onInstallStarted listeners. This one (the global one) and the one + // created as part of ExperimentEntry._installAddon. Because of the order + // they are registered in, this one likely executes first. Unfortunately, + // this means that the add-on ID is not yet set on the ExperimentEntry. + // So, we can't just look at this._trackedAddonIds because the new experiment + // will have its add-on ID set to null. We work around this by storing a + // identifying field - the source URL of the install - in a module-level + // variable (so multiple Experiments instances doesn't cancel each other + // out). + + if (this._trackedAddonIds.has(install.addon.id)) { + this._log.info("onInstallStarted allowing install because add-on ID " + + "tracked by us."); + return; + } + + if (gActiveInstallURLs.has(install.sourceURI.spec)) { + this._log.info("onInstallStarted allowing install because install " + + "tracked by us."); + return; + } + + this._log.warn("onInstallStarted cancelling install of unknown " + + "experiment add-on: " + install.addon.id); + return false; + }, + // END OF ADD-ON LISTENERS. _getExperimentByAddonId: function (addonId) { @@ -851,6 +901,17 @@ Experiments.Experiments.prototype = { return this._run(); }, + /** + * The Set of add-on IDs that we know about from manifests. + */ + get _trackedAddonIds() { + if (!this._experiments) { + return new Set(); + } + + return new Set([e._addonId for ([,e] of this._experiments) if (e._addonId)]); + }, + /* * Task function to check applicability of experiments, disable the active * experiment if needed and activate the first applicable candidate. @@ -875,7 +936,7 @@ Experiments.Experiments.prototype = { // should have some record of it. In the end, we decide to discard all // knowledge for these unknown experiment add-ons. let installedExperiments = yield installedExperimentAddons(); - let expectedAddonIds = new Set([e._addonId for ([,e] of this._experiments)]); + let expectedAddonIds = this._trackedAddonIds; let unknownAddons = [a for (a of installedExperiments) if (!expectedAddonIds.has(a.id))]; if (unknownAddons.length) { this._log.warn("_evaluateExperiments() - unknown add-ons in AddonManager: " + @@ -1402,11 +1463,14 @@ Experiments.ExperimentEntry.prototype = { 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=" + (install.state || "?") + ", error=" + install.error; this._log.error("_installAddon() - " + message); this._failedStart = true; + gActiveInstallURLs.delete(install.sourceURI.spec); TelemetryLog.log(TELEMETRY_LOG.ACTIVATION_KEY, [TELEMETRY_LOG.ACTIVATION.INSTALL_FAILURE, this.id]); @@ -1446,6 +1510,8 @@ Experiments.ExperimentEntry.prototype = { 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(); this._enabled = true; diff --git a/browser/experiments/test/xpcshell/test_activate.js b/browser/experiments/test/xpcshell/test_activate.js index 019307b91ee..e30e57ea231 100644 --- a/browser/experiments/test/xpcshell/test_activate.js +++ b/browser/experiments/test/xpcshell/test_activate.js @@ -86,6 +86,12 @@ add_task(function* test_startStop() { }); let experiment = new Experiments.ExperimentEntry(gPolicy); experiment.initFromManifestData(manifestData); + + // We need to associate it with the singleton so the onInstallStarted + // Addon Manager listener will know about it. + Experiments.instance()._experiments = new Map(); + Experiments.instance()._experiments.set(experiment.id, experiment); + let result; defineNow(gPolicy, baseDate); diff --git a/browser/experiments/test/xpcshell/test_api.js b/browser/experiments/test/xpcshell/test_api.js index d92083fad85..3faec84fbfd 100644 --- a/browser/experiments/test/xpcshell/test_api.js +++ b/browser/experiments/test/xpcshell/test_api.js @@ -151,6 +151,8 @@ add_task(function* test_getExperiments() { "Experiments observer should not have been called yet."); let list = yield experiments.getExperiments(); Assert.equal(list.length, 0, "Experiment list should be empty."); + let addons = yield getExperimentAddons(); + Assert.equal(addons.length, 0, "Precondition: No experiment add-ons are installed."); // Trigger update, clock set for experiment 1 to start. @@ -164,6 +166,8 @@ add_task(function* test_getExperiments() { list = yield experiments.getExperiments(); Assert.equal(list.length, 1, "Experiment list should have 1 entry now."); + addons = yield getExperimentAddons(); + Assert.equal(addons.length, 1, "An experiment add-on was installed."); experimentListData[1].active = true; experimentListData[1].endDate = now.getTime() + 10 * MS_IN_ONE_DAY; @@ -187,6 +191,8 @@ add_task(function* test_getExperiments() { list = yield experiments.getExperiments(); Assert.equal(list.length, 1, "Experiment list should have 1 entry."); + addons = yield getExperimentAddons(); + Assert.equal(addons.length, 0, "The experiment add-on should be uninstalled."); experimentListData[1].active = false; experimentListData[1].endDate = now.getTime(); @@ -211,6 +217,8 @@ add_task(function* test_getExperiments() { list = yield experiments.getExperiments(); Assert.equal(list.length, 2, "Experiment list should have 2 entries now."); + addons = yield getExperimentAddons(); + Assert.equal(addons.length, 1, "An experiment add-on is installed."); experimentListData[0].active = true; experimentListData[0].endDate = now.getTime() + 10 * MS_IN_ONE_DAY; @@ -236,6 +244,8 @@ add_task(function* test_getExperiments() { list = yield experiments.getExperiments(); Assert.equal(list.length, 2, "Experiment list should have 2 entries now."); + addons = yield getExperimentAddons(); + Assert.equal(addons.length, 0, "No experiments add-ons are installed."); experimentListData[0].active = false; experimentListData[0].endDate = now.getTime(); @@ -301,11 +311,6 @@ add_task(function* test_addonAlreadyInstalled() { let list = yield experiments.getExperiments(); Assert.equal(list.length, 0, "Experiment list should be empty."); - // Install conflicting addon. - - let installed = yield installAddon(gDataRoot + EXPERIMENT1_XPI_NAME, EXPERIMENT1_XPI_SHA1); - Assert.ok(installed, "Addon should have been installed."); - // Trigger update, clock set for the experiment to start. now = futureDate(startDate, 10 * MS_IN_ONE_DAY); @@ -320,6 +325,20 @@ add_task(function* test_addonAlreadyInstalled() { 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."); + let addons = yield getExperimentAddons(); + Assert.equal(addons.length, 1, "1 add-on is installed."); + + // Install conflicting addon. + + let installed = yield installAddon(gDataRoot + EXPERIMENT1_XPI_NAME, EXPERIMENT1_XPI_SHA1); + Assert.ok(installed, "Addon should have been installed."); + addons = yield getExperimentAddons(); + Assert.equal(addons.length, 1, "1 add-on is installed."); + list = yield experiments.getExperiments(); + Assert.equal(list.length, 1, "Experiment list should still have 1 entry."); + 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."); + // Cleanup. Services.obs.removeObserver(observer, OBSERVER_TOPIC); @@ -1351,7 +1370,12 @@ add_task(function* testUnknownExperimentsUninstalled() { let addons = yield getExperimentAddons(); Assert.equal(addons.length, 0, "Precondition: No experiment add-ons are present."); + + // Simulate us not listening. + experiments._stopWatchingAddons(); yield installAddon(gDataRoot + EXPERIMENT1_XPI_NAME, EXPERIMENT1_XPI_SHA1); + experiments._startWatchingAddons(); + addons = yield getExperimentAddons(); Assert.equal(addons.length, 1, "Experiment 1 installed via AddonManager"); @@ -1372,3 +1396,24 @@ add_task(function* testUnknownExperimentsUninstalled() { yield experiments.uninit(); yield removeCacheFile(); }); + +// If someone else installs an experiment add-on, we detect and stop that. +add_task(function* testForeignExperimentInstall() { + let experiments = new Experiments.Experiments(gPolicy); + + gManifestObject = { + "version": 1, + experiments: [], + }; + + yield experiments.init(); + + let addons = yield getExperimentAddons(); + 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(); +}); diff --git a/toolkit/mozapps/extensions/test/browser/browser_experiments.js b/toolkit/mozapps/extensions/test/browser/browser_experiments.js index 2f760acc3f6..adeb35669c0 100644 --- a/toolkit/mozapps/extensions/test/browser/browser_experiments.js +++ b/toolkit/mozapps/extensions/test/browser/browser_experiments.js @@ -5,6 +5,7 @@ let gManagerWindow; let gCategoryUtilities; let gInstalledAddons = []; +let gContext = this; function test() { waitForExplicitFinish(); @@ -13,6 +14,14 @@ function test() { gManagerWindow = win; gCategoryUtilities = new CategoryUtilities(win); + // The Experiments Manager will interfere with us by preventing installs + // of experiments it doesn't know about. We remove it from the equation + // because here we are only concerned with core Addon Manager operation, + // not the super set Experiments Manager has imposed. + if ("@mozilla.org/browser/experiments-service;1" in Components.classes) { + Components.utils.import("resource:///modules/experiments/Experiments.jsm", gContext); + gContext.Experiments.instance()._stopWatchingAddons(); + } run_next_test(); }); } @@ -22,7 +31,13 @@ function end_test() { addon.uninstall(); } - close_manager(gManagerWindow, finish); + close_manager(gManagerWindow, () => { + if ("@mozilla.org/browser/experiments-service;1" in Components.classes) { + gContext.Experiments.instance()._startWatchingAddons(); + } + + finish(); + }); } // On an empty profile with no experiments, the experiment category