Bug 1601678 - Resolve shutdown deadlock in EnvironmentAddonBuilder r=mixedpuppy

TelemetryEnvironment.jsm's EnvironmentAddonBuilder has a shutdown
blocker that depends on the addons database to have been loaded.
There are two calls to asyncLoadDB() in XPIProvider.jsm that are
supposed to activate the load. Neither of them work:

- XPIProvider calls asyncLoadDB() during quitApplicationGranted.
  But "quit-application-granted" is not always triggered, as seen in:
  https://bugzilla.mozilla.org/show_bug.cgi?id=1601678#c12

- XPIProvider.shutdown() calls asyncLoadDB().
  But shutdown() is only called when TelemetryEnvironment's blocker has
  been released. So this is never reached. More details in:
  https://bugzilla.mozilla.org/show_bug.cgi?id=1601678#c7

To fix the deadlock, asyncLoadDB() is called from profile-before-change,
which is the same phase as the blocker of EnvironmentAddonBuilder.

The two existing calls to asyncLoadDB() mentioned above are obsolete and
have been removed.

---

After the removal of asyncLoadDB() from XPIProvider.shutdown(), the
test_ext_persistent_events.js test started to fail. This is because the
test sends the "sessionstore-windows-restored" notification, for which
XPIProvider has a handler that calls asyncLoadDB(), without awaiting
the result.
Since XPIProvider.shutdown() doesn't await the DB load any more, it is
possible for the DB to be unloaded while being used. This only happens
in tests, because the construction with the TelemetryEnvironment ensures
that the addons database has fully loaded before shutdown() is called.

To resolve this test-only issue, AddonTestUtils.promiseShutdownManager()
has been updated to explicitly wait for the pending _dbPromise if any.

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

toolkit/mozapps/extensions/internal/AddonTestUtils.jsm

@@ -1029,8 +1029,20 @@ var AddonTestUtils = {
       this.overrideEntry = null;
     }
 
+    const XPIscope = ChromeUtils.import(
+      "resource://gre/modules/addons/XPIProvider.jsm",
+      null
+    );
+
     Services.obs.notifyObservers(null, "quit-application-granted");
     await MockAsyncShutdown.quitApplicationGranted.trigger();
+
+    // If XPIDatabase.asyncLoadDB() has been called before, then _dbPromise is
+    // a promise, potentially still pending. Wait for it to settle before
+    // triggering profileBeforeChange, because the latter can trigger errors in
+    // the pending asyncLoadDB() by an indirect call to XPIDatabase.shutdown().
+    await XPIscope.XPIDatabase._dbPromise;
+
     await MockAsyncShutdown.profileBeforeChange.trigger();
     await MockAsyncShutdown.profileChangeTeardown.trigger();
 
@@ -1052,10 +1064,7 @@ var AddonTestUtils = {
 
     // Force the XPIProvider provider to reload to better
     // simulate real-world usage.
-    let XPIscope = ChromeUtils.import(
-      "resource://gre/modules/addons/XPIProvider.jsm",
-      null
-    );
+
     // This would be cleaner if I could get it as the rejection reason from
     // the AddonManagerInternal.shutdown() promise
     let shutdownError = XPIscope.XPIDatabase._saveError;

toolkit/mozapps/extensions/internal/XPIProvider.jsm

@@ -2558,8 +2558,6 @@ var XPIProvider = {
         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
@@ -2618,6 +2616,13 @@ var XPIProvider = {
       // sessionstore-windows-restored.  In a browser toolbox process
       // we wait for the toolbox to show up, based on xul-window-visible
       // and a visible toolbox window.
+      //
+      // TelemetryEnvironment's EnvironmentAddonBuilder awaits databaseReady
+      // before releasing a blocker on AddonManager.beforeShutdown, which in its
+      // turn is a blocker of a shutdown blocker at "profile-before-change".
+      // To avoid a deadlock, trigger the DB load at "profile-before-change" if
+      // the database hasn't started loading yet.
+      //
       // Finally, we have a test-only event called test-load-xpi-database
       // as a temporary workaround for bug 1372845.  The latter can be
       // cleaned up when that bug is resolved.
@@ -2625,6 +2630,7 @@ var XPIProvider = {
         const EVENTS = [
           "sessionstore-windows-restored",
           "xul-window-visible",
+          "profile-before-change",
           "test-load-xpi-database",
         ];
         let observer = (subject, topic, data) => {
@@ -2689,13 +2695,6 @@ var XPIProvider = {
       Services.prefs.setBoolPref(PREF_PENDING_OPERATIONS, false);
     }
 
-    // Ugh, if we reach this point without loading the xpi database,
-    // we need to load it know, otherwise the telemetry shutdown blocker
-    // will never resolve.
-    if (!XPIDatabase.initialized) {
-      await XPIDatabase.asyncLoadDB();
-    }
-
     await XPIDatabase.shutdown();
   },
 

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

@@ -0,0 +1,61 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+"use strict";
+
+const { TelemetryEnvironment } = ChromeUtils.import(
+  "resource://gre/modules/TelemetryEnvironment.jsm"
+);
+
+// Regression test for bug 1665568: verifies that AddonManager unblocks shutdown
+// when startup is interrupted very early.
+add_task(async function test_shutdown_immediately_after_startup() {
+  // Set as migrated to prevent sync DB load at startup.
+  Services.prefs.setBoolPref("extensions.incognito.migrated", true);
+  Services.prefs.setCharPref("extensions.lastAppVersion", "42");
+  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "42");
+
+  Cc["@mozilla.org/addons/integration;1"]
+    .getService(Ci.nsIObserver)
+    .observe(null, "addons-startup", null);
+
+  // Above, we have configured the runtime to avoid a forced synchronous load
+  // of the database. Confirm that this is indeed the case.
+  equal(AddonManagerPrivate.isDBLoaded(), false, "DB not loaded synchronously");
+
+  let shutdownCount = 0;
+  AddonManager.beforeShutdown.addBlocker("count", async () => ++shutdownCount);
+
+  let databaseLoaded = false;
+  AddonManagerPrivate.databaseReady.then(() => {
+    databaseLoaded = true;
+  });
+
+  // Accessing TelemetryEnvironment.currentEnvironment triggers initialization
+  // of TelemetryEnvironment / EnvironmentAddonBuilder, which registers a
+  // shutdown blocker.
+  equal(
+    TelemetryEnvironment.currentEnvironment.addons,
+    undefined,
+    "TelemetryEnvironment.currentEnvironment.addons is uninitialized"
+  );
+
+  info("Immediate exit at startup, without quit-application-granted");
+  Services.obs.notifyObservers(null, "profile-before-change");
+  let shutdownPromise = MockAsyncShutdown.profileBeforeChange.trigger();
+  equal(shutdownCount, 1, "AddonManager.beforeShutdown has started");
+
+  // Note: Until now everything ran in the same tick of the event loop.
+
+  // Waiting for AddonManager to have shut down.
+  await shutdownPromise;
+
+  ok(databaseLoaded, "Addon DB loaded for use by TelemetryEnvironment");
+  equal(AddonManagerPrivate.isDBLoaded(), false, "DB unloaded after shutdown");
+
+  Assert.deepEqual(
+    TelemetryEnvironment.currentEnvironment.addons.activeAddons,
+    {},
+    "TelemetryEnvironment.currentEnvironment.addons is initialized"
+  );
+});

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

@@ -90,6 +90,7 @@ tags = webextensions
 [test_seen.js]
 [test_shutdown.js]
 [test_shutdown_barriers.js]
+[test_shutdown_early.js]
 [test_sideload_scopes.js]
 head = head_addons.js head_sideload.js
 skip-if = os == "linux" # Bug 1613268