From 113132383362e9a99942ef91dd287b23ba08cb42 Mon Sep 17 00:00:00 2001 From: Benjamin Smedberg Date: Tue, 18 Mar 2014 22:52:30 +0100 Subject: [PATCH] Bug 984879 - Experiment manager shutdown, r=felipe --- browser/experiments/Experiments.jsm | 62 +++++++++++++++++++---- browser/experiments/ExperimentsService.js | 32 +++--------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/browser/experiments/Experiments.jsm b/browser/experiments/Experiments.jsm index fe40d474e17..90bd9e7a0ad 100644 --- a/browser/experiments/Experiments.jsm +++ b/browser/experiments/Experiments.jsm @@ -18,6 +18,7 @@ Cu.import("resource://gre/modules/osfile.jsm"); Cu.import("resource://gre/modules/Log.jsm"); Cu.import("resource://gre/modules/Preferences.jsm"); Cu.import("resource://services-common/utils.js"); +Cu.import("resource://gre/modules/AsyncShutdown.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "UpdateChannel", "resource://gre/modules/UpdateChannel.jsm"); @@ -221,6 +222,8 @@ Experiments.Experiments = function (policy=new Experiments.Policy()) { // Timer for re-evaluating experiment status. this._timer = null; + this._shutdown = false; + this.init(); }; @@ -240,6 +243,9 @@ Experiments.Experiments.prototype = { gPrefsTelemetry.observe(PREF_TELEMETRY_ENABLED, this._telemetryStatusChanged, this); gPrefsTelemetry.observe(PREF_TELEMETRY_PRERELEASE, this._telemetryStatusChanged, this); + AsyncShutdown.profileBeforeChange.addBlocker("Experiments.jsm shutdown", + this.uninit.bind(this)); + AddonManager.addAddonListener(this); this._experiments = new Map(); @@ -251,21 +257,35 @@ Experiments.Experiments.prototype = { * The promise is fulfilled when all pending tasks are finished. */ uninit: function () { - AddonManager.removeAddonListener(this); + if (!this._shutdown) { + AddonManager.removeAddonListener(this); - gPrefs.ignore(PREF_LOGGING, configureLogging); - gPrefs.ignore(PREF_MANIFEST_URI, this.updateManifest, this); - gPrefs.ignore(PREF_ENABLED, this._toggleExperimentsEnabled, this); + gPrefs.ignore(PREF_LOGGING, configureLogging); + gPrefs.ignore(PREF_MANIFEST_URI, this.updateManifest, this); + gPrefs.ignore(PREF_ENABLED, this._toggleExperimentsEnabled, this); - gPrefsTelemetry.ignore(PREF_TELEMETRY_ENABLED, this._telemetryStatusChanged, this); - gPrefsTelemetry.ignore(PREF_TELEMETRY_PRERELEASE, this._telemetryStatusChanged, this); + gPrefsTelemetry.ignore(PREF_TELEMETRY_ENABLED, this._telemetryStatusChanged, this); + gPrefsTelemetry.ignore(PREF_TELEMETRY_PRERELEASE, this._telemetryStatusChanged, this); - if (this._timer) { - this._timer.clear(); + if (this._timer) { + this._timer.clear(); + } } - let tasks = this._pendingTasks; - return this._pendingTasksDone(); + this._shutdown = true; + if (this._pendingTasks.saveToCache) { + return this._pendingTasks.saveToCache; + } + return Promise.resolve(); + }, + + /** + * Throws an exception if we've already shut down. + */ + _checkForShutdown: function() { + if (this._shutdown) { + throw Error("uninit() already called"); + } }, /** @@ -369,6 +389,10 @@ Experiments.Experiments.prototype = { return Promise.reject(new Error("experiments are disabled")); } + if (this._shutdown) { + return Promise.reject(Error("uninit() alrady called")); + } + if (this._pendingTasks.updateManifest) { return this._pendingTasks.updateManifest; } @@ -376,6 +400,8 @@ Experiments.Experiments.prototype = { let uri = Services.urlFormatter.formatURLPref(PREF_BRANCH + PREF_MANIFEST_URI); this._pendingTasks.updateManifest = Task.spawn(function () { + this._checkForShutdown(); + // Don't interfere while we're saving or loading the cache. try { yield this._pendingTasksDone([this._pendingTasks.updateManifest]); @@ -387,6 +413,8 @@ Experiments.Experiments.prototype = { let responseText = yield this._httpGetRequest(uri); gLogger.trace("Experiments::updateManifest::updateTask() - responseText=\"" + responseText + "\""); + this._checkForShutdown(); + let data = JSON.parse(responseText); this._updateExperiments(data); yield this._evaluateExperiments(); @@ -407,6 +435,8 @@ Experiments.Experiments.prototype = { notify: function (timer) { gLogger.trace("Experiments::notify()"); + this._checkForShutdown(); + if (this._pendingTasks.evaluateExperiments) { return; } @@ -421,6 +451,7 @@ Experiments.Experiments.prototype = { }, onDisabled: function (addon) { + this._checkForShutdown(); let experiment = this._experiments.get(addon.id); if (!experiment) { return; @@ -430,6 +461,7 @@ Experiments.Experiments.prototype = { }, onUninstalled: function (addon) { + this._checkForShutdown(); let experiment = this._experiments.get(addon.id); if (!experiment) { return; @@ -672,6 +704,10 @@ Experiments.Experiments.prototype = { _disableExperiments: function () { gLogger.trace("Experiments::disableExperiments()"); + if (this._shutdown) { + return Promise.reject(Error("uninit() alrady called")); + } + let active = this._getActiveExperiment(); let promise = Promise.resolve(); if (active) { @@ -694,6 +730,8 @@ Experiments.Experiments.prototype = { disableExperiment: function (experimentId, userDisabled=true) { gLogger.trace("Experiments::disableExperiment() - " + experimentId); + this._checkForShutdown(); + let experiment = this._experiments.get(experimentId); if (!experiment) { let message = "no experiment with this id"; @@ -730,7 +768,10 @@ Experiments.Experiments.prototype = { _evaluateExperiments: function () { gLogger.trace("Experiments::evaluateExperiments()"); + this._checkForShutdown(); + return Task.spawn(function Experiments_evaluateExperiments_evaluateTask() { + this._checkForShutdown(); let activeExperiment = this._getActiveExperiment(); let activeChanged = false; @@ -792,6 +833,7 @@ Experiments.Experiments.prototype = { * Schedule the soonest re-check of experiment applicability that is needed. */ _scheduleExperimentEvaluation: function () { + this._checkForShutdown(); if (!gExperimentsEnabled || this._experiments.length == 0) { return; } diff --git a/browser/experiments/ExperimentsService.js b/browser/experiments/ExperimentsService.js index 29dec7c1c3d..516863d2c71 100644 --- a/browser/experiments/ExperimentsService.js +++ b/browser/experiments/ExperimentsService.js @@ -9,45 +9,29 @@ const {interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource:///modules/experiments/Experiments.jsm"); +Cu.import("resource://gre/modules/osfile.jsm") function ExperimentsService() { } ExperimentsService.prototype = { - _experiments: null, - _pendingManifestUpdate: false, - classID: Components.ID("{f7800463-3b97-47f9-9341-b7617e6d8d49}"), QueryInterface: XPCOMUtils.generateQI([Ci.nsITimerCallback, Ci.nsIObserver]), notify: function (timer) { - if (this._experiments) { - this._experiments.updateManifest(); - } else { - this._pendingManifestUpdate = true; + if (OS.Constants.Path.profileDir === undefined) { + throw Error("Update timer fired before profile was initialized?"); } + Experiments.instance().updateManifest(); }, observe: function (subject, topic, data) { - switch(topic) { - case "profile-after-change": - Services.obs.addObserver(this, "xpcom-shutdown", false); - this._experiments = Experiments.instance(); - if (this._pendingManifestUpdate) { - this._experiments.updateManifest(); - this._pendingManifestUpdate = false; - } - break; - case "xpcom-shutdown": - Services.obs.removeObserver(this, "xpcom-shutdown"); - this._experiments.uninit(); - break; + switch (topic) { + case "profile-after-change": + Experiments.instance(); // for side effects + break; } }, - - get experiments() { - return this._experiments; - }, }; this.NSGetFactory = XPCOMUtils.generateNSGetFactory([ExperimentsService]);