Bug 1492475 - Part 6: Update the cache build task to work with an actual Promise and re-initialize only once at the same time - all to fix race conditions here. r=florian
authorMike de Boer <mdeboer@mozilla.com>
Wed, 17 Oct 2018 12:51:51 +0200
changeset 452868 2ae7189dfaa63cab0e264e7a2796b1610505c40a
parent 452867 cd41189eac88aa6023af1b0a060c15ddcd407952
child 452869 c8ee92973f24a44496f2bee23c13e0c74b6e11d8
push id2
push usermdeboer@mozilla.com
push dateTue, 08 Jan 2019 17:01:21 +0000
reviewersflorian
bugs1492475
milestone66.0a1
Bug 1492475 - Part 6: Update the cache build task to work with an actual Promise and re-initialize only once at the same time - all to fix race conditions here. r=florian Differential Revision: https://phabricator.services.mozilla.com/D9280
toolkit/components/search/nsSearchService.js
--- a/toolkit/components/search/nsSearchService.js
+++ b/toolkit/components/search/nsSearchService.js
@@ -846,16 +846,17 @@ function getMozParamPref(prefName) {
  * @param aEngine
  *        The nsISearchEngine object to which the change applies.
  * @param aVerb
  *        A verb describing the change.
  *
  * @see nsISearchService.idl
  */
 var gInitialized = false;
+var gReinitializing = false;
 function notifyAction(aEngine, aVerb) {
   if (gInitialized) {
     LOG("NOTIFY: Engine: \"" + aEngine.name + "\"; Verb: \"" + aVerb + "\"");
     Services.obs.notifyObservers(aEngine, SEARCH_ENGINE_TOPIC, aVerb);
   }
 }
 
 function parseJsonFromStream(aInputStream) {
@@ -2646,16 +2647,18 @@ SearchService.prototype = {
       LOG("_init: failure determining region: " + ex);
     }
     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.
+    this._buildCache();
     this._addObservers();
     gInitialized = true;
     if (Components.isSuccessCode(this._initRV)) {
       this._initObservers.resolve(this._initRV);
     } else {
       this._initObservers.reject(this._initRV);
     }
     Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "init-complete");
@@ -2730,17 +2733,17 @@ SearchService.prototype = {
   },
 
   resetToOriginalDefaultEngine: function SRCH_SVC__resetToOriginalDefaultEngine() {
     let originalDefaultEngine = this.originalDefaultEngine;
     originalDefaultEngine.hidden = false;
     this.defaultEngine = originalDefaultEngine;
   },
 
-  _buildCache: function SRCH_SVC__buildCache() {
+  async _buildCache() {
     if (this._batchTask)
       this._batchTask.disarm();
 
     let cache = {};
     let locale = getLocale();
     let buildID = Services.appinfo.platformBuildID;
     let appVersion = Services.appinfo.version;
 
@@ -2766,27 +2769,20 @@ SearchService.prototype = {
 
     try {
       if (!cache.engines.length)
         throw "cannot write without any engine.";
 
       LOG("_buildCache: Writing to cache file.");
       let path = OS.Path.join(OS.Constants.Path.profileDir, CACHE_FILENAME);
       let data = gEncoder.encode(JSON.stringify(cache));
-      let promise = OS.File.writeAtomic(path, data, {compression: "lz4",
-                                                     tmpPath: path + ".tmp"});
-
-      promise.then(
-        function onSuccess() {
-          Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, SEARCH_SERVICE_CACHE_WRITTEN);
-        },
-        function onError(e) {
-          LOG("_buildCache: failure during writeAtomic: " + e);
-        }
-      );
+      await OS.File.writeAtomic(path, data, {compression: "lz4",
+                                             tmpPath: path + ".tmp"});
+      LOG("_buildCache: cache file written to disk.");
+      Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, SEARCH_SERVICE_CACHE_WRITTEN);
     } catch (ex) {
       LOG("_buildCache: Could not write to cache file: " + ex);
     }
   },
 
   /**
    * Loads engines asynchronously.
    *
@@ -2852,33 +2848,46 @@ SearchService.prototype = {
     }
     let enginesFromURLs = await this._loadFromChromeURLs(chromeURIs);
     enginesFromURLs.forEach(this._addEngineToStore, this);
 
     LOG("_loadEngines: loading user-installed engines from the obsolete cache");
     this._loadEnginesFromCache(cache, true);
 
     this._loadEnginesMetadataFromCache(cache);
-    this._buildCache();
 
     LOG("_loadEngines: done using rebuilt cache");
   },
 
-  _reInit() {
+  _reInit(origin) {
     LOG("_reInit");
+    // Re-entrance guard, because we're using an async lambda below.
+    if (gReinitializing) {
+      LOG("_reInit: already re-initializing, bailing out.");
+      return;
+    }
+    gReinitializing = true;
+
     // Start by clearing the initialized state, so we don't abort early.
     gInitialized = false;
 
     (async () => {
       try {
+        this._initObservers = PromiseUtils.defer();
         if (this._batchTask) {
           LOG("finalizing batch task");
           let task = this._batchTask;
           this._batchTask = null;
-          await task.finalize();
+          // Tests manipulate the cache directly, so let's not double-write with
+          // stale cache data here.
+          if (origin == "test") {
+            task.disarm();
+          } else {
+            await task.finalize();
+          }
         }
 
         // Clear the engines, too, so we don't stick with the stale ones.
         this._engines = {};
         this.__sortedEngines = null;
         this._currentEngine = null;
         this._visibleDefaultEngines = [];
         this._searchDefault = null;
@@ -2889,25 +2898,29 @@ SearchService.prototype = {
         // be notified when we are done uninitializing.
         Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC,
                                      "uninit-complete");
 
         let cache = await this._readCacheFile();
 
         await ensureKnownRegion(this);
         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();
         Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "init-complete");
       } catch (err) {
         LOG("Reinit failed: " + err);
         Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-failed");
       } finally {
+        gReinitializing = false;
         Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "reinit-complete");
       }
     })();
   },
 
   /**
    * Read the cache file asynchronously.
    *
@@ -2936,19 +2949,19 @@ SearchService.prototype = {
       this._metaData = json.metaData;
 
     return json;
   },
 
   _batchTask: null,
   get batchTask() {
     if (!this._batchTask) {
-      let task = () => {
+      let task = async () => {
         LOG("batchTask: Invalidating engine cache");
-        this._buildCache();
+        await this._buildCache();
       };
       this._batchTask = new DeferredTask(task, CACHE_INVALIDATION_DELAY);
     }
     return this._batchTask;
   },
 
   _submissionURLIgnoreList: [
     "ignore=true",
@@ -4285,17 +4298,17 @@ SearchService.prototype = {
         break;
 
       case TOPIC_LOCALES_CHANGE:
         // Locale changed. Re-init. We rely on observers, because we can't
         // return this promise to anyone.
         // FYI, This is also used by the search tests to do an async reinit.
         // Locales are removed during shutdown, so ignore this message
         if (!Services.startup.shuttingDown) {
-          this._reInit();
+          this._reInit(aVerb);
         }
         break;
     }
   },
 
   // nsITimerCallback
   notify: function SRCH_SVC_notify(aTimer) {
     LOG("_notify: checking for updates");