Backed out 4 changesets (bug 1647320, bug 1647359) for failures on /test_json_cache.js. CLOSED TREE

Backed out changeset fdf086cfd0f9 (bug 1647359)
Backed out changeset b39278706c66 (bug 1647359)
Backed out changeset 65c58515f2cb (bug 1647359)
Backed out changeset 7ebc3ec422ec (bug 1647320)

browser/components/preferences/search.js

@@ -639,7 +639,7 @@ EngineStore.prototype = {
 
   _cloneEngine(aEngine) {
     var clonedObj = {};
-    for (let i of ["name", "alias", "iconURI"]) {
+    for (var i in aEngine) {
       clonedObj[i] = aEngine[i];
     }
     clonedObj.originalEngine = aEngine;
@@ -738,7 +738,7 @@ EngineStore.prototype = {
 
   reloadIcons() {
     this._engines.forEach(function(e) {
-      e.iconURI = e.originalEngine.iconURI;
+      e.uri = e.originalEngine.uri;
     });
   },
 };

toolkit/components/search/SearchCache.jsm

@@ -46,6 +46,9 @@ XPCOMUtils.defineLazyGetter(this, "gEncoder", function() {
   return new TextEncoder();
 });
 
+// Delay for batching invalidation of the JSON cache (ms)
+const CACHE_INVALIDATION_DELAY = 1000;
+
 const CACHE_FILENAME = "search.json.mozlz4";
 
 /**
@@ -59,12 +62,6 @@ class SearchCache {
   constructor(searchService) {
     this._searchService = searchService;
   }
-
-  QueryInterface = ChromeUtils.generateQI([Ci.nsIObserver]);
-
-  // Delay for batching invalidation of the JSON cache (ms)
-  static CACHE_INVALIDATION_DELAY = 1000;
-
   /**
    * A reference to the pending DeferredTask, if there is one.
    */
@@ -96,32 +93,14 @@ class SearchCache {
    */
   _searchService;
 
-  addObservers() {
-    Services.obs.addObserver(this, SearchUtils.TOPIC_ENGINE_MODIFIED);
-    Services.obs.addObserver(this, SearchUtils.TOPIC_SEARCH_SERVICE);
-  }
-
-  /**
-   * Cleans up, removing observers.
-   */
-  removeObservers() {
-    Services.obs.removeObserver(this, SearchUtils.TOPIC_ENGINE_MODIFIED);
-    Services.obs.removeObserver(this, SearchUtils.TOPIC_SEARCH_SERVICE);
-  }
-
   /**
    * Reads the cache file.
    *
-   * @param {string} origin
-   *   If this parameter is "test", then the cache will not be written. As
-   *   some tests manipulate the cache directly, we allow turning off writing to
-   *   avoid writing stale cache data.
    * @returns {object}
    *   Returns the cache file data.
    */
-  async get(origin = "") {
+  async get() {
     let json;
-    await this._ensurePendingWritesCompleted(origin);
     try {
       let cacheFilePath = OS.Path.join(
         OS.Constants.Path.profileDir,
@@ -156,15 +135,15 @@ class SearchCache {
    * Queues writing the cache until after CACHE_INVALIDATION_DELAY. If there
    * is a currently queued task then it will be restarted.
    */
-  _delayedWrite() {
+  delayedWrite() {
     if (this._batchTask) {
       this._batchTask.disarm();
     } else {
       let task = async () => {
         logConsole.debug("batchTask: Invalidating engine cache");
-        await this._write();
+        await this.write();
       };
-      this._batchTask = new DeferredTask(task, this.CACHE_INVALIDATION_DELAY);
+      this._batchTask = new DeferredTask(task, CACHE_INVALIDATION_DELAY);
     }
     this._batchTask.arm();
   }
@@ -177,27 +156,26 @@ class SearchCache {
    *   some tests manipulate the cache directly, we allow turning off writing to
    *   avoid writing stale cache data.
    */
-  async _ensurePendingWritesCompleted(origin = "") {
+  async ensurePendingWritesCompleted(origin = "") {
     // Before we read the cache file, first make sure all pending tasks are clear.
-    if (!this._batchTask) {
-      return;
-    }
-    logConsole.debug("finalizing batch task");
-    let task = this._batchTask;
-    this._batchTask = null;
-    // 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();
+    if (this._batchTask) {
+      logConsole.debug("finalizing batch task");
+      let task = this._batchTask;
+      this._batchTask = null;
+      // 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();
+      }
     }
   }
 
   /**
    * Writes the cache to disk (no delay).
    */
-  async _write() {
+  async write() {
     if (this._batchTask) {
       this._batchTask.disarm();
     }
@@ -260,7 +238,7 @@ class SearchCache {
    */
   setAttribute(name, val) {
     this._metaData[name] = val;
-    this._delayedWrite();
+    this.delayedWrite();
   }
 
   /**
@@ -273,10 +251,8 @@ class SearchCache {
    *   The value to set.
    */
   setVerifiedAttribute(name, val) {
-    this._metaData[name] = val;
-    this._metaData[this.getHashName(name)] = getVerificationHash(val);
-    console.log(this._metaData);
-    this._delayedWrite();
+    this.setAttribute(name, val);
+    this.setAttribute(this.getHashName(name), getVerificationHash(val));
   }
 
   /**
@@ -353,28 +329,4 @@ class SearchCache {
       }
     }
   }
-
-  // nsIObserver
-  observe(engine, topic, verb) {
-    switch (topic) {
-      case SearchUtils.TOPIC_ENGINE_MODIFIED:
-        switch (verb) {
-          case SearchUtils.MODIFIED_TYPE.ADDED:
-          case SearchUtils.MODIFIED_TYPE.CHANGED:
-          case SearchUtils.MODIFIED_TYPE.REMOVED:
-            this._delayedWrite();
-            break;
-        }
-        break;
-      case SearchUtils.TOPIC_SEARCH_SERVICE:
-        switch (verb) {
-          case "init-complete":
-          case "reinit-complete":
-          case "engines-reloaded":
-            this._delayedWrite();
-            break;
-        }
-        break;
-    }
-  }
 }

toolkit/components/search/SearchService.jsm

@@ -556,6 +556,7 @@ SearchService.prototype = {
 
       // Make sure the current list of engines is persisted, without the need to wait.
       logConsole.debug("_init: engines loaded, writing cache");
+      this._cache.write();
       this._addObservers();
     } catch (ex) {
       this._initRV = ex.result !== undefined ? ex.result : Cr.NS_ERROR_FAILURE;
@@ -1291,6 +1292,8 @@ SearchService.prototype = {
       return;
     }
 
+    await this._cache.ensurePendingWritesCompleted();
+
     // Capture the current engine state, in case we need to notify below.
     const prevCurrentEngine = this._currentEngine;
     const prevPrivateEngine = this._currentPrivateEngine;
@@ -1302,6 +1305,8 @@ SearchService.prototype = {
     // engine order.
     this.__sortedEngines = null;
     await this._loadEngines(await this._cache.get(), true);
+    // Make sure the current list of engines is persisted.
+    await this._cache.write();
 
     // If the defaultEngine has changed between the previous load and this one,
     // dispatch the appropriate notifications.
@@ -1366,6 +1371,7 @@ SearchService.prototype = {
         }
 
         this._initObservers = PromiseUtils.defer();
+        await this._cache.ensurePendingWritesCompleted(origin);
 
         // Clear the engines, too, so we don't stick with the stale ones.
         this._resetLocalData();
@@ -1378,7 +1384,7 @@ SearchService.prototype = {
           "uninit-complete"
         );
 
-        let cache = await this._cache.get(origin);
+        let cache = await this._cache.get();
         // 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.
@@ -1405,6 +1411,9 @@ SearchService.prototype = {
           return;
         }
 
+        // Make sure the current list of engines is persisted.
+        await this._cache.write();
+
         // Typically we'll re-init as a result of a pref observer,
         // so signal to 'callers' that we're done.
         gInitialized = true;
@@ -3335,6 +3344,7 @@ SearchService.prototype = {
           case SearchUtils.MODIFIED_TYPE.ADDED:
           case SearchUtils.MODIFIED_TYPE.CHANGED:
           case SearchUtils.MODIFIED_TYPE.REMOVED:
+            this._cache.delayedWrite();
             // Invalidate the map used to parse URLs to search engines.
             this._parseSubmissionMap = null;
             break;
@@ -3447,8 +3457,6 @@ SearchService.prototype = {
     Services.obs.addObserver(this, QUIT_APPLICATION_TOPIC);
     Services.obs.addObserver(this, TOPIC_LOCALES_CHANGE);
 
-    this._cache.addObservers();
-
     // The current stage of shutdown. Used to help analyze crash
     // signatures in case of shutdown timeout.
     let shutdownState = {
@@ -3501,8 +3509,6 @@ SearchService.prototype = {
       this._queuedIdle = false;
     }
 
-    this._cache.removeObservers();
-
     Services.obs.removeObserver(this, SearchUtils.TOPIC_ENGINE_MODIFIED);
     Services.obs.removeObserver(this, QUIT_APPLICATION_TOPIC);
     Services.obs.removeObserver(this, TOPIC_LOCALES_CHANGE);

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

@@ -12,7 +12,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   Region: "resource://gre/modules/Region.jsm",
   RemoteSettings: "resource://services-settings/remote-settings.js",
   RemoteSettingsClient: "resource://services-settings/RemoteSettingsClient.jsm",
-  SearchCache: "resource://gre/modules/SearchCache.jsm",
   SearchEngineSelector: "resource://gre/modules/SearchEngineSelector.jsm",
   SearchTestUtils: "resource://testing-common/SearchTestUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
@@ -70,10 +69,6 @@ Services.prefs.setBoolPref(
   true
 );
 
-// For tests, allow the cache to write sooner than it would do normally so that
-// the tests that need to wait for it can run a bit faster.
-SearchCache.CACHE_INVALIDATION_DELAY = 250;
-
 /**
  * Load engines from test data located in particular folders.
  *

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

@@ -156,7 +156,7 @@ add_task(async function test_cache_write() {
   Services.tm.dispatchToMainThread(() => {
     // Call the observe method directly to simulate a remove but not actually
     // remove anything.
-    Services.search.wrappedJSObject._cache
+    Services.search
       .QueryInterface(Ci.nsIObserver)
       .observe(null, "browser-search-engine-modified", "engine-removed");
   });

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

@@ -113,4 +113,5 @@ add_task(async function basic_multilocale_test() {
   );
 
   await ext.unload();
+  await promiseAfterCache();
 });