Bug 1959339 - Stop waiting for background load when aborted on shutdown a=RyanVM

Original Revision: https://phabricator.services.mozilla.com/D256910

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

toolkit/components/extensions/ExtensionParent.sys.mjs

@@ -1872,14 +1872,17 @@ const DebugUtils = {
  * was received by the message manager. The promise is rejected if the message
  * manager was closed before a message was received.
  *
+ * Accepts an AbortSignal to allow early unregistration of the listeners.
+ *
  * @param {MessageListenerManager} messageManager
  *        The message manager on which to listen for messages.
  * @param {string} messageName
  *        The message to listen for.
+ * @param {AbortSignal} abortSignal
  * @returns {Promise<*>}
  */
-function promiseMessageFromChild(messageManager, messageName) {
-  return new Promise((resolve, reject) => {
+function promiseMessageFromChild(messageManager, messageName, abortSignal) {
+  const promise = new Promise((resolve, reject) => {
     let unregister;
     function listener(message) {
       unregister();
@@ -1897,19 +1900,88 @@ function promiseMessageFromChild(messageManager, messageName) {
     }
     unregister = () => {
       Services.obs.removeObserver(observer, "message-manager-close");
+      abortSignal.removeEventListener("abort", unregister);
       messageManager.removeMessageListener(messageName, listener);
+      messageManager = null;
     };
     messageManager.addMessageListener(messageName, listener);
     Services.obs.addObserver(observer, "message-manager-close");
+    abortSignal.addEventListener("abort", unregister);
   });
+  return promise;
+}
+
+/**
+ * Returns a Promise which rejects if the load in the browser is aborted.
+ * Accepts an AbortSignal to allow early unregistration of the listeners.
+ *
+ * @param {XULBrowserElement} browser
+ * @param {AbortSignal} abortSignal
+ * @returns {Promise<void>} A promise that never resolves, but only rejects.
+ */
+function promiseBrowserStopped(browser, abortSignal) {
+  const { promise, reject } = Promise.withResolvers();
+  let unregister;
+  let listener = {
+    QueryInterface: ChromeUtils.generateQI([
+      "nsIWebProgressListener",
+      "nsISupportsWeakReference",
+    ]),
+
+    onStateChange(webProgress, request, stateFlags, status) {
+      if (
+        webProgress.isTopLevel &&
+        stateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
+        !Components.isSuccessCode(status) &&
+        // Ignore state change triggered by navigating away from about:blank.
+        status !== Cr.NS_BINDING_ABORTED
+      ) {
+        unregister();
+        // Known failures (and test coverage):
+        // - NS_ERROR_ILLEGAL_DURING_SHUTDOWN (test_ext_background_early_quit.js)
+        // - NS_ERROR_FILE_NOT_FOUND (test_ext_background_file_invalid.js)
+        reject(
+          new Error(
+            `Browser load failed: ${ChromeUtils.getXPCOMErrorName(status)}`
+          )
+        );
+      }
+    },
+  };
+
+  unregister = () => {
+    // browser.removeProgressListener throws if browser.webProgress is null.
+    if (browser?.webProgress) {
+      browser.removeProgressListener(listener);
+    }
+    abortSignal.removeEventListener("abort", unregister);
+    listener = null;
+    browser = null;
+  };
+  browser.addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
+  abortSignal.addEventListener("abort", unregister);
+  return promise;
 }
 
 // This should be called before browser.loadURI is invoked.
 async function promiseBackgroundViewLoaded(browser) {
-  let { childId } = await promiseMessageFromChild(
+  const abortController = new AbortController();
+  const messagePromise = promiseMessageFromChild(
     browser.messageManager,
-    "Extension:BackgroundViewLoaded"
+    "Extension:BackgroundViewLoaded",
+    abortController.signal
   );
+  const stopPromise = promiseBrowserStopped(browser, abortController.signal);
+
+  let childId;
+  try {
+    // stopPromise only rejects, so a non-rejection is from messagePromise.
+    let message = await Promise.race([messagePromise, stopPromise]);
+    childId = message.childId;
+  } finally {
+    abortController.abort();
+  }
+
   if (childId) {
     return ParentAPIManager.getContextById(childId);
   }

toolkit/components/extensions/test/xpcshell/test_ext_background_early_quit.js

@@ -0,0 +1,57 @@
+"use strict";
+
+const { Management } = ChromeUtils.importESModule(
+  "resource://gre/modules/Extension.sys.mjs"
+);
+
+// This test task quits in the middle of the test - do NOT add more tests below
+// this, unless you explicitly want to verify the behavior after quitting.
+//
+// Regression test for https://bugzilla.mozilla.org/show_bug.cgi?id=1959339
+// Quitting while an extension is starting should not block shutdown.
+// Previously, if an extension was starting at application startup, and the
+// application quit before the background page startup completed, then the
+// implementation would wait indefinitely for the completion of background
+// startup (which would not trigger because past shutdown any attempt to load
+// content is aborted with NS_ERROR_ILLEGAL_DURING_SHUTDOWN).
+add_task(async function test_quit_while_background_starts() {
+  let extension = ExtensionTestUtils.loadExtension({
+    // Extension startup is blocked on background startup (bug 1543354).
+    // If we somehow fail to make progress, then we should notice that.
+    delayedStartup: false,
+    background() {
+      browser.test.fail(
+        "Unexpected background page execution. eForceQuit should have aborted all document loads"
+      );
+    },
+  });
+
+  info("Waiting for extension to start up");
+  let browserCount = 0;
+  Management.once("extension-browser-inserted", (eventName, browser) => {
+    ++browserCount;
+    equal(
+      browser.getAttribute("webextension-view-type"),
+      "background",
+      "Got background browser"
+    );
+    // The Quit() call below calls ExitLastWindowClosingSurvivalArea() at
+    // https://searchfox.org/mozilla-central/rev/38e462fe13ea42ae6cc391fb36e8b9e82e842b00/toolkit/components/startup/nsAppStartup.cpp#428,431
+    // which expects an EnterLastWindowClosingSurvivalArea() to have called
+    // before, or else the following assertion will be triggered at:
+    // https://searchfox.org/mozilla-central/rev/38e462fe13ea42ae6cc391fb36e8b9e82e842b00/toolkit/components/startup/nsAppStartup.cpp#597-598
+    // ASSERTION: consider quit stopper out of bounds: 'mConsiderQuitStopper > 0
+    //
+    // During normal (non-xpcshell) execution, nsAppStartup::Run() runs, which
+    // calls EnterLastWindowClosingSurvivalArea(). In xpcshell tests, this is
+    // not called, and we need to call it here:
+    Services.startup.enterLastWindowClosingSurvivalArea();
+    Services.startup.quit(Ci.nsIAppStartup.eForceQuit);
+  });
+  await extension.startup();
+  equal(browserCount, 1, "Seen background browser");
+
+  equal(extension.extension.backgroundState, "stopped", "backgroundState");
+
+  await extension.unload();
+});

toolkit/components/extensions/test/xpcshell/test_ext_background_file_invalid.js

@@ -0,0 +1,55 @@
+"use strict";
+
+add_task(async function test_missing_background_file() {
+  let extension = ExtensionTestUtils.loadExtension({
+    // Extension startup is blocked on background startup (bug 1543354).
+    // If we somehow fail to make progress, then we should notice that.
+    delayedStartup: false,
+    manifest: {
+      background: { page: "non_existing_background_page.html" },
+    },
+    files: {
+      "tab.html": `<script src="tab.js"></script>`,
+      "tab.js": async () => {
+        browser.test.assertEq(
+          browser.extension.getBackgroundPage(),
+          null,
+          "extension.getBackgroundPage() is null"
+        );
+        browser.test.assertEq(
+          await browser.runtime.getBackgroundPage(),
+          null,
+          "runtime.getBackgroundPage() is null"
+        );
+        browser.test.sendMessage("done");
+      },
+    },
+  });
+  info("Waiting for extension to start up");
+  await extension.startup();
+
+  if (WebExtensionPolicy.useRemoteWebExtensions) {
+    // TODO bug 1978688: This is questionable, "stopped" would make more sense.
+    // The current implementation detects the background load, because the
+    // DOMContentLoaded event is fired right before the load navigates to an
+    // error page.
+    equal(extension.extension.backgroundState, "running", "backgroundState");
+  } else {
+    equal(extension.extension.backgroundState, "stopped", "backgroundState");
+  }
+
+  let contentPage = await ExtensionTestUtils.loadContentPage(
+    `moz-extension://${extension.uuid}/tab.html`
+  );
+  await extension.awaitMessage("done");
+  await contentPage.close();
+
+  equal(
+    extension.extension.backgroundState,
+    // Should ideally be "stopped", but see above.
+    WebExtensionPolicy.useRemoteWebExtensions ? "running" : "stopped",
+    "backgroundState not changed"
+  );
+
+  await extension.unload();
+});

toolkit/components/extensions/test/xpcshell/xpcshell-common.toml

@@ -42,10 +42,14 @@ skip-if = ["os == 'android'"] # Bug 1545439
 
 ["test_ext_background_api_injection.js"]
 
+["test_ext_background_early_quit.js"]
+
 ["test_ext_background_early_shutdown.js"]
 
 ["test_ext_background_empty.js"]
 
+["test_ext_background_file_invalid.js"]
+
 ["test_ext_background_generated_load_events.js"]
 
 ["test_ext_background_generated_reload.js"]