Backed out changeset 52747743fe65 (bug 1525729) for XPCShell failures on test_ext_chrome_settings_overrides_update.js . CLOSED TREE
authorNarcis Beleuzu <nbeleuzu@mozilla.com>
Thu, 28 Feb 2019 22:59:55 +0200
changeset 519680 008cdc952e8c48e4af1536fee05919970b1ce426
parent 519679 e6ff2537e53c4bd7dc6578df6d803e748e0d8262
child 519681 ca1739a4ed1d6d83301dca93d9397943b62444ce
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1525729
milestone67.0a1
backs out52747743fe65e7db44792f77f02aecd40634f931
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 52747743fe65 (bug 1525729) for XPCShell failures on test_ext_chrome_settings_overrides_update.js . CLOSED TREE
browser/components/extensions/parent/ext-browser.js
browser/components/extensions/parent/ext-chrome-settings-overrides.js
browser/components/extensions/test/xpcshell/test_ext_settings_overrides_shutdown.js
browser/components/extensions/test/xpcshell/xpcshell-common.ini
--- a/browser/components/extensions/parent/ext-browser.js
+++ b/browser/components/extensions/parent/ext-browser.js
@@ -239,18 +239,18 @@ global.TabContext = class extends EventE
     windowTracker.removeListener("progress", this);
     windowTracker.removeListener("TabSelect", this);
     tabTracker.off("tab-adopted", this.tabAdopted);
   }
 };
 
 // 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. That might never happen,
-// e.g. if the application shuts down before the search service initializes.
+// 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.
 XPCOMUtils.defineLazyGetter(global, "searchInitialized", () => {
   if (Services.search.isInitialized) {
     return Promise.resolve();
   }
   return ExtensionUtils.promiseObserved("browser-search-service", (_, data) => data == "init-complete");
 });
 
 class WindowTracker extends WindowTrackerBase {
--- a/browser/components/extensions/parent/ext-chrome-settings-overrides.js
+++ b/browser/components/extensions/parent/ext-chrome-settings-overrides.js
@@ -74,23 +74,16 @@ async function handleInitialHomepagePopu
     if (currentUrl == homepageUrl && gBrowser.selectedTab == tab) {
       homepagePopup.open();
       return;
     }
   }
   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();
     let item = ExtensionSettingsStore.getSetting(DEFAULT_SEARCH_STORE_TYPE, DEFAULT_SEARCH_SETTING_NAME);
     if (!item) {
       return;
     }
     if (Services.search.defaultEngine.name != item.value &&
@@ -133,21 +126,17 @@ this.chrome_settings_overrides = class e
 
   static removeSearchSettings(id) {
     return Promise.all([
       this.processDefaultSearchSetting("removeSetting", id),
       this.removeEngine(id),
     ]);
   }
 
-  static async onUninstall(id) {
-    let searchStartupPromise = pendingSearchSetupTasks.get(id);
-    if (searchStartupPromise) {
-      await searchStartupPromise;
-    }
+  static onUninstall(id) {
     // Note: We do not have to deal with homepage here as it is managed by
     // the ExtensionPreferencesManager.
     return Promise.all([
       this.removeSearchSettings(id),
       homepagePopup.clearConfirmation(id),
     ]);
   }
 
@@ -196,100 +185,78 @@ this.chrome_settings_overrides = class e
         close: () => {
           if (extension.shutdownReason == "ADDON_DISABLE") {
             homepagePopup.clearConfirmation(extension.id);
           }
         },
       });
     }
     if (manifest.chrome_settings_overrides.search_provider) {
-      // 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);
+      await searchInitialized;
+      extension.callOnClose({
+        close: () => {
+          if (extension.shutdownReason == "ADDON_DISABLE") {
+            chrome_settings_overrides.processDefaultSearchSetting("disable", extension.id);
+            chrome_settings_overrides.removeEngine(extension.id);
           }
-        });
-
-      // 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 searchProvider = manifest.chrome_settings_overrides.search_provider;
+      let engineName = searchProvider.name.trim();
+      if (searchProvider.is_default) {
         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 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);
+                  }
+                },
               },
-            },
-          };
-          Services.obs.notifyObservers(subject, "webextension-defaultsearch-prompt");
+            };
+            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);
         }
-      } 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);
       }
-    } 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);
     }
   }
 
   async setDefault(engineName) {
     let {extension} = this;
     if (extension.startupReason === "ADDON_INSTALL") {
       let defaultEngine = await Services.search.getDefault();
       let item = await ExtensionSettingsStore.addSetting(
deleted file mode 100644
--- a/browser/components/extensions/test/xpcshell/test_ext_settings_overrides_shutdown.js
+++ /dev/null
@@ -1,71 +0,0 @@
-/* -*- 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();
-});
--- a/browser/components/extensions/test/xpcshell/xpcshell-common.ini
+++ b/browser/components/extensions/test/xpcshell/xpcshell-common.ini
@@ -5,12 +5,11 @@
 [test_ext_browsingData_passwords.js]
 [test_ext_browsingData_settings.js]
 [test_ext_chrome_settings_overrides_update.js]
 [test_ext_distribution_popup.js]
 [test_ext_geckoProfiler_control.js]
 [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]