Bug 1556789 - Refactor extension install in searchservice to use promises r=robwu,daleharvey

This provides a set of promises that the searchservice resolves once the search engine has been configured

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

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

browser/base/content/test/performance/browser_startup_mainthreadio.js

@@ -587,7 +587,7 @@ const startupPhases = {
       // bug 1543090
       path: "XCurProcD:omni.ja",
       condition: WIN,
-      stat: 2,
+      stat: 8, // search engine loading can cause extra stats on win
     },
   ],
 
@@ -691,6 +691,7 @@ const startupPhases = {
     {
       // bug 1543090
       path: "XCurProcD:omni.ja",
+      ignoreIfUnused: true,
       condition: WIN,
       stat: 7,
     },

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

@@ -310,7 +310,12 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
     let { extension } = this;
     let { manifest } = extension;
     let searchProvider = manifest.chrome_settings_overrides.search_provider;
-    if (searchProvider.is_default) {
+    let handleIsDefault =
+      searchProvider.is_default && !extension.addonData.builtIn;
+    let engineName = searchProvider.name.trim();
+    // Builtin extensions are never marked with is_default.  We can safely wait on
+    // the search service to fully initialize before handling these extensions.
+    if (handleIsDefault) {
       await searchInitialized;
       if (!this.extension) {
         Cu.reportError(
@@ -318,10 +323,6 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
         );
         return;
       }
-    }
-
-    let engineName = searchProvider.name.trim();
-    if (searchProvider.is_default) {
       let engine = Services.search.getEngineByName(engineName);
       let defaultEngines = await Services.search.getDefaultEngines();
       if (
@@ -336,7 +337,7 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
       }
     }
     await this.addSearchEngine();
-    if (searchProvider.is_default) {
+    if (handleIsDefault) {
       if (extension.startupReason === "ADDON_INSTALL") {
         // Don't ask if it already the current engine
         let engine = Services.search.getEngineByName(engineName);
@@ -417,29 +418,10 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
 
   async addSearchEngine() {
     let { extension } = this;
-    let isCurrent = false;
-    let index = -1;
-    if (
-      extension.startupReason === "ADDON_UPGRADE" &&
-      !extension.addonData.builtIn
-    ) {
-      let engines = await Services.search.getEnginesByExtensionID(extension.id);
-      if (engines.length > 0) {
-        let firstEngine = engines[0];
-        let firstEngineName = firstEngine.name;
-        // There can be only one engine right now
-        isCurrent =
-          (await Services.search.getDefault()).name == firstEngineName;
-        // Get position of engine and store it
-        index = (await Services.search.getEngines())
-          .map(engine => engine.name)
-          .indexOf(firstEngineName);
-        await Services.search.removeEngine(firstEngine);
-      }
-    }
     try {
+      // This is safe to await prior to SearchService.init completing.
       let engines = await Services.search.addEnginesFromExtension(extension);
-      if (engines.length > 0) {
+      if (engines[0]) {
         await ExtensionSettingsStore.addSetting(
           extension.id,
           DEFAULT_SEARCH_STORE_TYPE,
@@ -447,26 +429,9 @@ this.chrome_settings_overrides = class extends ExtensionAPI {
           engines[0].name
         );
       }
-      if (
-        extension.startupReason === "ADDON_UPGRADE" &&
-        !extension.addonData.builtIn
-      ) {
-        let engines = await Services.search.getEnginesByExtensionID(
-          extension.id
-        );
-        let engine = Services.search.getEngineByName(engines[0].name);
-        if (isCurrent) {
-          await Services.search.setDefault(engine);
-        }
-        if (index != -1) {
-          await Services.search.moveEngine(engine, index);
-        }
-      }
     } catch (e) {
       Cu.reportError(e);
-      return false;
     }
-    return true;
   }
 };
 

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

@@ -20,6 +20,12 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   TestUtils: "resource://testing-common/TestUtils.jsm",
 });
 
+// For search related tests, reduce what is happening.  Search tests cover
+// these otherwise.
+Services.prefs.setCharPref("browser.search.region", "US");
+Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
 Services.prefs.setBoolPref("extensions.webextensions.remote", false);
 
 ExtensionTestUtils.init(this);

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

@@ -18,7 +18,12 @@ AddonTestUtils.createAppInfo(
 );
 
 add_task(async function setup() {
+  Services.prefs.setCharPref("browser.search.region", "US");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
   await AddonTestUtils.promiseStartupManager();
+  await Services.search.init();
 });
 
 add_task(async function test_overrides_update_removal() {

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

@@ -30,6 +30,7 @@ add_task(async function startup() {
   Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
   Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
   await AddonTestUtils.promiseStartupManager();
+  await Services.search.init(true);
 
   // Add a test engine and make it default so that when we do searches below,
   // Firefox doesn't try to include search suggestions from the actual default

browser/components/preferences/in-content/tests/browser.ini

@@ -2,6 +2,9 @@
 prefs =
   extensions.formautofill.available='on'
   extensions.formautofill.creditCards.available=true
+# turn off geo updates for search related tests
+  browser.search.region=US
+  browser.search.geoSpecificDefaults=false
 support-files =
   head.js
   privacypane_tests_perwindow.js

browser/components/preferences/in-content/tests/browser_engines.js

@@ -1,5 +1,8 @@
 // Test Engine list
 add_task(async function() {
+  // running stand-alone, be sure to wait for init
+  await Services.search.init();
+
   let prefs = await openPreferencesViaOpenPreferencesAPI("search", {
     leaveOpen: true,
   });

browser/components/search/test/browser/browser_contextmenu.js

@@ -4,6 +4,10 @@
  * Test searching for the selected text using the context menu
  */
 
+const { SearchExtensionLoader } = ChromeUtils.import(
+  "resource://gre/modules/SearchUtils.jsm"
+);
+
 const ENGINE_NAME = "mozSearch";
 const ENGINE_ID = "mozsearch-engine@search.mozilla.org";
 
@@ -28,7 +32,7 @@ add_task(async function() {
     Services.io.newURI("file://" + searchExtensions.path)
   );
 
-  await Services.search.ensureBuiltinExtension(ENGINE_ID);
+  await SearchExtensionLoader.installAddons([ENGINE_ID]);
 
   let engine = await Services.search.getEngineByName(ENGINE_NAME);
   ok(engine, "Got a search engine");

browser/components/search/test/marionette/test_engines_on_restart.py

@@ -15,7 +15,8 @@ class TestEnginesOnRestart(MarionetteTestCase):
         super(TestEnginesOnRestart, self).setUp()
         self.marionette.enforce_gecko_prefs({
             'browser.search.log': True,
-            'browser.search.geoSpecificDefaults': False
+            'browser.search.geoSpecificDefaults': False,
+            'browser.search.addonLoadTimeout': 0
         })
 
     def get_default_search_engine(self):

browser/components/tests/unit/test_distribution.js

@@ -280,6 +280,11 @@ add_task(async function() {
     "de-DE"
   );
 
+  // Turn off region updates and timeouts for search service
+  Services.prefs.setCharPref("browser.search.region", "DE");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
   await Services.search.init();
   var engine = Services.search.getEngineByName("Google");
   Assert.equal(engine.description, "override-de-DE");

browser/components/urlbar/tests/unit/head.js

@@ -35,6 +35,11 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 });
 const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 
+// Turn off region updates and timeouts for search service
+Services.prefs.setCharPref("browser.search.region", "US");
+Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
 /**
  * @param {string} searchString The search string to insert into the context.
  * @param {object} properties Overrides for the default values.
@@ -194,6 +199,9 @@ async function addTestEngine(basename, httpServer = undefined) {
 }
 
 /**
+ * WARNING: use of this function may result in intermittent failures when tests
+ * run in parallel due to reliance on port 9000.  Duplicated in/from unifiedcomplete.
+ *
  * Sets up a search engine that provides some suggestions by appending strings
  * onto the search query.
  *

testing/firefox-ui/tests/functional/private_browsing/test_about_private_browsing.py

@@ -16,6 +16,10 @@ class TestAboutPrivateBrowsingWithSearch(PuppeteerMixin, MarionetteTestCase):
 
         # Use a fake local support URL
         support_url = 'about:blank?'
+        self.marionette.enforce_gecko_prefs({
+            'browser.search.geoSpecificDefaults': False,
+            'browser.search.addonLoadTimeout': 0
+        })
         self.marionette.set_pref('app.support.baseURL', support_url)
         self.pb_url = support_url + 'private-browsing-myths'
 

testing/profiles/common/user.js

@@ -24,6 +24,10 @@ user_pref("browser.pagethumbnails.capturing_disabled", true);
 user_pref("browser.search.region", "US");
 // This will prevent HTTP requests for region defaults.
 user_pref("browser.search.geoSpecificDefaults", false);
+// Debug builds will timeout on the failsafe timeout for search init,
+// we just turn off the load timeout for tests in general.
+user_pref("browser.search.addonLoadTimeout", 0);
+
 // Disable webapp updates.  Yes, it is supposed to be an integer.
 user_pref("browser.webapps.checkForUpdates", 0);
 // We do not wish to display datareporting policy notifications as it might

toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js

@@ -74,7 +74,14 @@ AddonTestUtils.createAppInfo(
 );
 
 add_task(async function setup() {
+  // Tell the search service we are running in the US.  This also has the
+  // desired side-effect of preventing our geoip lookup.
+  Services.prefs.setCharPref("browser.search.region", "US");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
   await AddonTestUtils.promiseStartupManager();
+  await Services.search.init();
 });
 
 async function cleanup() {
@@ -560,6 +567,9 @@ function addTestEngine(basename, httpServer = undefined) {
 }
 
 /**
+ * WARNING: use of this function may result in intermittent failures when tests
+ * run in parallel due to reliance on port 9000.
+ *
  * Sets up a search engine that provides some suggestions by appending strings
  * onto the search query.
  *
@@ -606,6 +616,7 @@ add_task(async function ensure_search_engine() {
   await Services.search.addEngineWithDetails("MozSearch", {
     method: "GET",
     template: "http://s.example.com/search",
+    isBuiltin: true,
   });
   let engine = Services.search.getEngineByName("MozSearch");
   await Services.search.setDefault(engine);

toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js

@@ -6,12 +6,14 @@ const { PlacesSearchAutocompleteProvider } = ChromeUtils.import(
   "resource://gre/modules/PlacesSearchAutocompleteProvider.jsm"
 );
 
-add_task(async function() {
+add_task(async function setup() {
-  await Services.search.init();
   // Tell the search service we are running in the US.  This also has the
   // desired side-effect of preventing our geoip lookup.
   Services.prefs.setCharPref("browser.search.region", "US");
   Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
+  await Services.search.init();
 
   Services.search.restoreDefaultEngines();
   Services.search.resetToOriginalDefaultEngine();
@@ -38,9 +40,9 @@ add_task(async function hide_search_engine_nomatch() {
   let engine = await Services.search.getDefault();
   let domain = engine.getResultDomain();
   let token = domain.substr(0, 1);
-  let promiseTopic = promiseSearchTopic("engine-changed");
+  let promiseTopic = promiseSearchTopic("engine-removed");
   await Promise.all([Services.search.removeEngine(engine), promiseTopic]);
-  Assert.ok(engine.hidden);
+  Assert.ok(engine.hidden, "engine was hidden rather than removed");
   let matchedEngine = await PlacesSearchAutocompleteProvider.engineForDomainPrefix(
     token
   );
@@ -163,7 +165,11 @@ add_task(async function test_parseSubmissionURL_basic() {
   let result = PlacesSearchAutocompleteProvider.parseSubmissionURL(
     submissionURL
   );
-  Assert.equal(result.engineName, engine.name);
+  Assert.equal(
+    result.engineName,
+    engine.name,
+    "parsed submissionURL has matching engine name"
+  );
   Assert.equal(result.terms, "terms");
 
   result = PlacesSearchAutocompleteProvider.parseSubmissionURL(
@@ -174,8 +180,8 @@ add_task(async function test_parseSubmissionURL_basic() {
 
 add_task(async function test_builtin_aliased_search_engine_match() {
   let engine = await PlacesSearchAutocompleteProvider.engineForAlias("@google");
-  Assert.ok(engine);
+  Assert.ok(engine, "matched an engine with an alias");
-  Assert.equal(engine.name, "Google");
+  Assert.equal(engine.name, "Google", "correct engine for alias");
   let promiseTopic = promiseSearchTopic("engine-changed");
   await Promise.all([Services.search.removeEngine(engine), promiseTopic]);
   let matchedEngine = await PlacesSearchAutocompleteProvider.engineForAlias(
@@ -187,7 +193,7 @@ add_task(async function test_builtin_aliased_search_engine_match() {
     PlacesSearchAutocompleteProvider.engineForAlias("@google")
   );
   engine = await PlacesSearchAutocompleteProvider.engineForAlias("@google");
-  Assert.ok(engine);
+  Assert.ok(engine, "matched an engine with an alias");
 });
 
 function promiseSearchTopic(expectedVerb) {

toolkit/components/places/tests/unifiedcomplete/xpcshell.ini

@@ -43,14 +43,18 @@ skip-if = appname == "thunderbird"
 [test_query_url.js]
 [test_remote_tab_matches.js]
 skip-if = !sync
-[test_search_engine_alias.js]
 [test_search_engine_default.js]
 [test_search_engine_host.js]
 [test_search_engine_restyle.js]
 [test_search_suggestions.js]
-[test_special_search.js]
 [test_swap_protocol.js]
 [test_tab_matches.js]
 [test_trimming.js]
 [test_visit_url.js]
 [test_word_boundary_search.js]
+# The following tests use addTestSuggestionsEngine which doesn't
+# play well when run in parallel.
+[test_search_engine_alias.js]
+run-sequentially = Test relies on port 9000, fails intermittently
+[test_special_search.js]
+run-sequentially = Test relies on port 9000, may fail intermittently

toolkit/components/places/tests/unit/head_bookmarks.js

@@ -18,6 +18,11 @@ const { AddonTestUtils } = ChromeUtils.import(
   "resource://testing-common/AddonTestUtils.jsm"
 );
 
+// Turn off region updates and timeouts for search service
+Services.prefs.setCharPref("browser.search.region", "US");
+Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
 AddonTestUtils.init(this, false);
 AddonTestUtils.overrideCertDB();
 AddonTestUtils.createAppInfo(

toolkit/components/search/SearchEngine.jsm

@@ -48,6 +48,7 @@ const MOZSEARCH_NS_10 = "http://www.mozilla.org/2006/browser/search/";
 const MOZSEARCH_LOCALNAME = "SearchPlugin";
 
 const USER_DEFINED = "searchTerms";
+const SEARCH_TERM_PARAM = "{searchTerms}";
 
 // Custom search parameters
 const MOZ_PARAM_LOCALE = "moz:locale";
@@ -580,8 +581,18 @@ EngineURL.prototype = {
   },
 
   _getTermsParameterName() {
-    let queryParam = this.params.find(p => p.value == "{" + USER_DEFINED + "}");
+    if (this.params.length > 0) {
-    return queryParam ? queryParam.name : "";
+      let queryParam = this.params.find(p => p.value == SEARCH_TERM_PARAM);
+      return queryParam ? queryParam.name : "";
+    }
+    // If an engine only used template, then params is empty, fall back to checking the template.
+    let params = new URL(this.template).searchParams;
+    for (let [name, value] of params.entries()) {
+      if (value == SEARCH_TERM_PARAM) {
+        return name;
+      }
+    }
+    return "";
   },
 
   _hasRelation(rel) {
@@ -814,6 +825,8 @@ SearchEngine.prototype = {
   _iconUpdateURL: null,
   /* The extension ID if added by an extension. */
   _extensionID: null,
+  /* The extension version if added by an extension. */
+  _version: null,
   // Built in search engine extensions.
   _isBuiltin: false,
 
@@ -1403,6 +1416,7 @@ SearchEngine.prototype = {
    */
   _initFromMetadata(engineName, params) {
     this._extensionID = params.extensionID;
+    this._version = params.version;
     this._isBuiltin = !!params.isBuiltin;
 
     this._initEngineURLFromMetaData(SearchUtils.URL_TYPE.SEARCH, {
@@ -1684,6 +1698,9 @@ SearchEngine.prototype = {
     if (json.extensionID) {
       this._extensionID = json.extensionID;
     }
+    if (json.version) {
+      this._version = json.version;
+    }
     for (let i = 0; i < json._urls.length; ++i) {
       let url = json._urls[i];
       let engineURL = new EngineURL(
@@ -1742,6 +1759,9 @@ SearchEngine.prototype = {
     if (this._extensionID) {
       json.extensionID = this._extensionID;
     }
+    if (this._version) {
+      json.version = this._version;
+    }
 
     return json;
   },

toolkit/components/search/SearchService.jsm

@@ -13,7 +13,6 @@ const { PromiseUtils } = ChromeUtils.import(
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
-  AddonManager: "resource://gre/modules/AddonManager.jsm",
   clearTimeout: "resource://gre/modules/Timer.jsm",
   DeferredTask: "resource://gre/modules/DeferredTask.jsm",
   ExtensionParent: "resource://gre/modules/ExtensionParent.jsm",
@@ -22,6 +21,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   RemoteSettings: "resource://services-settings/remote-settings.js",
   RemoteSettingsClient: "resource://services-settings/RemoteSettingsClient.jsm",
   SearchEngine: "resource://gre/modules/SearchEngine.jsm",
+  SearchExtensionLoader: "resource://gre/modules/SearchUtils.jsm",
   SearchStaticData: "resource://gre/modules/SearchStaticData.jsm",
   SearchUtils: "resource://gre/modules/SearchUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
@@ -53,14 +53,6 @@ XPCOMUtils.defineLazyGetter(this, "gEncoder", function() {
 // Directory service keys
 const NS_APP_DISTRIBUTION_SEARCH_DIR_LIST = "SrchPluginsDistDL";
 
-// We load plugins from EXT_SEARCH_PREFIX, where a list.json
-// file needs to exist to list available engines.
-const EXT_SEARCH_PREFIX = "resource://search-extensions/";
-const APP_SEARCH_PREFIX = "resource://search-plugins/";
-
-// The address we use to sign the built in search extensions with.
-const EXT_SIGNING_ADDRESS = "search.mozilla.org";
-
 const TOPIC_LOCALES_CHANGE = "intl:app-locales-changed";
 const QUIT_APPLICATION_TOPIC = "quit-application";
 
@@ -267,11 +259,12 @@ function fetchRegion(ss) {
   let endpoint = Services.urlFormatter.formatURLPref(
     "browser.search.geoip.url"
   );
-  SearchUtils.log("_fetchRegion starting with endpoint " + endpoint);
   // As an escape hatch, no endpoint means no geoip.
   if (!endpoint) {
     return Promise.resolve();
   }
+  SearchUtils.log("_fetchRegion starting with endpoint " + endpoint);
+
   let startTime = Date.now();
   return new Promise(resolve => {
     // Instead of using a timeout on the xhr object itself, we simulate one
@@ -569,6 +562,9 @@ const gEmptyParseSubmissionResult = Object.freeze(
  */
 function SearchService() {
   this._initObservers = PromiseUtils.defer();
+  // This deferred promise is resolved once a set of engines have been
+  // parsed out of list.json, which happens in _loadEngines.
+  this._extensionLoadReady = PromiseUtils.defer();
 }
 
 SearchService.prototype = {
@@ -640,6 +636,7 @@ SearchService.prototype = {
   async _init(skipRegionCheck) {
     SearchUtils.log("_init start");
 
+    TelemetryStopwatch.start("SEARCH_SERVICE_INIT_MS");
     try {
       // See if we have a cache file so we don't have to parse a bunch of XML.
       let cache = await this._readCacheFile();
@@ -665,15 +662,19 @@ SearchService.prototype = {
       this._buildCache();
       this._addObservers();
     } catch (ex) {
-      this._initRV = ex.result !== undefined ? ex.result : Cr.NS_ERROR_FAILURE;
+      // If loadEngines has a rejected promise chain, ex is undefined.
+      this._initRV =
+        ex && ex.result !== undefined ? ex.result : Cr.NS_ERROR_FAILURE;
       SearchUtils.log(
-        "_init: failure initializng search: " + ex + "\n" + ex.stack
+        "_init: failure initializing search: " + ex + "\n" + (ex && ex.stack)
       );
     }
     gInitialized = true;
     if (Components.isSuccessCode(this._initRV)) {
+      TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
       this._initObservers.resolve(this._initRV);
     } else {
+      TelemetryStopwatch.cancel("SEARCH_SERVICE_INIT_MS");
       this._initObservers.reject(this._initRV);
     }
     Services.obs.notifyObservers(
@@ -683,7 +684,6 @@ SearchService.prototype = {
     );
 
     SearchUtils.log("_init: Completed _init");
-    return this._initRV;
   },
 
   /**
@@ -847,20 +847,16 @@ SearchService.prototype = {
     return val;
   },
 
-  _listJSONURL:
+  // Some tests need to modify this url, they can do so through SearchUtils.
-    (AppConstants.platform == "android"
+  get _listJSONURL() {
-      ? APP_SEARCH_PREFIX
+    return SearchUtils.LIST_JSON_URL;
-      : EXT_SEARCH_PREFIX) + "list.json",
+  },
 
   _engines: {},
   __sortedEngines: null,
   _visibleDefaultEngines: [],
   _searchDefault: null,
   _searchOrder: [],
-  // A Set of installed search extensions reported by AddonManager
-  // startup before SearchSevice has started. Will be installed
-  // during init().
-  _startupExtensions: new Set(),
 
   get _sortedEngines() {
     if (!this.__sortedEngines) {
@@ -1026,12 +1022,15 @@ SearchService.prototype = {
         this._visibleDefaultEngines.length ||
       this._visibleDefaultEngines.some(notInCacheVisibleEngines);
 
+    this._engineLocales = this._enginesToLocales(engines);
+    this._extensionLoadReady.resolve();
+
     if (!rebuildCache) {
       SearchUtils.log("_loadEngines: loading from cache directories");
       this._loadEnginesFromCache(cache);
       if (Object.keys(this._engines).length) {
         SearchUtils.log("_loadEngines: done using existing cache");
-        return;
+        return Promise.resolve();
       }
       SearchUtils.log(
         "_loadEngines: No valid engines found in cache. Loading engines from disk."
@@ -1049,19 +1048,7 @@ SearchService.prototype = {
       let enginesFromURLs = await this._loadFromChromeURLs(engines, isReload);
       enginesFromURLs.forEach(this._addEngineToStore, this);
     } else {
-      let engineList = this._enginesToLocales(engines);
+      return SearchExtensionLoader.installAddons(this._engineLocales.keys());
-      for (let [id, locales] of engineList) {
-        await this.ensureBuiltinExtension(id, locales);
-      }
-
-      SearchUtils.log(
-        "_loadEngines: loading " +
-          this._startupExtensions.size +
-          " engines reported by AddonManager startup"
-      );
-      for (let extension of this._startupExtensions) {
-        await this._installExtensionEngine(extension, [DEFAULT_TAG], true);
-      }
     }
 
     SearchUtils.log(
@@ -1072,39 +1059,7 @@ SearchService.prototype = {
     this._loadEnginesMetadataFromCache(cache);
 
     SearchUtils.log("_loadEngines: done using rebuilt cache");
-  },
+    return Promise.resolve();
-
-  /**
-   * Ensures a built in search WebExtension is installed, installing
-   * it if necessary.
-   *
-   * @param {string} id
-   *   The WebExtension ID.
-   * @param {Array<string>} locales
-   *   An array of locales to use for the WebExtension. If more than
-   *   one is specified, different versions of the same engine may
-   *   be installed.
-   */
-  async ensureBuiltinExtension(id, locales = [DEFAULT_TAG]) {
-    SearchUtils.log("ensureBuiltinExtension: " + id);
-    try {
-      let policy = WebExtensionPolicy.getByID(id);
-      if (!policy) {
-        SearchUtils.log("ensureBuiltinExtension: Installing " + id);
-        let path = EXT_SEARCH_PREFIX + id.split("@")[0] + "/";
-        await AddonManager.installBuiltinAddon(path);
-        policy = WebExtensionPolicy.getByID(id);
-      }
-      // On startup the extension may have not finished parsing the
-      // manifest, wait for that here.
-      await policy.readyPromise;
-      await this._installExtensionEngine(policy.extension, locales);
-      SearchUtils.log("ensureBuiltinExtension: " + id + " installed.");
-    } catch (err) {
-      Cu.reportError(
-        "Failed to install engine: " + err.message + "\n" + err.stack
-      );
-    }
   },
 
   /**
@@ -1113,13 +1068,13 @@ SearchService.prototype = {
    *
    * @param {array} engines
    *   An array of engines
-   * @returns {Map} A Map of extension names + locales.
+   * @returns {Map} A Map of extension IDs to locales.
    */
   _enginesToLocales(engines) {
     let engineLocales = new Map();
     for (let engine of engines) {
       let [extensionName, locale] = this._parseEngineName(engine);
-      let id = extensionName + "@" + EXT_SIGNING_ADDRESS;
+      let id = SearchUtils.makeExtensionId(extensionName);
       let locales = engineLocales.get(id) || new Set();
       locales.add(locale);
       engineLocales.set(id, locales);
@@ -1202,9 +1157,14 @@ SearchService.prototype = {
     // Start by clearing the initialized state, so we don't abort early.
     gInitialized = false;
 
+    // Reset any init promises synchronously before the async init below.
+    this._initObservers = PromiseUtils.defer();
+    this._extensionLoadReady = PromiseUtils.defer();
+    // If reset is called prior to reinit, be sure to mark init as started.
+    this._initStarted = true;
+
     (async () => {
       try {
-        this._initObservers = PromiseUtils.defer();
         if (this._batchTask) {
           SearchUtils.log("finalizing batch task");
           let task = this._batchTask;
@@ -1226,6 +1186,7 @@ SearchService.prototype = {
         this._searchDefault = null;
         this._searchOrder = [];
         this._metaData = {};
+        this._engineLocales = null;
 
         // Tests that want to force a synchronous re-initialization need to
         // be notified when we are done uninitializing.
@@ -1270,6 +1231,7 @@ SearchService.prototype = {
           SearchUtils.TOPIC_SEARCH_SERVICE,
           "reinit-failed"
         );
+        this._initObservers.reject();
       } finally {
         gReinitializing = false;
         Services.obs.notifyObservers(
@@ -1293,6 +1255,8 @@ SearchService.prototype = {
     this._visibleDefaultEngines = [];
     this._searchOrder = [];
     this._metaData = {};
+    this._extensionLoadReady = PromiseUtils.defer();
+    this._engineLocales = null;
   },
 
   /**
@@ -1347,21 +1311,31 @@ SearchService.prototype = {
       return;
     }
 
-    SearchUtils.log('_addEngineToStore: Adding engine: "' + engine.name + '"');
-
     // See if there is an existing engine with the same name. However, if this
     // engine is updating another engine, it's allowed to have the same name.
-    var hasSameNameAsUpdate =
+    var matchingEngineUpdate =
-      engine._engineToUpdate && engine.name == engine._engineToUpdate.name;
+      engine._engineToUpdate &&
-    if (engine.name in this._engines && !hasSameNameAsUpdate) {
+      (engine.name == engine._engineToUpdate.name ||
+        (engine._extensionID &&
+          engine._extensionID == engine._engineToUpdate._extensionID));
+    if (engine.name in this._engines && !matchingEngineUpdate) {
       SearchUtils.log("_addEngineToStore: Duplicate engine found, aborting!");
       return;
     }
 
     if (engine._engineToUpdate) {
+      SearchUtils.log(
+        '_addEngineToStore: Updating engine: "' + engine.name + '"'
+      );
       // We need to replace engineToUpdate with the engine that just loaded.
       var oldEngine = engine._engineToUpdate;
 
+      let index = -1;
+      if (this.__sortedEngines) {
+        index = this.__sortedEngines.indexOf(oldEngine);
+      }
+      let isCurrent = this._currentEngine == oldEngine;
+
       // Remove the old engine from the hash, since it's keyed by name, and our
       // name might change (the update might have a new name).
       delete this._engines[oldEngine.name];
@@ -1380,8 +1354,18 @@ SearchService.prototype = {
 
       // Add the engine back
       this._engines[engine.name] = engine;
+      if (index >= 0) {
+        this.__sortedEngines[index] = engine;
+        this._saveSortedEngineList();
+      }
+      if (isCurrent) {
+        this._currentEngine = engine;
+      }
       SearchUtils.notifyAction(engine, SearchUtils.MODIFIED_TYPE.CHANGED);
     } else {
+      SearchUtils.log(
+        '_addEngineToStore: Adding engine: "' + engine.name + '"'
+      );
       // Not an update, just add the new engine.
       this._engines[engine.name] = engine;
       // Only add the engine to the list of sorted engines if the initial list
@@ -1534,7 +1518,9 @@ SearchService.prototype = {
         SearchUtils.log(
           "_loadFromChromeURLs: loading engine from chrome url: " + url
         );
-        let uri = Services.io.newURI(APP_SEARCH_PREFIX + url + ".xml");
+        let uri = Services.io.newURI(
+          SearchUtils.APP_SEARCH_PREFIX + url + ".xml"
+        );
         let engine = new SearchEngine({
           uri,
           readOnly: true,
@@ -1575,10 +1561,10 @@ SearchService.prototype = {
     let request = new XMLHttpRequest();
     request.overrideMimeType("text/plain");
     let list = await new Promise(resolve => {
-      request.onload = function(event) {
+      request.onload = event => {
         resolve(event.target.responseText);
       };
-      request.onerror = function(event) {
+      request.onerror = event => {
         SearchUtils.log("_findEngines: failed to read " + this._listJSONURL);
         resolve();
       };
@@ -1586,7 +1572,7 @@ SearchService.prototype = {
       request.send();
     });
 
-    return this._parseListJSON(list);
+    return list !== undefined ? this._parseListJSON(list) : [];
   },
 
   _parseListJSON(list) {
@@ -1742,7 +1728,7 @@ SearchService.prototype = {
     }
 
     if (!this._searchDefault) {
-      Cu.reportError("parseListJSON: No searchDefault");
+      SearchUtils.log("parseListJSON: No searchDefault");
     }
 
     if (
@@ -1925,38 +1911,18 @@ SearchService.prototype = {
 
   // nsISearchService
   async init(skipRegionCheck = false) {
-    SearchUtils.log("SearchService.init");
     if (this._initStarted) {
       if (!skipRegionCheck) {
         await this._ensureKnownRegionPromise;
       }
       return this._initObservers.promise;
     }
-
-    TelemetryStopwatch.start("SEARCH_SERVICE_INIT_MS");
     this._initStarted = true;
-    try {
+    SearchUtils.log("SearchService.init");
-      // Complete initialization by calling asynchronous initializer.
+
-      await this._init(skipRegionCheck);
+    // Don't await on _init, _initObservers is resolved or rejected in _init.
-      TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
+    this._init(skipRegionCheck);
-    } catch (ex) {
+    return this._initObservers.promise;
-      if (ex.result == Cr.NS_ERROR_ALREADY_INITIALIZED) {
-        // No need to pursue asynchronous because synchronous fallback was
-        // called and has finished.
-        TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
-      } else {
-        this._initObservers.reject(ex.result);
-        TelemetryStopwatch.cancel("SEARCH_SERVICE_INIT_MS");
-        throw ex;
-      }
-    }
-    if (!Components.isSuccessCode(this._initRV)) {
-      throw Components.Exception(
-        "SearchService initialization failed",
-        this._initRV
-      );
-    }
-    return this._initRV;
   },
 
   get isInitialized() {
@@ -2083,17 +2049,17 @@ SearchService.prototype = {
   },
 
   async addEngineWithDetails(name, details) {
-    SearchUtils.log('addEngineWithDetails: Adding "' + name + '".');
+    // We only enforce init when called via the IDL API.  Internally we are adding engines
-    let isCurrent = false;
+    // during init and do not wait on this.
-    var params = details;
+    if (!gInitialized) {
-
-    let isBuiltin = !!params.isBuiltin;
-    // We install search extensions during the init phase, both built in
-    // web extensions freshly installed (via addEnginesFromExtension) or
-    // user installed extensions being reenabled calling this directly.
-    if (!gInitialized && !isBuiltin && !params.initEngine) {
       await this.init(true);
     }
+    return this._addEngineWithDetails(name, details);
+  },
+
+  async _addEngineWithDetails(name, params) {
+    SearchUtils.log('addEngineWithDetails: Adding "' + name + '".');
+
     if (!name) {
       SearchUtils.fail("Invalid name passed to addEngineWithDetails!");
     }
@@ -2102,26 +2068,47 @@ SearchService.prototype = {
     }
     let existingEngine = this._engines[name];
     if (existingEngine) {
-      if (
+      // Is this a webextension update?  If not we're dealing with legacy opensearch or an override attempt.
+      let webExtUpdate =
         params.extensionID &&
-        existingEngine._loadPath.startsWith(
+        params.extensionID === existingEngine._extensionID;
-          `jar:[profile]/extensions/${params.extensionID}`
+      if (!webExtUpdate) {
-        )
+        let webExtBuiltin = params.extensionID && params.isBuiltin;
-      ) {
+        // Is the existing engine a distribution engine?
-        // This is a legacy extension engine that needs to be migrated to WebExtensions.
+        if (
-        isCurrent = this.defaultEngine == existingEngine;
+          webExtBuiltin &&
-        await this.removeEngine(existingEngine);
+          existingEngine._loadPath.startsWith(
-      } else {
+            `[profile]/distribution/searchplugins/`
-        SearchUtils.fail(
+          )
-          "An engine with that name already exists!",
+        ) {
-          Cr.NS_ERROR_FILE_ALREADY_EXISTS
+          SearchExtensionLoader.reject(
-        );
+            params.extensionID,
+            new Error(
+              `${params.extensionID} cannot override distribution engine.`
+            )
+          );
+          return null;
+        } else if (
+          params.extensionID &&
+          existingEngine._loadPath.startsWith(
+            `jar:[profile]/extensions/${params.extensionID}`
+          )
+        ) {
+          // We uninstall the legacy engine, but we don't need to wait or do anything else here,
+          // _addEngineToStore will handle updating the engine data we're using.
+          this._removeEngineInstall(existingEngine);
+        } else {
+          SearchUtils.fail(
+            `An engine with the name ${name} already exists!`,
+            Cr.NS_ERROR_FILE_ALREADY_EXISTS
+          );
+        }
       }
     }
 
     let newEngine = new SearchEngine({
       name,
-      readOnly: isBuiltin,
+      readOnly: !!params.isBuiltin,
       sanitizeName: true,
     });
     newEngine._initFromMetadata(name, params);
@@ -2129,43 +2116,26 @@ SearchService.prototype = {
     if (params.extensionID) {
       newEngine._loadPath += ":" + params.extensionID;
     }
+    newEngine._engineToUpdate = existingEngine;
 
     this._addEngineToStore(newEngine);
-    if (isCurrent) {
-      this.defaultEngine = newEngine;
-    }
     return newEngine;
   },
 
   async addEnginesFromExtension(extension) {
     SearchUtils.log("addEnginesFromExtension: " + extension.id);
-    if (extension.addonData.builtIn) {
+    // Wait for the list.json engines to be parsed before
-      SearchUtils.log("addEnginesFromExtension: Ignoring builtIn engine.");
+    // allowing addEnginesFromExtension to continue.  This delays early start
-      return [];
+    // extensions until we are at a stage that they can be handled.
-    }
+    await this._extensionLoadReady.promise;
-    // If we havent started SearchService yet, store this extension
+    let locales = this._engineLocales.get(extension.id) || [DEFAULT_TAG];
-    // to install in SearchService.init().
-    if (!gInitialized) {
-      this._startupExtensions.add(extension);
-      return [];
-    }
-    return this._installExtensionEngine(extension, [DEFAULT_TAG]);
-  },
-
-  async _installExtensionEngine(extension, locales, initEngine) {
-    SearchUtils.log("installExtensionEngine: " + extension.id);
 
     let installLocale = async locale => {
       let manifest =
         locale === DEFAULT_TAG
           ? extension.manifest
           : await extension.getLocalizedManifest(locale);
-      return this._addEngineForManifest(
+      return this._addEngineForManifest(extension, manifest, locale);
-        extension,
-        manifest,
-        locale,
-        initEngine
-      );
     };
 
     let engines = [];
@@ -2176,17 +2146,15 @@ SearchService.prototype = {
           ":" +
           locale
       );
-      engines.push(await installLocale(locale));
+      engines.push(installLocale(locale));
     }
-    return engines;
+    return Promise.all(engines).then(installedEngines => {
+      SearchExtensionLoader.resolve(extension.id);
+      return installedEngines;
+    });
   },
 
-  async _addEngineForManifest(
+  async _addEngineForManifest(extension, manifest, locale = DEFAULT_TAG) {
-    extension,
-    manifest,
-    locale = DEFAULT_TAG,
-    initEngine = false
-  ) {
     let { IconDetails } = ExtensionParent;
 
     // General set of icons for an engine.
@@ -2241,10 +2209,10 @@ SearchService.prototype = {
       suggestGetParams: searchProvider.suggest_url_get_params,
       queryCharset: searchProvider.encoding || "UTF-8",
       mozParams: searchProvider.params,
-      initEngine,
+      version: extension.version,
     };
 
-    return this.addEngineWithDetails(params.name, params);
+    return this._addEngineWithDetails(params.name, params);
   },
 
   async addEngine(engineURL, iconURL, confirm, extensionID) {
@@ -2292,6 +2260,19 @@ SearchService.prototype = {
     }
   },
 
+  async _removeEngineInstall(engine) {
+    // Make sure there is a file and this is not a webextension.
+    if (!engine._filePath || engine._extensionID) {
+      return;
+    }
+    let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
+    file.persistentDescriptor = engine._filePath;
+    if (file.exists()) {
+      file.remove(false);
+    }
+    engine._filePath = null;
+  },
+
   async removeEngine(engine) {
     await this.init(true);
     if (!engine) {
@@ -2323,14 +2304,7 @@ SearchService.prototype = {
       engineToRemove.alias = null;
     } else {
       // Remove the engine file from disk if we had a legacy file in the profile.
-      if (engineToRemove._filePath) {
+      this._removeEngineInstall(engineToRemove);
-        let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
-        file.persistentDescriptor = engineToRemove._filePath;
-        if (file.exists()) {
-          file.remove(false);
-        }
-        engineToRemove._filePath = null;
-      }
 
       // Remove the engine from _sortedEngines
       var index = this._sortedEngines.indexOf(engineToRemove);

toolkit/components/search/SearchUtils.jsm

@@ -6,18 +6,36 @@
 
 "use strict";
 
-var EXPORTED_SYMBOLS = ["SearchUtils"];
+var EXPORTED_SYMBOLS = ["SearchUtils", "SearchExtensionLoader"];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 
 XPCOMUtils.defineLazyModuleGetters(this, {
+  AddonManager: "resource://gre/modules/AddonManager.jsm",
+  AppConstants: "resource://gre/modules/AppConstants.jsm",
+  PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
+  clearTimeout: "resource://gre/modules/Timer.jsm",
+  setTimeout: "resource://gre/modules/Timer.jsm",
 });
 
 const BROWSER_SEARCH_PREF = "browser.search.";
 
+const EXT_SEARCH_PREFIX = "resource://search-extensions/";
+const APP_SEARCH_PREFIX = "resource://search-plugins/";
+
+// By the time we start loading an extension, it should load much
+// faster than 1000ms.  This simply ensures we resolve all the
+// promises and let search init complete if something happens.
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "ADDON_LOAD_TIMEOUT",
+  BROWSER_SEARCH_PREF + "addonLoadTimeout",
+  1000
+);
+
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "loggingEnabled",
@@ -26,9 +44,14 @@ XPCOMUtils.defineLazyPreferenceGetter(
 );
 
 var SearchUtils = {
-  APP_SEARCH_PREFIX: "resource://search-plugins/",
+  APP_SEARCH_PREFIX,
 
   BROWSER_SEARCH_PREF,
+  EXT_SEARCH_PREFIX,
+  LIST_JSON_URL:
+    (AppConstants.platform == "android"
+      ? APP_SEARCH_PREFIX
+      : EXT_SEARCH_PREFIX) + "list.json",
 
   /**
    * Topic used for events involving the service itself.
@@ -95,7 +118,6 @@ var SearchUtils = {
    */
   log(text) {
     if (loggingEnabled) {
-      dump("*** Search: " + text + "\n");
       Services.console.logStringMessage(text);
     }
   },
@@ -150,4 +172,122 @@ var SearchUtils = {
 
     return null;
   },
+
+  makeExtensionId(name) {
+    return name + "@search.mozilla.org";
+  },
+
+  getExtensionUrl(id) {
+    return EXT_SEARCH_PREFIX + id.split("@")[0] + "/";
+  },
+};
+
+/**
+ * SearchExtensionLoader provides a simple install function that
+ * returns a set of promises.  The caller (SearchService) must resolve
+ * each extension id once it has handled the final part of the install
+ * (creating the SearchEngine).  Once they are resolved, the extensions
+ * are fully functional, in terms of the SearchService, and initialization
+ * can be completed.
+ *
+ * When an extension is installed (that has a search provider), the
+ * extension system will call ss.addEnginesFromExtension. When that is
+ * completed, SearchService calls back to resolve the promise.
+ */
+const SearchExtensionLoader = {
+  _promises: new Map(),
+  // strict is used in tests.
+  _strict: false,
+
+  /**
+   * Creates a deferred promise for an extension install.
+   * @param {string} id the extension id.
+   * @returns {Promise}
+   */
+  _addPromise(id) {
+    let deferred = PromiseUtils.defer();
+    // We never want to have some uncaught problem stop the SearchService
+    // init from completing, so timeout the promise.
+    if (ADDON_LOAD_TIMEOUT > 0) {
+      deferred.timeout = setTimeout(() => {
+        deferred.reject(id, new Error("addon install timed out."));
+        this._promises.delete(id);
+      }, ADDON_LOAD_TIMEOUT);
+    }
+    this._promises.set(id, deferred);
+    return deferred.promise;
+  },
+
+  /**
+   * @param {string} id the extension id to resolve.
+   */
+  resolve(id) {
+    if (this._promises.has(id)) {
+      let deferred = this._promises.get(id);
+      if (deferred.timeout) {
+        clearTimeout(deferred.timeout);
+      }
+      deferred.resolve();
+      this._promises.delete(id);
+    }
+  },
+
+  /**
+   * @param {string} id the extension id to reject.
+   * @param {object} error The error to log when rejecting.
+   */
+  reject(id, error) {
+    if (this._promises.has(id)) {
+      let deferred = this._promises.get(id);
+      if (deferred.timeout) {
+        clearTimeout(deferred.timeout);
+      }
+      // We don't want to reject here because that will reject the promise.all
+      // and stop the searchservice init.  Log the error, and resolve the promise.
+      // strict mode can be used by tests to force an exception to occur.
+      Cu.reportError(`Addon install for search engine ${id} failed: ${error}`);
+      if (this._strict) {
+        deferred.reject();
+      } else {
+        deferred.resolve();
+      }
+      this._promises.delete(id);
+    }
+  },
+
+  _reset() {
+    SearchUtils.log(`SearchExtensionLoader.reset`);
+    for (let id of this._promises.keys()) {
+      this.reject(id, new Error(`installAddons reset during install`));
+    }
+    this._promises = new Map();
+  },
+
+  /**
+   * Tell AOM to install a set of built-in extensions.  If the extension is
+   * already installed, it will be reinstalled.
+   *
+   * @param {Array} engineIDList is an array of extension IDs.
+   * @returns {Promise} resolved when all engines have finished installation.
+   */
+  async installAddons(engineIDList) {
+    SearchUtils.log(`SearchExtensionLoader.installAddons`);
+    // If SearchService calls us again, it is being re-inited.  reset ourselves.
+    this._reset();
+    let promises = [];
+    for (let id of engineIDList) {
+      promises.push(this._addPromise(id));
+      let path = SearchUtils.getExtensionUrl(id);
+      SearchUtils.log(
+        `SearchExtensionLoader.installAddons: installing ${id} at ${path}`
+      );
+      // The AddonManager will install the engine asynchronously
+      AddonManager.installBuiltinAddon(path).catch(error => {
+        // Catch any install errors and propogate.
+        this.reject(id, error);
+      });
+    }
+
+    return Promise.all(promises);
+  },
 };

toolkit/components/search/nsISearchService.idl

@@ -221,8 +221,6 @@ interface nsISearchService : nsISupports
    */
   void reInit([optional] in boolean skipRegionCheck);
   void reset();
-  Promise ensureBuiltinExtension(in AString id,
-                                [optional] in jsval locales);
 
   /**
    * Determine whether initialization has been completed.

toolkit/components/search/tests/xpcshell/data/invalid-extension/invalid/manifest.json

@@ -0,0 +1,19 @@
+{
+  "name": "Invalid",
+  "description": "Invalid Engine",
+  "manifest_version": 2,
+  "version": "1.0",
+  "applications": {
+    "gecko": {
+      "id": "invalid@search.mozilla.org"
+    }
+  },
+  "hidden": true,
+  "chrome_settings_overrides": {
+    "search_provider": {
+      "name": "Invalid",
+      "search_url": "ssh://duckduckgo.com/",
+      "suggest_url": "ssh://ac.duckduckgo.com/ac/q={searchTerms}&type=list"
+    }
+  }
+}

toolkit/components/search/tests/xpcshell/data/invalid-extension/list.json

@@ -0,0 +1,7 @@
+{
+  "default": {
+    "visibleDefaultEngines": [
+      "invalid"
+    ]
+  }
+}

toolkit/components/search/tests/xpcshell/head_search.js

@@ -41,6 +41,10 @@ var XULRuntime = Cc["@mozilla.org/xre/runtime;1"].getService(Ci.nsIXULRuntime);
 // Expand the amount of information available in error logs
 Services.prefs.setBoolPref("browser.search.log", true);
 
+// Some tests load tons of extensions and will timeout, disable the timeout
+// here to allow tests to be slow.
+Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
 // The geo-specific search tests assume certain prefs are already setup, which
 // might not be true when run in comm-central etc.  So create them here.
 Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", true);

toolkit/components/search/tests/xpcshell/test_async_distribution.js

@@ -11,19 +11,21 @@ add_task(async function test_async_distribution() {
 
   Assert.ok(!Services.search.isInitialized);
 
-  return Services.search.init().then(function search_initialized(aStatus) {
+  let aStatus = await Services.search.init();
-    Assert.ok(Components.isSuccessCode(aStatus));
+  Assert.ok(Components.isSuccessCode(aStatus));
-    Assert.ok(Services.search.isInitialized);
+  Assert.ok(Services.search.isInitialized);
 
-    // test that the engine from the distribution overrides our jar engine
+  // test that the engine from the distribution overrides our jar engine
-    return Services.search.getEngines().then(engines => {
+  let engines = await Services.search.getEngines();
-      Assert.equal(engines.length, 1);
+  Assert.equal(engines.length, 1);
 
-      let engine = Services.search.getEngineByName("bug645970");
+  let engine = Services.search.getEngineByName("bug645970");
-      Assert.notEqual(engine, null);
+  Assert.ok(!!engine, "engine is installed");
 
-      // check the engine we have is actually the one from the distribution
+  // check the engine we have is actually the one from the distribution
-      Assert.equal(engine.description, "override");
+  Assert.equal(
-    });
+    engine.description,
-  });
+    "override",
+    "distribution engine override installed"
+  );
 });

toolkit/components/search/tests/xpcshell/test_list_json_searchorder.js

@@ -6,6 +6,10 @@
 "use strict";
 
 add_task(async function setup() {
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setCharPref("browser.search.geoip.url", "");
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
   await AddonTestUtils.promiseStartupManager();
 });
 
@@ -18,7 +22,7 @@ add_task(async function test_searchOrderJSON() {
     .QueryInterface(Ci.nsIResProtocolHandler);
   resProt.setSubstitution("search-extensions", Services.io.newURI(url));
 
-  await asyncReInit();
+  await Services.search.init();
 
   Assert.ok(Services.search.isInitialized, "search initialized");
   Assert.equal(

toolkit/components/search/tests/xpcshell/test_migrateWebExtensionEngine.js

@@ -3,6 +3,14 @@
 
 "use strict";
 
+const { ExtensionTestUtils } = ChromeUtils.import(
+  "resource://testing-common/ExtensionXPCShellUtils.jsm"
+);
+ExtensionTestUtils.init(this);
+
+AddonTestUtils.usePrivilegedSignatures = false;
+AddonTestUtils.overrideCertDB();
+
 const kSearchEngineID = "addEngineWithDetails_test_engine";
 const kExtensionID = "test@example.com";
 
@@ -13,16 +21,14 @@ const kSearchEngineDetails = {
     "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==",
   suggestURL: "http://example.com/?suggest={searchTerms}",
   alias: "alias_foo",
-  extensionID: kExtensionID,
 };
 
 add_task(async function setup() {
   await AddonTestUtils.promiseStartupManager();
+  await Services.search.init();
 });
 
 add_task(async function test_migrateLegacyEngine() {
-  Assert.ok(!Services.search.isInitialized);
-
   await Services.search.addEngineWithDetails(
     kSearchEngineID,
     kSearchEngineDetails
@@ -30,19 +36,125 @@ add_task(async function test_migrateLegacyEngine() {
 
   // Modify the loadpath so it looks like an legacy plugin loadpath
   let engine = Services.search.getEngineByName(kSearchEngineID);
+  Assert.ok(!!engine, "opensearch engine installed");
   engine.wrappedJSObject._loadPath = `jar:[profile]/extensions/${kExtensionID}.xpi!/engine.xml`;
-  engine.wrappedJSObject._extensionID = null;
+  await Services.search.setDefault(engine);
-
+  Assert.equal(
-  // This should replace the existing engine
+    engine.name,
-  await Services.search.addEngineWithDetails(
+    Services.search.defaultEngine.name,
-    kSearchEngineID,
+    "set engine to default"
-    kSearchEngineDetails
   );
 
+  // We assume the default engines are installed, so our position will be after the default engine.
+  // This sets up the test to later test the engine position after updates.
+  let allEngines = await Services.search.getEngines();
+  Assert.ok(
+    allEngines.length > 2,
+    "default engines available " + allEngines.length
+  );
+  let origIndex = allEngines.map(e => e.name).indexOf(kSearchEngineID);
+  Assert.ok(
+    origIndex > 1,
+    "opensearch engine installed at position " + origIndex
+  );
+  await Services.search.moveEngine(engine, origIndex - 1);
+  let index = (await Services.search.getEngines())
+    .map(e => e.name)
+    .indexOf(kSearchEngineID);
+  Assert.equal(
+    origIndex - 1,
+    index,
+    "opensearch engine moved to position " + index
+  );
+
+  // Replace the opensearch extension with a webextension
+  let extensionInfo = {
+    useAddonManager: "permanent",
+    manifest: {
+      version: "1.0",
+      applications: {
+        gecko: {
+          id: kExtensionID,
+        },
+      },
+      chrome_settings_overrides: {
+        search_provider: {
+          name: kSearchEngineID,
+          search_url: "https://example.com/?q={searchTerms}",
+        },
+      },
+    },
+  };
+
+  let extension = ExtensionTestUtils.loadExtension(extensionInfo);
+  await extension.startup();
+
   engine = Services.search.getEngineByName(kSearchEngineID);
   Assert.equal(
     engine.wrappedJSObject._loadPath,
     "[other]addEngineWithDetails:" + kExtensionID
   );
   Assert.equal(engine.wrappedJSObject._extensionID, kExtensionID);
+  Assert.equal(engine.wrappedJSObject._version, "1.0");
+  index = (await Services.search.getEngines())
+    .map(e => e.name)
+    .indexOf(kSearchEngineID);
+  Assert.equal(origIndex - 1, index, "webext position " + index);
+  Assert.equal(
+    engine.name,
+    Services.search.defaultEngine.name,
+    "engine stil default"
+  );
+
+  extensionInfo.manifest.version = "2.0";
+  await extension.upgrade(extensionInfo);
+  await AddonTestUtils.waitForSearchProviderStartup(extension);
+
+  engine = Services.search.getEngineByName(kSearchEngineID);
+  Assert.equal(
+    engine.wrappedJSObject._loadPath,
+    "[other]addEngineWithDetails:" + kExtensionID
+  );
+  Assert.equal(engine.wrappedJSObject._extensionID, kExtensionID);
+  Assert.equal(engine.wrappedJSObject._version, "2.0");
+  index = (await Services.search.getEngines())
+    .map(e => e.name)
+    .indexOf(kSearchEngineID);
+  Assert.equal(origIndex - 1, index, "webext position " + index);
+  Assert.equal(
+    engine.name,
+    Services.search.defaultEngine.name,
+    "engine stil default"
+  );
+
+  // A different extension cannot use the same name
+  extensionInfo.manifest.applications.gecko.id = "takeover@search.foo";
+  let otherExt = ExtensionTestUtils.loadExtension(extensionInfo);
+  await otherExt.startup();
+  // Verify correct owner
+  engine = Services.search.getEngineByName(kSearchEngineID);
+  Assert.equal(
+    engine.wrappedJSObject._extensionID,
+    kExtensionID,
+    "prior search engine could not be overwritten"
+  );
+  // Verify no engine installed
+  let engines = await Services.search.getEnginesByExtensionID(
+    "takeover@search.foo"
+  );
+  Assert.equal(engines.length, 0, "no search engines installed");
+  await otherExt.unload();
+
+  // An opensearch engine cannot replace a webextension.
+  try {
+    await Services.search.addEngineWithDetails(
+      kSearchEngineID,
+      kSearchEngineDetails
+    );
+    Assert.ok(false, "unable to install opensearch over webextension");
+  } catch (e) {
+    Assert.ok(true, "unable to install opensearch over webextension");
+  }
+
+  await extension.unload();
 });

toolkit/components/search/tests/xpcshell/test_parseSubmissionURL.js

@@ -18,7 +18,7 @@ add_task(async function test_parseSubmissionURL() {
     await Services.search.removeEngine(engine);
   }
 
-  let [engine1, engine2, engine3, engine4] = await addTestEngines([
+  let engines = await addTestEngines([
     { name: "Test search engine", xmlFileName: "engine.xml" },
     { name: "Test search engine (fr)", xmlFileName: "engine-fr.xml" },
     {
@@ -52,113 +52,132 @@ add_task(async function test_parseSubmissionURL() {
     },
   ]);
 
-  engine3.addParam("q", "{searchTerms}", null);
+  engines[2].addParam("q", "{searchTerms}", null);
-  engine4.addParam("q", "{searchTerms}", null);
+  engines[3].addParam("q", "{searchTerms}", null);
+
+  function testParseSubmissionURL(url, engine, terms = "", offsetTerm) {
+    let result = Services.search.parseSubmissionURL(url);
+    Assert.equal(result.engine.name, engine.name, "engine matches");
+    Assert.equal(result.terms, terms, "term matches");
+    if (offsetTerm) {
+      Assert.ok(
+        url.slice(result.termsOffset).startsWith(offsetTerm),
+        "offset term matches"
+      );
+      Assert.equal(
+        result.termsLength,
+        offsetTerm.length,
+        "offset term length matches"
+      );
+    } else {
+      Assert.equal(result.termsOffset, url.length, "no term offset");
+    }
+  }
 
   // Test the first engine, whose URLs use UTF-8 encoding.
-  let url = "http://www.google.com/search?foo=bar&q=caff%C3%A8";
+  info("URLs use UTF-8 encoding");
-  let result = Services.search.parseSubmissionURL(url);
+  testParseSubmissionURL(
-  Assert.equal(result.engine, engine1);
+    "http://www.google.com/search?foo=bar&q=caff%C3%A8",
-  Assert.equal(result.terms, "caff\u00E8");
+    engines[0],
-  Assert.ok(url.slice(result.termsOffset).startsWith("caff%C3%A8"));
+    "caff\u00E8",
-  Assert.equal(result.termsLength, "caff%C3%A8".length);
+    "caff%C3%A8"
+  );
 
   // The second engine uses a locale-specific domain that is an alternate domain
   // of the first one, but the second engine should get priority when matching.
   // The URL used with this engine uses ISO-8859-1 encoding instead.
-  url = "http://www.google.fr/search?q=caff%E8";
+  info("URLs use alternate domain and ISO-8859-1 encoding");
-  result = Services.search.parseSubmissionURL(url);
+  testParseSubmissionURL(
-  Assert.equal(result.engine, engine2);
+    "http://www.google.fr/search?q=caff%E8",
-  Assert.equal(result.terms, "caff\u00E8");
+    engines[1],
-  Assert.ok(url.slice(result.termsOffset).startsWith("caff%E8"));
+    "caff\u00E8",
-  Assert.equal(result.termsLength, "caff%E8".length);
+    "caff%E8"
+  );
 
   // Test a domain that is an alternate domain of those defined.  In this case,
   // the first matching engine from the ordered list should be returned.
-  url = "http://www.google.co.uk/search?q=caff%C3%A8";
+  info("URLs use alternate domain");
-  result = Services.search.parseSubmissionURL(url);
+  testParseSubmissionURL(
-  Assert.equal(result.engine, engine1);
+    "http://www.google.co.uk/search?q=caff%C3%A8",
-  Assert.equal(result.terms, "caff\u00E8");
+    engines[0],
-  Assert.ok(url.slice(result.termsOffset).startsWith("caff%C3%A8"));
+    "caff\u00E8",
-  Assert.equal(result.termsLength, "caff%C3%A8".length);
+    "caff%C3%A8"
+  );
 
   // We support parsing URLs from a dynamically added engine.  Those engines use
   // windows-1252 encoding by default.
-  url = "http://www.bacon.test/find?q=caff%E8";
+  info("URLs use windows-1252");
-  result = Services.search.parseSubmissionURL(url);
+  testParseSubmissionURL(
-  Assert.equal(result.engine, engine3);
+    "http://www.bacon.test/find?q=caff%E8",
-  Assert.equal(result.terms, "caff\u00E8");
+    engines[2],
-  Assert.ok(url.slice(result.termsOffset).startsWith("caff%E8"));
+    "caff\u00E8",
-  Assert.equal(result.termsLength, "caff%E8".length);
+    "caff%E8"
-
-  // Test URLs with unescaped unicode characters.
-  url = "http://www.google.com/search?q=foo+b\u00E4r";
-  result = Services.search.parseSubmissionURL(url);
-  Assert.equal(result.engine, engine1);
-  Assert.equal(result.terms, "foo b\u00E4r");
-  Assert.ok(url.slice(result.termsOffset).startsWith("foo+b\u00E4r"));
-  Assert.equal(result.termsLength, "foo+b\u00E4r".length);
-
-  // Test search engines with unescaped IDNs.
-  url = "http://www.b\u00FCcher.ch/search?q=foo+bar";
-  result = Services.search.parseSubmissionURL(url);
-  Assert.equal(result.engine, engine4);
-  Assert.equal(result.terms, "foo bar");
-  Assert.ok(url.slice(result.termsOffset).startsWith("foo+bar"));
-  Assert.equal(result.termsLength, "foo+bar".length);
-
-  // Test search engines with escaped IDNs.
-  url = "http://www.xn--bcher-kva.ch/search?q=foo+bar";
-  result = Services.search.parseSubmissionURL(url);
-  Assert.equal(result.engine, engine4);
-  Assert.equal(result.terms, "foo bar");
-  Assert.ok(url.slice(result.termsOffset).startsWith("foo+bar"));
-  Assert.equal(result.termsLength, "foo+bar".length);
-
-  // Parsing of parameters from an engine template URL is not supported.
-  Assert.equal(
-    Services.search.parseSubmissionURL("http://www.bacon.moz/search?q=").engine,
-    null
-  );
-  Assert.equal(
-    Services.search.parseSubmissionURL("https://duckduckgo.com?q=test").engine,
-    null
-  );
-  Assert.equal(
-    Services.search.parseSubmissionURL("https://duckduckgo.com/?q=test").engine,
-    null
   );
 
-  // HTTP and HTTPS schemes are interchangeable.
+  info("URLs with unescaped unicode characters");
-  url = "https://www.google.com/search?q=caff%C3%A8";
+  testParseSubmissionURL(
-  result = Services.search.parseSubmissionURL(url);
+    "http://www.google.com/search?q=foo+b\u00E4r",
-  Assert.equal(result.engine, engine1);
+    engines[0],
-  Assert.equal(result.terms, "caff\u00E8");
+    "foo b\u00E4r",
-  Assert.ok(url.slice(result.termsOffset).startsWith("caff%C3%A8"));
+    "foo+b\u00E4r"
-
-  // Decoding search terms with multiple spaces should work.
-  result = Services.search.parseSubmissionURL(
-    "http://www.google.com/search?q=+with++spaces+"
   );
-  Assert.equal(result.engine, engine1);
-  Assert.equal(result.terms, " with  spaces ");
 
-  // An empty query parameter should work the same.
+  info("URLs with unescaped IDNs");
-  url = "http://www.google.com/search?q=";
+  testParseSubmissionURL(
-  result = Services.search.parseSubmissionURL(url);
+    "http://www.b\u00FCcher.ch/search?q=foo+bar",
-  Assert.equal(result.engine, engine1);
+    engines[3],
-  Assert.equal(result.terms, "");
+    "foo bar",
-  Assert.equal(result.termsOffset, url.length);
+    "foo+bar"
+  );
 
-  // There should be no match when the path is different.
+  info("URLs with escaped IDNs");
-  result = Services.search.parseSubmissionURL(
+  testParseSubmissionURL(
+    "http://www.xn--bcher-kva.ch/search?q=foo+bar",
+    engines[3],
+    "foo bar",
+    "foo+bar"
+  );
+
+  info("URLs with engines using template params, no value");
+  testParseSubmissionURL("http://www.bacon.moz/search?q=", engines[5]);
+
+  info("URLs with engines using template params");
+  testParseSubmissionURL(
+    "https://duckduckgo.com?q=test",
+    engines[4],
+    "test",
+    "test"
+  );
+
+  info("HTTP and HTTPS schemes are interchangeable.");
+  testParseSubmissionURL(
+    "https://www.google.com/search?q=caff%C3%A8",
+    engines[0],
+    "caff\u00E8",
+    "caff%C3%A8"
+  );
+
+  info("Decoding search terms with multiple spaces should work.");
+  testParseSubmissionURL(
+    "http://www.google.com/search?q=+with++spaces+",
+    engines[0],
+    " with  spaces ",
+    "+with++spaces+"
+  );
+
+  info("An empty query parameter should work the same.");
+  testParseSubmissionURL("http://www.google.com/search?q=", engines[0]);
+
+  // These test slightly different so we don't use testParseSubmissionURL.
+  info("There should be no match when the path is different.");
+  let result = Services.search.parseSubmissionURL(
     "http://www.google.com/search/?q=test"
   );
   Assert.equal(result.engine, null);
   Assert.equal(result.terms, "");
   Assert.equal(result.termsOffset, -1);
 
-  // There should be no match when the argument is different.
+  info("There should be no match when the argument is different.");
   result = Services.search.parseSubmissionURL(
     "http://www.google.com/search?q2=test"
   );
@@ -166,7 +185,7 @@ add_task(async function test_parseSubmissionURL() {
   Assert.equal(result.terms, "");
   Assert.equal(result.termsOffset, -1);
 
-  // There should be no match for URIs that are not HTTP or HTTPS.
+  info("There should be no match for URIs that are not HTTP or HTTPS.");
   result = Services.search.parseSubmissionURL("file://localhost/search?q=test");
   Assert.equal(result.engine, null);
   Assert.equal(result.terms, "");

toolkit/components/search/tests/xpcshell/test_remove_profile_engine.js

@@ -35,7 +35,8 @@ add_task(async function run_test() {
 
   await promiseSaveCacheData(data);
 
-  await asyncReInit();
+  Services.search.reset();
+  await Services.search.init();
 
   // test the engine is loaded ok.
   let engine = Services.search.getEngineByName("bug645970");

toolkit/components/search/tests/xpcshell/test_require_engines_for_cache.js

@@ -0,0 +1,16 @@
+"strict";
+
+// https://bugzilla.mozilla.org/show_bug.cgi?id=1255605
+add_task(async function skip_writing_cache_without_engines() {
+  Services.prefs.setCharPref("browser.search.region", "US");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  await AddonTestUtils.promiseStartupManager();
+
+  useTestEngines("no-extensions");
+  Assert.strictEqual(
+    0,
+    (await Services.search.getEngines()).length,
+    "no engines loaded"
+  );
+  Assert.ok(!removeCacheFile(), "empty cache file was not created.");
+});

toolkit/components/search/tests/xpcshell/test_require_engines_in_cache.js

@@ -2,6 +2,8 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 add_task(async function setup() {
+  Services.prefs.setCharPref("browser.search.region", "US");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
   configureToLoadJarEngines();
   await AddonTestUtils.promiseStartupManager();
 });
@@ -9,7 +11,7 @@ add_task(async function setup() {
 add_task(async function ignore_cache_files_without_engines() {
   let commitPromise = promiseAfterCache();
   let engineCount = (await Services.search.getEngines()).length;
-  Assert.equal(engineCount, 1);
+  Assert.equal(engineCount, 1, "one engine installed on search init");
 
   // Wait for the file to be saved to disk, so that we can mess with it.
   await commitPromise;
@@ -22,7 +24,11 @@ add_task(async function ignore_cache_files_without_engines() {
   // Check that after an async re-initialization, we still have the same engine count.
   commitPromise = promiseAfterCache();
   await asyncReInit();
-  Assert.equal(engineCount, (await Services.search.getEngines()).length);
+  Assert.equal(
+    engineCount,
+    (await Services.search.getEngines()).length,
+    "Search got correct number of engines"
+  );
   await commitPromise;
 
   // Check that after a sync re-initialization, we still have the same engine count.
@@ -32,41 +38,13 @@ add_task(async function ignore_cache_files_without_engines() {
   );
   let reInitPromise = asyncReInit();
   await unInitPromise;
-  Assert.ok(!Services.search.isInitialized);
+  Assert.ok(!Services.search.isInitialized, "Search is not initialized");
   // Synchronously check the engine count; will force a sync init.
-  Assert.equal(engineCount, (await Services.search.getEngines()).length);
+  Assert.equal(
-  Assert.ok(Services.search.isInitialized);
+    engineCount,
-  await reInitPromise;
+    (await Services.search.getEngines()).length,
-});
+    "Search got correct number of engines"
-
+  );
-add_task(async function skip_writing_cache_without_engines() {
+  Assert.ok(Services.search.isInitialized, "Search is initialized");
-  let unInitPromise = SearchTestUtils.promiseSearchNotification(
-    "uninit-complete"
-  );
-  let reInitPromise = asyncReInit();
-  await unInitPromise;
-
-  // Configure so that no engines will be found.
-  Assert.ok(removeCacheFile());
-  let resProt = Services.io
-    .getProtocolHandler("resource")
-    .QueryInterface(Ci.nsIResProtocolHandler);
-  resProt.setSubstitution(
-    "search-extensions",
-    Services.io.newURI("about:blank")
-  );
-
-  // Let the async-reInit happen.
-  await reInitPromise;
-  Assert.strictEqual(0, (await Services.search.getEngines()).length);
-
-  // Trigger yet another re-init, to flush of any pending cache writing task.
-  unInitPromise = SearchTestUtils.promiseSearchNotification("uninit-complete");
-  reInitPromise = asyncReInit();
-  await unInitPromise;
-
-  // Now check that a cache file doesn't exist.
-  Assert.ok(!removeCacheFile());
-
   await reInitPromise;
 });

toolkit/components/search/tests/xpcshell/test_validate_engines.js

@@ -9,10 +9,9 @@
 
 Cu.importGlobalProperties(["fetch"]);
 
-const { SearchService } = ChromeUtils.import(
+const { SearchUtils, SearchExtensionLoader } = ChromeUtils.import(
-  "resource://gre/modules/SearchService.jsm"
+  "resource://gre/modules/SearchUtils.jsm"
 );
-const LIST_JSON_URL = "resource://search-extensions/list.json";
 
 function traverse(obj, fun) {
   for (var i in obj) {
@@ -23,10 +22,10 @@ function traverse(obj, fun) {
   }
 }
 
-const ss = new SearchService();
+add_task(async function setup() {
-
+  // Read all the builtin engines and locales, create a giant list.json
-add_task(async function test_validate_engines() {
+  // that includes everything.
-  let engines = await fetch(LIST_JSON_URL).then(req => req.json());
+  let engines = await fetch(SearchUtils.LIST_JSON_URL).then(req => req.json());
 
   let visibleDefaultEngines = new Set();
   traverse(engines, (key, val) => {
@@ -40,8 +39,41 @@ add_task(async function test_validate_engines() {
       visibleDefaultEngines: Array.from(visibleDefaultEngines),
     },
   };
-  ss._listJSONURL = "data:application/json," + JSON.stringify(listjson);
+  SearchUtils.LIST_JSON_URL =
+    "data:application/json," + JSON.stringify(listjson);
 
+  // Set strict so the addon install promise is rejected.  This causes
+  // search.init to throw the error, and this test fails.
+  SearchExtensionLoader._strict = true;
   await AddonTestUtils.promiseStartupManager();
-  await ss.init();
+});
+
+add_task(async function test_validate_engines() {
+  // All engines should parse and init should work fine.
+  await Services.search
+    .init()
+    .then(() => {
+      ok(true, "all engines parsed and loaded");
+    })
+    .catch(() => {
+      ok(false, "an engine failed to parse and load");
+    });
+});
+
+add_task(async function test_install_timeout_failure() {
+  // Set an incredibly unachievable timeout here and make sure
+  // that init throws.  We're loading every engine/locale combo under the
+  // sun, it's unlikely we could intermittently succeed in loading
+  // them all.
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 1);
+  removeCacheFile();
+  Services.search.reset();
+  await Services.search
+    .init()
+    .then(() => {
+      ok(false, "search init did not time out");
+    })
+    .catch(error => {
+      equal(Cr.NS_ERROR_FAILURE, error, "search init timed out");
+    });
 });

toolkit/components/search/tests/xpcshell/test_webextensions_install_failure.js

@@ -0,0 +1,30 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+const { SearchExtensionLoader } = ChromeUtils.import(
+  "resource://gre/modules/SearchUtils.jsm"
+);
+const { ExtensionTestUtils } = ChromeUtils.import(
+  "resource://testing-common/ExtensionXPCShellUtils.jsm"
+);
+
+ExtensionTestUtils.init(this);
+AddonTestUtils.usePrivilegedSignatures = false;
+AddonTestUtils.overrideCertDB();
+
+add_task(async function test_install_manifest_failure() {
+  // Force addon loading to reject on errors
+  SearchExtensionLoader._strict = true;
+  useTestEngines("invalid-extension");
+  await AddonTestUtils.promiseStartupManager();
+
+  await Services.search
+    .init()
+    .then(() => {
+      ok(false, "search init did not throw");
+    })
+    .catch(e => {
+      equal(Cr.NS_ERROR_FAILURE, e, "search init error");
+    });
+});

toolkit/components/search/tests/xpcshell/xpcshell.ini

@@ -48,6 +48,8 @@ support-files =
   data/test-extensions/multilocale/manifest.json
   data/test-extensions/multilocale/_locales/af/messages.json
   data/test-extensions/multilocale/_locales/an/messages.json
+  data/invalid-extension/list.json
+  data/invalid-extension/invalid/manifest.json
 tags=searchmain
 
 [test_nocache.js]
@@ -99,6 +101,7 @@ tags = addons
 [test_geodefaults.js]
 [test_hidden.js]
 [test_currentEngine_fallback.js]
+[test_require_engines_for_cache.js]
 [test_require_engines_in_cache.js]
 skip-if = (verify && !debug && (os == 'linux'))
 [test_svg_icon.js]
@@ -113,4 +116,5 @@ skip-if = (verify && !debug && (os == 'linux'))
 [test_validate_engines.js]
 [test_validate_manifests.js]
 [test_webextensions_install.js]
+[test_webextensions_install_failure.js]
 [test_purpose.js]

toolkit/components/telemetry/tests/marionette/harness/telemetry_harness/runner.py

@@ -25,10 +25,14 @@ class TelemetryTestRunner(BaseMarionetteTestRunner):
         # Set Firefox Client Telemetry specific preferences
         prefs.update(
             {
-                # Fake the geoip lookup to always return Germany to:
+                # Force search region to DE and disable geo lookups.
-                #   * avoid net access in tests
+                "browser.search.region": "DE",
-                #   * stabilize browser.search.region to avoid an extra subsession (bug 1545207)
+                "browser.search.geoSpecificDefaults": False,
-                "browser.search.geoip.url": "data:application/json,{\"country_code\": \"DE\"}",
+                # Turn off timeouts for loading search extensions
+                "browser.search.addonLoadTimeout": 0,
+                "browser.search.log": True,
+                # geoip is skipped if url is empty (bug 1545207)
+                "browser.search.geoip.url": "",
                 # Disable smart sizing because it changes prefs at startup. (bug 1547750)
                 "browser.cache.disk.smart_size.enabled": False,
                 "toolkit.telemetry.server": "{}/pings".format(SERVER_URL),

toolkit/components/telemetry/tests/unit/head.js

@@ -484,6 +484,11 @@ function setEmptyPrefWatchlist() {
 }
 
 if (runningInParent) {
+  // Turn off region updates and timeouts for search service
+  Services.prefs.setCharPref("browser.search.region", "US");
+  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
+  Services.prefs.setIntPref("browser.search.addonLoadTimeout", 0);
+
   // Set logging preferences for all the tests.
   Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
   // Telemetry archiving should be on.