Bug 1916424 - use category manager for quit-application-granted consumers from BrowserGlue, r=firefox-desktop-core-reviewers ,mossop

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

browser/components/BrowserComponents.manifest

@@ -34,4 +34,19 @@ category browser-idle-startup resource://gre/modules/UpdateListener.sys.mjs Upda
 category browser-idle-startup resource:///modules/WindowsJumpLists.sys.mjs WinTaskbarJumpList.startup
 #endif
 
+# App shutdown consumers
+category browser-quit-application-granted resource:///modules/BrowserUsageTelemetry.sys.mjs BrowserUsageTelemetry.uninit
+category browser-quit-application-granted resource:///modules/Interactions.sys.mjs Interactions.uninit
+category browser-quit-application-granted resource:///modules/pagedata/PageDataService.sys.mjs PageDataService.uninit
+category browser-quit-application-granted resource://gre/modules/PageThumbs.sys.mjs PageThumbs.uninit
+category browser-quit-application-granted resource://gre/modules/NewTabUtils.sys.mjs NewTabUtils.uninit
+category browser-quit-application-granted resource://normandy/Normandy.sys.mjs Normandy.uninit
+category browser-quit-application-granted resource://gre/modules/RFPHelper.sys.mjs RFPHelper.uninit
+category browser-quit-application-granted resource:///modules/ShoppingUtils.sys.mjs ShoppingUtils.uninit
+category browser-quit-application-granted resource:///modules/asrouter/ASRouterNewTabHook.sys.mjs ASRouterNewTabHook.destroy
+#ifdef MOZ_UPDATER
+category browser-quit-application-granted resource://gre/modules/UpdateListener.sys.mjs UpdateListener.reset
+#endif
+category browser-quit-application-granted resource:///modules/UrlbarSearchTermsPersistence.sys.mjs UrlbarSearchTermsPersistence.uninit
+
 category search-service-notification resource:///modules/SearchUIUtils.sys.mjs SearchUIUtils.showSearchServiceNotification

browser/components/BrowserGlue.sys.mjs

@@ -74,7 +74,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.sys.mjs",
   ProcessHangMonitor: "resource:///modules/ProcessHangMonitor.sys.mjs",
   QuickSuggest: "resource:///modules/QuickSuggest.sys.mjs",
-  RFPHelper: "resource://gre/modules/RFPHelper.sys.mjs",
   RemoteSecuritySettings:
     "resource://gre/modules/psm/RemoteSecuritySettings.sys.mjs",
   RemoteSettings: "resource://services-settings/remote-settings.sys.mjs",
@@ -92,7 +91,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   SessionStore: "resource:///modules/sessionstore/SessionStore.sys.mjs",
   ShellService: "resource:///modules/ShellService.sys.mjs",
   ShortcutUtils: "resource://gre/modules/ShortcutUtils.sys.mjs",
-  ShoppingUtils: "resource:///modules/ShoppingUtils.sys.mjs",
   SpecialMessageActions:
     "resource://messaging-system/lib/SpecialMessageActions.sys.mjs",
   TelemetryReportingPolicy:
@@ -100,8 +98,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   TRRRacer: "resource:///modules/TRRPerformance.sys.mjs",
   TabCrashHandler: "resource:///modules/ContentCrashHandlers.sys.mjs",
   UrlbarPrefs: "resource:///modules/UrlbarPrefs.sys.mjs",
-  UrlbarSearchTermsPersistence:
-    "resource:///modules/UrlbarSearchTermsPersistence.sys.mjs",
   UsageReporting: "resource://gre/modules/UsageReporting.sys.mjs",
   WebChannel: "resource://gre/modules/WebChannel.sys.mjs",
   WebProtocolHandlerRegistrar:
@@ -113,24 +109,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   setTimeout: "resource://gre/modules/Timer.sys.mjs",
 });
 
-if (AppConstants.MOZ_UPDATER) {
-  ChromeUtils.defineESModuleGetters(lazy, {
-    UpdateListener: "resource://gre/modules/UpdateListener.sys.mjs",
-  });
-  XPCOMUtils.defineLazyServiceGetters(lazy, {
-    UpdateServiceStub: [
-      "@mozilla.org/updates/update-service-stub;1",
-      "nsIApplicationUpdateServiceStub",
-    ],
-  });
-}
-if (AppConstants.MOZ_UPDATE_AGENT) {
-  ChromeUtils.defineESModuleGetters(lazy, {
-    BackgroundUpdate: "resource://gre/modules/BackgroundUpdate.sys.mjs",
-  });
-}
-
-// PluginManager is used in the listeners object below.
 XPCOMUtils.defineLazyServiceGetters(lazy, {
   BrowserHandler: ["@mozilla.org/browser/clh;1", "nsIBrowserHandler"],
   PushService: ["@mozilla.org/push/Service;1", "nsIPushService"],
@@ -1032,6 +1010,12 @@ const listeners = {
   },
 };
 if (AppConstants.MOZ_UPDATER) {
+  ChromeUtils.defineESModuleGetters(lazy, {
+    // This listeners/observers/lazy indirection is too much for eslint:
+    // eslint-disable-next-line mozilla/valid-lazy
+    UpdateListener: "resource://gre/modules/UpdateListener.sys.mjs",
+  });
+
   listeners.observers["update-downloading"] = ["UpdateListener"];
   listeners.observers["update-staged"] = ["UpdateListener"];
   listeners.observers["update-downloaded"] = ["UpdateListener"];
@@ -2192,8 +2176,28 @@ BrowserGlue.prototype = {
 
   /**
    * Application shutdown handler.
+   *
+   * If you need new code to be called on shutdown, please use
+   * the category manager browser-quit-application-granted category
+   * instead of adding new manual code to this function.
    */
   _onQuitApplicationGranted() {
+    function failureHandler(ex) {
+      if (Cu.isInAutomation) {
+        // This usually happens after the test harness is done collecting
+        // test errors, thus we can't easily add a failure to it. The only
+        // noticeable solution we have is crashing.
+        Cc["@mozilla.org/xpcom/debug;1"]
+          .getService(Ci.nsIDebug2)
+          .abort(ex.filename, ex.lineNumber);
+      }
+    }
+
+    lazy.BrowserUtils.callModulesFromCategory({
+      categoryName: "browser-quit-application-granted",
+      failureHandler,
+    });
+
     let tasks = [
       // This pref must be set here because SessionStore will use its value
       // on quit-application.
@@ -2213,28 +2217,16 @@ BrowserGlue.prototype = {
         }
       },
 
-      () => lazy.BrowserUsageTelemetry.uninit(),
+      // These should also be moved to use the category manager, but ran into
+      // leaking issues. Bug 1949294 tracks.
       () => lazy.SearchSERPTelemetry.uninit(),
       () => lazy.SearchSERPCategorization.uninit(),
-      () => lazy.Interactions.uninit(),
+
-      () => lazy.PageDataService.uninit(),
-      () => lazy.PageThumbs.uninit(),
-      () => lazy.NewTabUtils.uninit(),
-      () => lazy.Normandy.uninit(),
-      () => lazy.RFPHelper.uninit(),
-      () => lazy.ShoppingUtils.uninit(),
-      () => lazy.ASRouterNewTabHook.destroy(),
-      () => {
-        if (AppConstants.MOZ_UPDATER) {
-          lazy.UpdateListener.reset();
-        }
-      },
       () => {
         // bug 1839426 - The FOG service needs to be instantiated reliably so it
         // can perform at-shutdown tasks later in shutdown.
         Services.fog;
       },
-      () => lazy.UrlbarSearchTermsPersistence.uninit(),
     ];
 
     for (let task of tasks) {
@@ -2242,14 +2234,7 @@ BrowserGlue.prototype = {
         task();
       } catch (ex) {
         console.error(`Error during quit-application-granted: ${ex}`);
-        if (Cu.isInAutomation) {
+        failureHandler(ex);
-          // This usually happens after the test harness is done collecting
-          // test errors, thus we can't easily add a failure to it. The only
-          // noticeable solution we have is crashing.
-          Cc["@mozilla.org/xpcom/debug;1"]
-            .getService(Ci.nsIDebug2)
-            .abort(ex.filename, ex.lineNumber);
-        }
       }
     }
   },
@@ -3019,22 +3004,25 @@ BrowserGlue.prototype = {
 
       {
         name: "BackgroundUpdate",
-        condition: AppConstants.MOZ_UPDATE_AGENT,
+        condition: AppConstants.MOZ_UPDATE_AGENT && AppConstants.MOZ_UPDATER,
         task: async () => {
+          let updateServiceStub = Cc[
+            "@mozilla.org/updates/update-service-stub;1"
+          ].getService(Ci.nsIApplicationUpdateServiceStub);
           // Never in automation!
-          if (
+          if (!updateServiceStub.updateDisabledForTesting) {
-            AppConstants.MOZ_UPDATER &&
+            let { BackgroundUpdate } = ChromeUtils.importESModule(
-            !lazy.UpdateServiceStub.updateDisabledForTesting
+              "resource://gre/modules/BackgroundUpdate.sys.mjs"
-          ) {
+            );
             try {
-              await lazy.BackgroundUpdate.scheduleFirefoxMessagingSystemTargetingSnapshotting();
+              await BackgroundUpdate.scheduleFirefoxMessagingSystemTargetingSnapshotting();
             } catch (e) {
               console.error(
                 "There was an error scheduling Firefox Messaging System targeting snapshotting: ",
                 e
               );
             }
-            await lazy.BackgroundUpdate.maybeScheduleBackgroundUpdateTask();
+            await BackgroundUpdate.maybeScheduleBackgroundUpdateTask();
           }
         },
       },

toolkit/modules/BrowserUtils.sys.mjs

@@ -499,11 +499,19 @@ export var BrowserUtils = {
    * @param {string} [options.profilerMarker=""]
    *        If specified, will create a profiler marker with the provided
    *        identifier for each consumer.
+   * @param {function} [options.failureHandler]
+   *        If specified, will be called for any exceptions raised, in
+   *        order to do custom failure handling.
    * @param {...any} args
    *        Arguments to pass to the consumers.
    */
   callModulesFromCategory(
-    { categoryName, profilerMarker = "", idleDispatch = false },
+    {
+      categoryName,
+      profilerMarker = "",
+      idleDispatch = false,
+      failureHandler = null,
+    },
     ...args
   ) {
     // Use an async function for profiler markers and error handling.
@@ -518,6 +526,17 @@ export var BrowserUtils = {
           `Error in processing ${categoryName} for ${fn._descriptiveName}`
         );
         console.error(ex);
+        try {
+          await failureHandler?.(ex);
+        } catch (nestedEx) {
+          console.error(`Error in handling failure: ${nestedEx}`);
+          // Crash in automation:
+          if (BrowserUtils._inAutomation) {
+            Cc["@mozilla.org/xpcom/debug;1"]
+              .getService(Ci.nsIDebug2)
+              .abort(nestedEx.filename, nestedEx.lineNumber);
+          }
+        }
       }
       if (profilerMarker) {
         ChromeUtils.addProfilerMarker(

toolkit/modules/tests/xpcshell/my_catman_1.sys.mjs

@@ -6,4 +6,7 @@ export let Module1 = {
       arg
     );
   },
+  throwingFunction() {
+    throw new Error("Uh oh. I have a bad feeling about this.");
+  },
 };

toolkit/modules/tests/xpcshell/test_BrowserUtils.js

@@ -399,3 +399,66 @@ add_task(async function test_callModulesFromCategory() {
 
   Services.obs.removeObserver(ob, OBSTOPIC1);
 });
+
+// Test that errors are reported but do not throw at the callsite,
+// and that custom error handlers are invoked.
+add_task(async function test_callModulesFromCategory_errors() {
+  const OTHER_CAT = "someothercat";
+  const MODULE1 = "resource://test/my_catman_1.sys.mjs";
+
+  // Add an item that doesn't exist, and check that although we report errors,
+  // the callsite doesn't throw.
+  let catManUpdated = TestUtils.topicObserved("xpcom-category-entry-added");
+  Services.catMan.addCategoryEntry(
+    OTHER_CAT,
+    MODULE1,
+    `Module1.nonExistantFunction`,
+    false,
+    false
+  );
+  await catManUpdated;
+  let catEntries = Array.from(Services.catMan.enumerateCategory(OTHER_CAT));
+  Assert.equal(catEntries.length, 1);
+
+  let consolePromise = TestUtils.consoleMessageObserved(m => {
+    let firstArg = m.wrappedJSObject.arguments?.[0]?.message;
+    return typeof firstArg == "string" && firstArg.includes("not a function");
+  });
+  BrowserUtils.callModulesFromCategory(
+    {
+      categoryName: OTHER_CAT,
+    },
+    "Hello"
+  );
+  let reportedError = await consolePromise;
+  let firstArg = reportedError.wrappedJSObject.arguments?.[0]?.message;
+  Assert.stringContains(
+    firstArg,
+    MODULE1,
+    "Error message should include module URL."
+  );
+  Services.catMan.deleteCategoryEntry(OTHER_CAT, MODULE1, false);
+
+  // Check that custom exception handling from extant methods works:
+  catManUpdated = TestUtils.topicObserved("xpcom-category-entry-added");
+  Services.catMan.addCategoryEntry(
+    OTHER_CAT,
+    MODULE1,
+    `Module1.throwingFunction`,
+    false,
+    false
+  );
+  await catManUpdated;
+  Assert.equal(catEntries.length, 1);
+  let exHandler = Promise.withResolvers();
+  BrowserUtils.callModulesFromCategory({
+    categoryName: OTHER_CAT,
+    failureHandler: exHandler.resolve,
+  });
+  let caughtException = await exHandler.promise;
+  Assert.stringContains(
+    caughtException.message,
+    "Uh oh",
+    "Exceptions should be handled."
+  );
+});