Bug 1525729 - Ignore searchInitialized promise on shutdown r=aswan

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

--HG--
extra : moz-landing-system : lando

browser/components/extensions/parent/ext-browser.js

@@ -244,8 +244,8 @@ global.TabContext = class extends EventEmitter {
 
 // This promise is used to wait for the search service to be initialized.
 // None of the code in the WebExtension modules requests that initialization.
-// It is assumed that it is started at some point. If tests start to fail
-// because this promise never resolves, that's likely the cause.
+// It is assumed that it is started at some point. That might never happen,
+// e.g. if the application shuts down before the search service initializes.
 XPCOMUtils.defineLazyGetter(global, "searchInitialized", () => {
   if (Services.search.isInitialized) {
     return Promise.resolve();

browser/components/extensions/parent/ext-chrome-settings-overrides.js

@@ -79,6 +79,13 @@ async function handleInitialHomepagePopup(extensionId, homepageUrl) {
   homepagePopup.addObserver(extensionId);
 }
 
+// When an extension starts up, a search engine may asynchronously be
+// registered, without blocking the startup. When an extension is
+// uninstalled, we need to wait for this registration to finish
+// before running the uninstallation handler.
+// Map[extension id -> Promise]
+var pendingSearchSetupTasks = new Map();
+
 this.chrome_settings_overrides = class extends ExtensionAPI {
   static async processDefaultSearchSetting(action, id) {
     await ExtensionSettingsStore.initialize();
@@ -131,7 +138,11 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
     ]);
   }
 
-  static onUninstall(id) {
+  static async onUninstall(id) {
+    let searchStartupPromise = pendingSearchSetupTasks.get(id);
+    if (searchStartupPromise) {
+      await searchStartupPromise;
+    }
     // Note: We do not have to deal with homepage here as it is managed by
     // the ExtensionPreferencesManager.
     return Promise.all([
@@ -190,68 +201,90 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
       });
     }
     if (manifest.chrome_settings_overrides.search_provider) {
-      await searchInitialized;
-      extension.callOnClose({
-        close: () => {
-          if (extension.shutdownReason == "ADDON_DISABLE") {
-            chrome_settings_overrides.processDefaultSearchSetting("disable", extension.id);
-            chrome_settings_overrides.removeEngine(extension.id);
+      // Registering a search engine can potentially take a long while,
+      // or not complete at all (when searchInitialized is never resolved),
+      // so we are deliberately not awaiting the returned promise here.
+      let searchStartupPromise =
+        this.processSearchProviderManifestEntry().finally(() => {
+          if (pendingSearchSetupTasks.get(extension.id) === searchStartupPromise) {
+            pendingSearchSetupTasks.delete(extension.id);
           }
-        },
-      });
+        });
 
-      let searchProvider = manifest.chrome_settings_overrides.search_provider;
-      let engineName = searchProvider.name.trim();
-      if (searchProvider.is_default) {
+      // Save the promise so we can await at onUninstall.
+      pendingSearchSetupTasks.set(extension.id, searchStartupPromise);
+    }
+  }
+
+  async processSearchProviderManifestEntry() {
+    await searchInitialized;
+
+    let {extension} = this;
+    if (!extension) {
+      Cu.reportError(`Extension shut down before search provider was registered`);
+      return;
+    }
+    extension.callOnClose({
+      close: () => {
+        if (extension.shutdownReason == "ADDON_DISABLE") {
+          chrome_settings_overrides.processDefaultSearchSetting("disable", extension.id);
+          chrome_settings_overrides.removeEngine(extension.id);
+        }
+      },
+    });
+
+    let {manifest} = extension;
+    let searchProvider = manifest.chrome_settings_overrides.search_provider;
+    let engineName = searchProvider.name.trim();
+    if (searchProvider.is_default) {
+      let engine = Services.search.getEngineByName(engineName);
+      let defaultEngines = await Services.search.getDefaultEngines();
+      if (engine && defaultEngines.some(defaultEngine => defaultEngine.name == engineName)) {
+        // Needs to be called every time to handle reenabling, but
+        // only sets default for install or enable.
+        await this.setDefault(engineName);
+        // For built in search engines, we don't do anything further
+        return;
+      }
+    }
+    await this.addSearchEngine();
+    if (searchProvider.is_default) {
+      if (extension.startupReason === "ADDON_INSTALL") {
+        // Don't ask if it already the current engine
         let engine = Services.search.getEngineByName(engineName);
-        let defaultEngines = await Services.search.getDefaultEngines();
-        if (engine && defaultEngines.some(defaultEngine => defaultEngine.name == engineName)) {
-          // Needs to be called every time to handle reenabling, but
-          // only sets default for install or enable.
-          await this.setDefault(engineName);
-          // For built in search engines, we don't do anything further
-          return;
-        }
-      }
-      await this.addSearchEngine();
-      if (searchProvider.is_default) {
-        if (extension.startupReason === "ADDON_INSTALL") {
-          // Don't ask if it already the current engine
-          let engine = Services.search.getEngineByName(engineName);
-          let defaultEngine = await Services.search.getDefault();
-          if (defaultEngine.name != engine.name) {
-            let subject = {
-              wrappedJSObject: {
-                // This is a hack because we don't have the browser of
-                // the actual install. This means the popup might show
-                // in a different window. Will be addressed in a followup bug.
-                browser: windowTracker.topWindow.gBrowser.selectedBrowser,
-                name: this.extension.name,
-                icon: this.extension.iconURL,
-                currentEngine: defaultEngine.name,
-                newEngine: engineName,
-                resolve(allow) {
-                  if (allow) {
-                    ExtensionSettingsStore.addSetting(
-                      extension.id, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => defaultEngine.name);
-                    Services.search.defaultEngine = Services.search.getEngineByName(engineName);
-                  }
-                },
+        let defaultEngine = await Services.search.getDefault();
+        if (defaultEngine.name != engine.name) {
+          let subject = {
+            wrappedJSObject: {
+              // This is a hack because we don't have the browser of
+              // the actual install. This means the popup might show
+              // in a different window. Will be addressed in a followup bug.
+              browser: windowTracker.topWindow.gBrowser.selectedBrowser,
+              name: this.extension.name,
+              icon: this.extension.iconURL,
+              currentEngine: defaultEngine.name,
+              newEngine: engineName,
+              resolve(allow) {
+                if (allow) {
+                  ExtensionSettingsStore.addSetting(
+                    extension.id, DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME, engineName, () => defaultEngine.name);
+                  Services.search.defaultEngine = Services.search.getEngineByName(engineName);
+                }
               },
-            };
-            Services.obs.notifyObservers(subject, "webextension-defaultsearch-prompt");
-          }
-        } else {
-          // Needs to be called every time to handle reenabling, but
-          // only sets default for install or enable.
-          this.setDefault(engineName);
+            },
+          };
+          Services.obs.notifyObservers(subject, "webextension-defaultsearch-prompt");
         }
-      } else if (ExtensionSettingsStore.hasSetting(extension.id,
-                                                   DEFAULT_SEARCH_STORE_TYPE,
-                                                   DEFAULT_SEARCH_SETTING_NAME)) {
-        // is_default has been removed, but we still have a setting. Remove it.
-        chrome_settings_overrides.processDefaultSearchSetting("removeSetting", extension.id);
+      } else {
+        // Needs to be called every time to handle reenabling, but
+        // only sets default for install or enable.
+        this.setDefault(engineName);
       }
+    } else if (ExtensionSettingsStore.hasSetting(extension.id,
+                                                 DEFAULT_SEARCH_STORE_TYPE,
+                                                 DEFAULT_SEARCH_SETTING_NAME)) {
+      // is_default has been removed, but we still have a setting. Remove it.
+      chrome_settings_overrides.processDefaultSearchSetting("removeSetting", extension.id);
     }
   }
 

browser/components/extensions/test/xpcshell/test_ext_settings_overrides_shutdown.js

@@ -0,0 +1,71 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+
+"use strict";
+
+const {AddonTestUtils} = ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm");
+const {ExtensionParent} = ChromeUtils.import("resource://gre/modules/ExtensionParent.jsm");
+
+AddonTestUtils.init(this);
+AddonTestUtils.overrideCertDB();
+AddonTestUtils.createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "42", "42");
+
+add_task(async function shutdown_during_search_provider_startup() {
+  await AddonTestUtils.promiseStartupManager();
+
+  let extension = ExtensionTestUtils.loadExtension({
+    useAddonManager: "permanent",
+    manifest: {
+      chrome_settings_overrides: {
+        search_provider: {
+          name: "dummy name",
+          search_url: "https://example.com/",
+        },
+      },
+    },
+  });
+
+  info("Starting up search extension");
+  await extension.startup();
+
+  let initialized = false;
+  ExtensionParent.apiManager.global.searchInitialized.then(() => {
+    initialized = true;
+  });
+
+  await extension.addon.disable();
+
+  info("Extension managed to shut down despite the uninitialized search");
+  // Initialize search after extension shutdown to check that it does not cause
+  // any problems, and that the test can continue to test uninstall behavior.
+  Assert.ok(!initialized, "Search service should not have been initialized");
+
+  extension.addon.enable();
+  await extension.awaitStartup();
+
+  // Check that uninstall is blocked until the search registration at startup
+  // has finished. This registration only finished once the search service is
+  // initialized.
+  let uninstallingPromise = new Promise(resolve => {
+    let Management = ExtensionParent.apiManager;
+    Management.on("uninstall", function listener(eventName, {id}) {
+      Management.off("uninstall", listener);
+      Assert.equal(id, extension.id, "Expected extension");
+      resolve();
+    });
+  });
+
+  let uninstalledPromise = extension.addon.uninstall();
+  let uninstalled = false;
+  uninstalledPromise.then(() => { uninstalled = true; });
+
+  await uninstallingPromise;
+  Assert.ok(!uninstalled, "Uninstall should not be finished yet");
+  Assert.ok(!initialized, "Search service should still be uninitialized");
+  await Services.search.init();
+  Assert.ok(initialized, "Search service should be initialized");
+  // After initializing the search service, uninstall should eventually finish.
+  await uninstalledPromise;
+
+  await AddonTestUtils.promiseShutdownManager();
+});

browser/components/extensions/test/xpcshell/xpcshell-common.ini

@@ -10,6 +10,7 @@
 [test_ext_history.js]
 [test_ext_settings_overrides_search.js]
 [test_ext_settings_overrides_search_mozParam.js]
+[test_ext_settings_overrides_shutdown.js]
 [test_ext_url_overrides_newtab.js]
 [test_ext_url_overrides_newtab_update.js]