Bug 1647320 - Make the search cache responsible for listening to notifications of changes. r=daleharvey.

This patch additionally makes the search cache responsible for listening to notifications rather than being directly told by the search service to write the cache.

It also makes writes after init/reinit/maybeReloadEngines into delayed writes as they don't need to be immediate - the code already ensures we write any pending cache before reading, and that we write it before shutdown. Therefore, it doesn't matter if we wait a second or so.

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

toolkit/components/search/SearchCache.jsm

@@ -46,9 +46,6 @@ 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";
 
 /**
@@ -62,6 +59,12 @@ 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.
    */
@@ -93,14 +96,32 @@ 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() {
+  async get(origin = "") {
     let json;
+    await this._ensurePendingWritesCompleted(origin);
     try {
       let cacheFilePath = OS.Path.join(
         OS.Constants.Path.profileDir,
@@ -135,15 +156,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, CACHE_INVALIDATION_DELAY);
+      this._batchTask = new DeferredTask(task, this.CACHE_INVALIDATION_DELAY);
     }
     this._batchTask.arm();
   }
@@ -156,26 +177,27 @@ 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) {
+    if (!this._batchTask) {
-      logConsole.debug("finalizing batch task");
+      return;
-      let task = this._batchTask;
+    }
-      this._batchTask = null;
+    logConsole.debug("finalizing batch task");
-      // Tests manipulate the cache directly, so let's not double-write with
+    let task = this._batchTask;
-      // stale cache data here.
+    this._batchTask = null;
-      if (origin == "test") {
+    // Tests manipulate the cache directly, so let's not double-write with
-        task.disarm();
+    // stale cache data here.
-      } else {
+    if (origin == "test") {
-        await task.finalize();
+      task.disarm();
-      }
+    } else {
+      await task.finalize();
     }
   }
 
   /**
    * Writes the cache to disk (no delay).
    */
-  async write() {
+  async _write() {
     if (this._batchTask) {
       this._batchTask.disarm();
     }
@@ -238,7 +260,7 @@ class SearchCache {
    */
   setAttribute(name, val) {
     this._metaData[name] = val;
-    this.delayedWrite();
+    this._delayedWrite();
   }
 
   /**
@@ -251,8 +273,10 @@ class SearchCache {
    *   The value to set.
    */
   setVerifiedAttribute(name, val) {
-    this.setAttribute(name, val);
+    this._metaData[name] = val;
-    this.setAttribute(this.getHashName(name), getVerificationHash(val));
+    this._metaData[this.getHashName(name)] = getVerificationHash(val);
+    console.log(this._metaData);
+    this._delayedWrite();
   }
 
   /**
@@ -329,4 +353,28 @@ 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,7 +556,6 @@ 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;
@@ -1292,8 +1291,6 @@ 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;
@@ -1305,8 +1302,6 @@ 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.
@@ -1371,7 +1366,6 @@ 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();
@@ -1384,7 +1378,7 @@ SearchService.prototype = {
           "uninit-complete"
         );
 
-        let cache = await this._cache.get();
+        let cache = await this._cache.get(origin);
         // 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.
@@ -1411,9 +1405,6 @@ 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;
@@ -3344,7 +3335,6 @@ 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;
@@ -3457,6 +3447,8 @@ 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 = {
@@ -3509,6 +3501,8 @@ 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,6 +12,7 @@ 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",
@@ -69,6 +70,10 @@ 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
+    Services.search.wrappedJSObject._cache
       .QueryInterface(Ci.nsIObserver)
       .observe(null, "browser-search-engine-modified", "engine-removed");
   });

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

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