Bug 1555632: Add separate AddonManager.beforeShutdown barrier and use where appropriate. r=aswan

The AddonManager.shutdown barrier blocks the final phase of AddonManager
shutdown, which means that things which use it to mean "I need to do this
before the add-on manager shuts down" are generally broken.

This patch adds a separate beforeShutdown barrier which blocks the start of
provider shutdown, and changes existing broken consumers to use it. It also
renames the existing barrier to `AddonManagerPrivate.finalShutdown` to make it
clearer a) what it does, and b) that only internal AddonManager code should
use it.

Differential Revision: https://phabricator.services.mozilla.com/D33195

--HG--
extra : rebase_source : de20ecdc145a3b0c2565da910be6c99698b0b7df
extra : source : 678144498f35fd4ffafe8f0ddf5f64143d380dcb

toolkit/components/telemetry/app/TelemetryEnvironment.jsm

@@ -555,15 +555,9 @@ EnvironmentAddonBuilder.prototype = {
    * Get the initial set of addons.
    * @returns Promise<void> when the initial load is complete.
    */
-  init() {
-    // Some tests don't initialize the addon manager. This accounts for the
-    // unfortunate reality of life.
-    try {
-      AddonManager.shutdown.addBlocker("EnvironmentAddonBuilder",
-        () => this._shutdownBlocker());
-    } catch (err) {
-      return Promise.reject(err);
-    }
+  async init() {
+    AddonManager.beforeShutdown.addBlocker("EnvironmentAddonBuilder",
+      () => this._shutdownBlocker());
 
     this._pendingTask = (async () => {
       try {

toolkit/mozapps/extensions/AddonManager.jsm

@@ -516,7 +516,8 @@ var gCheckUpdateSecurity = gCheckUpdateSecurityDefault;
 var gUpdateEnabled = true;
 var gAutoUpdateDefault = true;
 var gWebExtensionsMinPlatformVersion = "";
-var gShutdownBarrier = null;
+var gFinalShutdownBarrier = null;
+var gBeforeShutdownBarrier = null;
 var gRepoShutdownState = "";
 var gShutdownInProgress = false;
 var gPluginPageListener = null;
@@ -589,7 +590,7 @@ var AddonManagerInternal = {
       };
       logger.debug("Registering shutdown blocker for " + name);
       this.providerShutdowns.set(aProvider, AMProviderShutdown);
-      AddonManager.shutdown.addBlocker(name, AMProviderShutdown);
+      AddonManagerPrivate.finalShutdown.addBlocker(name, AMProviderShutdown);
     }
 
     this.pendingProviders.delete(aProvider);
@@ -711,7 +712,8 @@ var AddonManagerInternal = {
       }
 
       // Register our shutdown handler with the AsyncShutdown manager
-      gShutdownBarrier = new AsyncShutdown.Barrier("AddonManager: Waiting for providers to shut down.");
+      gBeforeShutdownBarrier = new AsyncShutdown.Barrier("AddonManager: Waiting to start provider shutdown.");
+      gFinalShutdownBarrier = new AsyncShutdown.Barrier("AddonManager: Waiting for providers to shut down.");
       AsyncShutdown.profileBeforeChange.addBlocker("AddonManager: shutting down.",
                                                    this.shutdownManager.bind(this),
                                                    {fetchState: this.shutdownState.bind(this)});
@@ -827,13 +829,14 @@ var AddonManagerInternal = {
 
     // If we're unregistering after startup but before shutting down,
     // remove the blocker for this provider's shutdown and call it.
-    // If we're already shutting down, just let gShutdownBarrier call it to avoid races.
+    // If we're already shutting down, just let gFinalShutdownBarrier
+    // call it to avoid races.
     if (gStarted && !gShutdownInProgress) {
       logger.debug("Unregistering shutdown blocker for " + providerName(aProvider));
       let shutter = this.providerShutdowns.get(aProvider);
       if (shutter) {
         this.providerShutdowns.delete(aProvider);
-        gShutdownBarrier.client.removeBlocker(shutter);
+        gFinalShutdownBarrier.client.removeBlocker(shutter);
         return shutter();
       }
     }
@@ -905,11 +908,10 @@ var AddonManagerInternal = {
    */
   shutdownState() {
     let state = [];
-    if (gShutdownBarrier) {
-      state.push({
-        name: gShutdownBarrier.client.name,
-        state: gShutdownBarrier.state,
-      });
+    for (let barrier of [gBeforeShutdownBarrier, gFinalShutdownBarrier]) {
+      if (barrier) {
+        state.push({name: barrier.client.name, state: barrier.state});
+      }
     }
     state.push({
       name: "AddonRepository: async shutdown",
@@ -925,6 +927,13 @@ var AddonManagerInternal = {
    *                       have finished shutting down
    */
   async shutdownManager() {
+    logger.debug("before shutdown");
+    try {
+      await gBeforeShutdownBarrier.wait();
+    } catch (e) {
+      Cu.reportError(e);
+    }
+
     logger.debug("shutdown");
     this.callManagerListeners("onShutdown");
 
@@ -943,7 +952,7 @@ var AddonManagerInternal = {
     // Only shut down providers if they've been started.
     if (gStarted) {
       try {
-        await gShutdownBarrier.wait();
+        await gFinalShutdownBarrier.wait();
       } catch (err) {
         savedError = err;
         logger.error("Failure during wait for shutdown barrier", err);
@@ -972,7 +981,8 @@ var AddonManagerInternal = {
       delete this.startupChanges[type];
     gStarted = false;
     gStartupComplete = false;
-    gShutdownBarrier = null;
+    gFinalShutdownBarrier = null;
+    gBeforeShutdownBarrier = null;
     gShutdownInProgress = false;
     if (savedError) {
       throw savedError;
@@ -2988,6 +2998,16 @@ var AddonManagerPrivate = {
     let provider = AddonManagerInternal._getProviderByName("XPIProvider");
     return provider ? provider.databaseReady : new Promise(() => {});
   },
+
+  /**
+   * Async shutdown barrier which blocks the completion of add-on
+   * manager shutdown. This should generally only be used by add-on
+   * providers (i.e., XPIProvider) to complete their final shutdown
+   * tasks.
+   */
+  get finalShutdown() {
+    return gFinalShutdownBarrier.client;
+  },
 };
 
 /**
@@ -3492,8 +3512,13 @@ var AddonManager = {
     return AddonManagerInternal.webAPI;
   },
 
-  get shutdown() {
-    return gShutdownBarrier.client;
+  /**
+   * Async shutdown barrier which blocks the start of AddonManager
+   * shutdown. Callers should add blockers to this barrier if they need
+   * to complete add-on manager operations before it shuts down.
+   */
+  get beforeShutdown() {
+    return gBeforeShutdownBarrier.client;
   },
 };
 

toolkit/mozapps/extensions/internal/XPIDatabase.jsm

@@ -2873,7 +2873,7 @@ this.XPIDatabaseReconcile = {
       // Do some blocklist checks. These will happen after we've just saved everything,
       // because they're async and depend on the blocklist loading. When we're done, save
       // the data if any of the add-ons' blocklist state has changed.
-      AddonManager.shutdown.addBlocker(
+      AddonManager.beforeShutdown.addBlocker(
         "Update add-on blocklist state into add-on DB",
         (async () => {
           // Avoid querying the AddonManager immediately to give startup a chance

toolkit/mozapps/extensions/internal/XPIProvider.jsm

@@ -1553,7 +1553,7 @@ var XPIStates = {
     if (!this._jsonFile) {
       this._jsonFile = new JSONFile({
         path: OS.Path.join(OS.Constants.Path.profileDir, FILE_XPI_STATES),
-        finalizeAt: AddonManager.shutdown,
+        finalizeAt: AddonManagerPrivate.finalShutdown,
         compression: "lz4",
       });
       this._jsonFile.data = this;
@@ -2340,6 +2340,9 @@ var XPIProvider = {
         "XPIProvider shutdown",
         async () => {
           XPIProvider._closing = true;
+
+          XPIDatabase.asyncLoadDB();
+
           await XPIProvider.cleanupTemporaryAddons();
           for (let addon of XPIProvider.sortBootstrappedAddons().reverse()) {
             // If no scope has been loaded for this add-on then there is no need

toolkit/mozapps/extensions/test/xpcshell/head_addons.js

@@ -57,6 +57,8 @@ ChromeUtils.defineModuleGetter(this, "RemoteSettings",
                                "resource://services-settings/remote-settings.js");
 ChromeUtils.defineModuleGetter(this, "TestUtils",
                                "resource://testing-common/TestUtils.jsm");
+ChromeUtils.defineModuleGetter(this, "setTimeout",
+                               "resource://gre/modules/Timer.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(this, "aomStartup",
                                    "@mozilla.org/addons/addon-manager-startup;1",
@@ -302,6 +304,12 @@ async function restartWithLocales(locales) {
   await promiseRestartManager();
 }
 
+function delay(msec) {
+  return new Promise(resolve => {
+    setTimeout(resolve, msec);
+  });
+}
+
 /**
  * Returns a map of Addon objects for installed add-ons with the given
  * IDs. The returned map contains a key for the ID of each add-on that

toolkit/mozapps/extensions/test/xpcshell/test_provider_shutdown.js

@@ -72,9 +72,9 @@ add_task(async function blockRepoShutdown() {
   await mockProvider.shutdownPromise;
   // check AsyncShutdown state
   let status = fetchState();
-  equal(findInStatus(status[0], "Mock provider"), "(none)");
-  equal(status[1].name, "AddonRepository: async shutdown");
-  equal(status[1].state, "pending");
+  equal(findInStatus(status[1], "Mock provider"), "(none)");
+  equal(status[2].name, "AddonRepository: async shutdown");
+  equal(status[2].state, "pending");
   // let the provider finish
   mockProvider.doneResolve();
 
@@ -82,10 +82,10 @@ add_task(async function blockRepoShutdown() {
   await mockRepo.shutdownPromise;
   // Check the shutdown state
   status = fetchState();
-  equal(status[0].name, "AddonManager: Waiting for providers to shut down.");
-  equal(status[0].state, "Complete");
-  equal(status[1].name, "AddonRepository: async shutdown");
-  equal(status[1].state, "in progress");
+  equal(status[1].name, "AddonManager: Waiting for providers to shut down.");
+  equal(status[1].state, "Complete");
+  equal(status[2].name, "AddonRepository: async shutdown");
+  equal(status[2].state, "in progress");
 
   // Now finish our shutdown, and wait for the manager to wrap up
   mockRepo.doneResolve();

toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js

@@ -11,7 +11,7 @@ const IGNORE = ["getPreferredIconURL", "escapeAddonURI",
                 "addInstallListener", "removeInstallListener",
                 "addManagerListener", "removeManagerListener",
                 "addExternalExtensionLoader",
-                "shutdown", "init",
+                "beforeShutdown", "init",
                 "stateToString", "errorToString", "getUpgradeListener",
                 "addUpgradeListener", "removeUpgradeListener",
                 "getInstallSourceFromHost", "getInstallSourceFromPrincipal",
@@ -21,7 +21,7 @@ const IGNORE_PRIVATE = ["AddonAuthor", "AddonCompatibilityOverride",
                         "AddonScreenshot", "AddonType", "startup", "shutdown",
                         "addonIsActive", "registerProvider", "unregisterProvider",
                         "addStartupChange", "removeStartupChange",
-                        "getNewSideloads",
+                        "getNewSideloads", "finalShutdown",
                         "recordTimestamp", "recordSimpleMeasure",
                         "recordException", "getSimpleMeasures", "simpleTimer",
                         "setTelemetryDetails", "getTelemetryDetails",

toolkit/mozapps/extensions/test/xpcshell/test_shutdown_barriers.js

@@ -0,0 +1,67 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+"use strict";
+
+add_task(async function test_shutdown_barriers() {
+  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "66");
+
+  await promiseStartupManager();
+
+  const ID = "thing@xpcshell.addons.mozilla.org";
+  const VERSION = "1.42";
+
+  let xpi = await createTempWebExtensionFile({
+    manifest: {
+      applications: {gecko: {id: ID}},
+      version: VERSION,
+    },
+  });
+
+  await AddonManager.installTemporaryAddon(xpi);
+
+  let blockersComplete = 0;
+  AddonManager.beforeShutdown.addBlocker("Before shutdown", async () => {
+    equal(blockersComplete, 0, "No blockers have run yet");
+
+    // Delay a bit to make sure this doesn't succeed because of a timing
+    // fluke.
+    await delay(1000);
+
+    let addon = await AddonManager.getAddonByID(ID);
+    checkAddon(ID, addon, {
+      version: VERSION,
+      userDisabled: false,
+      appDisabled: false,
+    });
+
+    await addon.disable();
+
+    // Delay again for the same reasons.
+    await delay(1000);
+
+    addon = await AddonManager.getAddonByID(ID);
+    checkAddon(ID, addon, {
+      version: VERSION,
+      userDisabled: true,
+      appDisabled: false,
+    });
+
+    blockersComplete++;
+  });
+  AddonManagerPrivate.finalShutdown.addBlocker("Before shutdown", async () => {
+    equal(blockersComplete, 1, "Before shutdown blocker has run");
+
+    // Should probably try to access XPIDatabase here to make sure it
+    // doesn't work correctly, but it's a bit hairy.
+
+    // Delay a bit to make sure this doesn't succeed because of a timing
+    // fluke.
+    await delay(1000);
+
+    blockersComplete++;
+  });
+
+  await promiseShutdownManager();
+  equal(blockersComplete, 2, "Both shutdown blockers ran before manager shutdown completed");
+});

toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini

@@ -90,6 +90,7 @@ tags = webextensions
 [test_schema_change.js]
 [test_seen.js]
 [test_shutdown.js]
+[test_shutdown_barriers.js]
 [test_sideloads.js]
 [test_signed_inject.js]
 # Bug 1394122