Bug 1492475 - Part 8: Introduce an init flag, which can only be used privately, that allows to explicitly skip waiting for the region check process to complete. r=florian
authorMike de Boer <mdeboer@mozilla.com>
Wed, 12 Dec 2018 14:00:43 +0100
changeset 452870 c44e674e160ebab49ea5ba1ed5821bb8d3c30e53
parent 452869 c8ee92973f24a44496f2bee23c13e0c74b6e11d8
child 452871 6c79eaf1d349638258d542ced0229d786f022683
push id2
push usermdeboer@mozilla.com
push dateTue, 08 Jan 2019 17:01:21 +0000
reviewersflorian
bugs1492475
milestone66.0a1
Bug 1492475 - Part 8: Introduce an init flag, which can only be used privately, that allows to explicitly skip waiting for the region check process to complete. r=florian Differential Revision: https://phabricator.services.mozilla.com/D14249
toolkit/components/search/nsISearchService.idl
toolkit/components/search/nsSearchService.js
toolkit/components/search/tests/xpcshell/test_reloadEngines.js
--- a/toolkit/components/search/nsISearchService.idl
+++ b/toolkit/components/search/nsISearchService.idl
@@ -240,16 +240,19 @@ interface nsIBrowserSearchInitObserver :
 interface nsISearchService : nsISupports
 {
   /**
    * Start asynchronous initialization.
    *
    * The promise is resolved once initialization is complete, which may be
    * immediately, if initialization has already been completed by some previous
    * call to this method.
+   * This method should only be called when you need or want to wait for the
+   * full initialization of the search service, which may include waiting for
+   * outbound service requests.
    */
   Promise init();
 
   /**
    * Determine whether initialization has been completed.
    *
    * Clients of the service can use this attribute to quickly determine whether
    * initialization is complete, and decide to trigger some immediate treatment,
--- a/toolkit/components/search/nsSearchService.js
+++ b/toolkit/components/search/nsSearchService.js
@@ -2600,16 +2600,18 @@ SearchService.prototype = {
 
   // The current status of initialization. Note that it does not determine if
   // initialization is complete, only if an error has been encountered so far.
   _initRV: Cr.NS_OK,
 
   // The boolean indicates that the initialization has started or not.
   _initStarted: null,
 
+  _ensureKnownRegionPromise: null,
+
   // Reading the JSON cache file is the first thing done during initialization.
   // During the async init, we save it in a field so that if we have to do a
   // sync init before the async init finishes, we can avoid reading the cache
   // with sync disk I/O and handling lz4 decompression synchronously.
   // This is set back to null as soon as the initialization is finished.
   _cacheFileJSON: null,
 
   // If initialization has not been completed yet, perform synchronous
@@ -2629,29 +2631,39 @@ SearchService.prototype = {
       "where to fix it:\n");
     err.message += err.stack;
     throw err;
   },
 
   /**
    * Asynchronous implementation of the initializer.
    *
+   * @param   [optional] skipRegionCheck
+   *          A boolean value indicating whether we should explicitly await the
+   *          the region check process to complete, which may be fetched remotely.
+   *          Pass in `false` if the caller needs to be absolutely certain of the
+   *          correct default engine and/ or ordering of visible engines.
    * @returns {Promise} A promise, resolved successfully if the initialization
    * succeeds.
    */
-  async _init() {
+  async _init(skipRegionCheck) {
     LOG("_init start");
 
     // See if we have a cache file so we don't have to parse a bunch of XML.
     let cache = await this._readCacheFile();
 
     // The init flow is not going to block on a fetch from an external service,
     // but we're kicking it off as soon as possible to prevent UI flickering as
     // much as possible.
-    ensureKnownRegion(this).catch(ex => LOG("_init: failure determining region: " + ex));
+    this._ensureKnownRegionPromise = ensureKnownRegion(this)
+      .catch(ex => LOG("_init: failure determining region: " + ex))
+      .finally(() => this._ensureKnownRegionPromise = null);
+    if (!skipRegionCheck) {
+      await this._ensureKnownRegionPromise;
+    }
 
     try {
       await this._loadEngines(cache);
     } catch (ex) {
       this._initRV = Cr.NS_ERROR_FAILURE;
       LOG("_init: failure loading engines: " + ex);
     }
     // Make sure the current list of engines is persisted, without the need to wait.
@@ -2936,17 +2948,19 @@ SearchService.prototype = {
         // be notified when we are done uninitializing.
         Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC,
                                      "uninit-complete");
 
         let cache = await this._readCacheFile();
         // The init flow is not going to block on a fetch from an external service,
         // but we're kicking it off as soon as possible to prevent UI flickering as
         // much as possible.
-        ensureKnownRegion(this).catch(ex => LOG("_reInit: failure determining region: " + ex));
+        this._ensureKnownRegionPromise = ensureKnownRegion(this)
+          .catch(ex => LOG("_reInit: failure determining region: " + ex))
+          .finally(() => this._ensureKnownRegionPromise = null);
         await this._loadEngines(cache);
         // Make sure the current list of engines is persisted.
         await this._buildCache();
 
         // Typically we'll re-init as a result of a pref observer,
         // so signal to 'callers' that we're done.
         gInitialized = true;
         this._initObservers.resolve();
@@ -3526,27 +3540,30 @@ SearchService.prototype = {
       return this._sortedEngines;
 
     return this._sortedEngines.filter(function(engine) {
                                         return !engine.hidden;
                                       });
   },
 
   // nsISearchService
-  async init() {
+  async init(skipRegionCheck = false) {
     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 {
       // Complete initialization by calling asynchronous initializer.
-      await this._init();
+      await this._init(skipRegionCheck);
       TelemetryStopwatch.finish("SEARCH_SERVICE_INIT_MS");
     } catch (ex) {
       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);
@@ -3561,31 +3578,31 @@ SearchService.prototype = {
     return this._initRV;
   },
 
   get isInitialized() {
     return gInitialized;
   },
 
   async getEngines() {
-    await this.init();
+    await this.init(true);
     LOG("getEngines: getting all engines");
     var engines = this._getSortedEngines(true);
     return engines;
   },
 
   async getVisibleEngines() {
     await this.init();
     LOG("getVisibleEngines: getting all visible engines");
     var engines = this._getSortedEngines(false);
     return engines;
   },
 
   async getDefaultEngines() {
-    await this.init();
+    await this.init(true);
     function isDefault(engine) {
       return engine._isDefault;
     }
     var engines = this._sortedEngines.filter(isDefault);
     var engineOrder = {};
     var engineName;
     var i = 1;
 
@@ -3640,17 +3657,17 @@ SearchService.prototype = {
 
       return a.name.localeCompare(b.name);
     }
     engines.sort(compareEngines);
     return engines;
   },
 
   async getEnginesByExtensionID(extensionID) {
-    await this.init();
+    await this.init(true);
     LOG("getEngines: getting all engines for " + extensionID);
     var engines = this._getSortedEngines(true).filter(function(engine) {
       return engine._extensionID == extensionID;
     });
     return engines;
   },
 
   getEngineByName: function SRCH_SVC_getEngineByName(aEngineName) {
@@ -3681,17 +3698,17 @@ SearchService.prototype = {
         alias,
         description,
         method,
         template,
         extensionID,
       };
     }
 
-    await this.init();
+    await this.init(true);
     if (!name)
       FAIL("Invalid name passed to addEngineWithDetails!");
     if (!params.template)
       FAIL("Invalid template passed to addEngineWithDetails!");
     let existingEngine = this._engines[name];
     if (existingEngine) {
       if (params.extensionID &&
           existingEngine._loadPath.startsWith(`jar:[profile]/extensions/${params.extensionID}`)) {
@@ -3744,17 +3761,17 @@ SearchService.prototype = {
       queryCharset: searchProvider.encoding || "UTF-8",
       mozParams: searchProvider.params,
     };
     return this.addEngineWithDetails(searchProvider.name.trim(), params);
   },
 
   async addEngine(engineURL, iconURL, confirm, extensionID) {
     LOG("addEngine: Adding \"" + engineURL + "\".");
-    await this.init();
+    await this.init(true);
     let errCode;
     try {
       var uri = makeURI(engineURL);
       var engine = new Engine(uri, false);
       engine._setIcon(iconURL, false);
       engine._confirm = confirm;
       if (extensionID) {
         engine._extensionID = extensionID;
@@ -3775,17 +3792,17 @@ SearchService.prototype = {
       if (engine)
         engine._installCallback = null;
       FAIL("addEngine: Error adding engine:\n" + ex, errCode || Cr.NS_ERROR_FAILURE);
     }
     return engine;
   },
 
   async removeEngine(engine) {
-    await this.init();
+    await this.init(true);
     if (!engine)
       FAIL("no engine passed to removeEngine!");
 
     var engineToRemove = null;
     for (var e in this._engines) {
       if (engine.wrappedJSObject == this._engines[e])
         engineToRemove = this._engines[e];
     }
@@ -3824,17 +3841,17 @@ SearchService.prototype = {
 
       // Since we removed an engine, we need to update the preferences.
       this._saveSortedEngineList();
     }
     notifyAction(engineToRemove, SEARCH_ENGINE_REMOVED);
   },
 
   async moveEngine(engine, newIndex) {
-    await this.init();
+    await this.init(true);
     if ((newIndex > this._sortedEngines.length) || (newIndex < 0))
       FAIL("SRCH_SVC_moveEngine: Index out of bounds!");
     if (!(engine instanceof Ci.nsISearchEngine) && !(engine instanceof Engine))
       FAIL("SRCH_SVC_moveEngine: Invalid engine passed to moveEngine!");
     if (engine.hidden)
       FAIL("moveEngine: Can't move a hidden engine!", Cr.NS_ERROR_FAILURE);
 
     engine = engine.wrappedJSObject;
@@ -3975,22 +3992,22 @@ SearchService.prototype = {
     this.setGlobalAttr("current", newName);
     this.setGlobalAttr("hash", getVerificationHash(newName));
 
     notifyAction(this._currentEngine, SEARCH_ENGINE_DEFAULT);
     notifyAction(this._currentEngine, SEARCH_ENGINE_CURRENT);
   },
 
   async getDefault() {
-    await this.init();
+    await this.init(true);
     return this.defaultEngine;
   },
 
   async setDefault(engine) {
-    await this.init();
+    await this.init(true);
     return this.defaultEngine = engine;
   },
 
   async getDefaultEngineInfo() {
     let result = {};
 
     let engine;
     try {
--- a/toolkit/components/search/tests/xpcshell/test_reloadEngines.js
+++ b/toolkit/components/search/tests/xpcshell/test_reloadEngines.js
@@ -22,17 +22,17 @@ add_task(async function test_regular_ini
 
     // Install kTestEngineName and wait for it to reach the disk.
     await Promise.all([installTestEngine(), promiseAfterCache()]);
 
     Assert.equal(kTestEngineName, (await Services.search.getDefault()).name,
       "Geo defined default should be set");
     checkNoRequest(requests);
 
-    Assert.ok(reloadObserved, "Engines should be reloaded during test, because region fetch succeeded");
+    Assert.ok(!reloadObserved, "Engines should not be reloaded during test, because region fetch succeeded");
     Services.obs.removeObserver(obs, SEARCH_SERVICE_TOPIC);
   }, { delay: 100 });
 });
 
 add_task(async function test_init_with_skip_regioncheck() {
   await withGeoServer(async function cont(requests) {
     let reloadObserved = false;
     let obs = (subject, topic, data) => {