Bug 1535049 - Separate out search ignore lists into their own module. r=daleharvey

This separates out the search ignore list handling into its own module in preparation for use elsewhere as well.

The search ignore list unit tests still largely interact with RemoteSettings to remain as integration tests.

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

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

toolkit/components/search/SearchService.jsm

@@ -19,8 +19,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   ExtensionParent: "resource://gre/modules/ExtensionParent.jsm",
   getVerificationHash: "resource://gre/modules/SearchEngine.jsm",
   OS: "resource://gre/modules/osfile.jsm",
-  RemoteSettings: "resource://services-settings/remote-settings.js",
-  RemoteSettingsClient: "resource://services-settings/RemoteSettingsClient.jsm",
+  IgnoreLists: "resource://gre/modules/IgnoreLists.jsm",
   SearchEngine: "resource://gre/modules/SearchEngine.jsm",
   SearchStaticData: "resource://gre/modules/SearchStaticData.jsm",
   SearchUtils: "resource://gre/modules/SearchUtils.jsm",
@@ -740,46 +739,6 @@ SearchService.prototype = {
     return this._initRV;
   },
 
-  /**
-   * Obtains the current ignore list from remote settings. This includes
-   * verifying the signature of the ignore list within the database.
-   *
-   * If the signature in the database is invalid, the database will be wiped
-   * and the stored dump will be used, until the settings next update.
-   *
-   * Note that this may cause a network check of the certificate, but that
-   * should generally be quick.
-   *
-   * @param {RemoteSettings} ignoreListSettings
-   *   The remote settings object associated with the ignore list.
-   * @param {boolean} [firstTime]
-   *   Internal boolean to indicate if this is the first time check or not.
-   * @returns {array}
-   *   An array of objects in the database, or an empty array if none
-   *   could be obtained.
-   */
-  async _getRemoteSettings(ignoreListSettings, firstTime = true) {
-    try {
-      return ignoreListSettings.get({ verifySignature: true });
-    } catch (ex) {
-      if (
-        ex instanceof RemoteSettingsClient.InvalidSignatureError &&
-        firstTime
-      ) {
-        // The local database is invalid, try and reset it.
-        const collection = await ignoreListSettings.openCollection();
-        await collection.clear();
-        await collection.db.close();
-        // Now call this again.
-        return this._getRemoteSettings(ignoreListSettings, false);
-      }
-      // Don't throw an error just log it, just continue with no data, and hopefully
-      // a sync will fix things later on.
-      Cu.reportError(ex);
-      return [];
-    }
-  },
-
   /**
    * Obtains the remote settings for the search service. This should only be
    * called from init(). Any subsequent updates to the remote settings are
@@ -790,15 +749,10 @@ SearchService.prototype = {
    * hence the `get` may take a while to return.
    */
   async _setupRemoteSettings() {
-    const ignoreListSettings = RemoteSettings(
-      SearchUtils.SETTINGS_IGNORELIST_KEY
-    );
-    // Trigger a get of the initial value.
-    const current = await this._getRemoteSettings(ignoreListSettings);
-
     // Now we have the values, listen for future updates.
     this._ignoreListListener = this._handleIgnoreListUpdated.bind(this);
-    ignoreListSettings.on("sync", this._ignoreListListener);
+
+    const current = await IgnoreLists.getAndSubscribe(this._ignoreListListener);
 
     await this._handleIgnoreListUpdated({ data: { current } });
     Services.obs.notifyObservers(
@@ -3096,10 +3050,7 @@ SearchService.prototype = {
 
   _removeObservers() {
     if (this._ignoreListListener) {
-      RemoteSettings(SearchUtils.SETTINGS_IGNORELIST_KEY).off(
-        "sync",
-        this._ignoreListListener
-      );
+      IgnoreLists.unsubscribe(this._ignoreListListener);
       delete this._ignoreListListener;
     }
 

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

@@ -1,87 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-const kSearchEngineID1 = "ignorelist_test_engine1";
-const kSearchEngineURL1 =
-  "http://example.com/?search={searchTerms}&ignore=true";
-const kExtensionID = "searchignore@mozilla.com";
-
-add_task(async function setup() {
-  await AddonTestUtils.promiseStartupManager();
-});
-
-add_task(async function test_ignoreList_db_modification() {
-  Assert.ok(
-    !Services.search.isInitialized,
-    "Search service should not be initialized to begin with."
-  );
-
-  // Fill the database so we can check it was cleared afterwards
-  const collection = await RemoteSettings("hijack-blocklists").openCollection();
-  await collection.clear();
-  await collection.create(
-    {
-      id: "submission-urls",
-      matches: ["ignore=true"],
-    },
-    { synced: true }
-  );
-  await collection.create(
-    {
-      id: "load-paths",
-      matches: ["[other]addEngineWithDetails:searchignore@mozilla.com"],
-    },
-    { synced: true }
-  );
-  await collection.db.saveLastModified(42);
-
-  const ignoreListSettings = RemoteSettings(
-    SearchUtils.SETTINGS_IGNORELIST_KEY
-  );
-  const getStub = sinon.stub(ignoreListSettings, "get");
-
-  // Stub the get() so that the first call simulates a signature error, and
-  // the second simulates success reading from the dump.
-  getStub
-    .onFirstCall()
-    .throws(new RemoteSettingsClient.InvalidSignatureError("abc"));
-  getStub.onSecondCall().returns([
-    {
-      id: "load-paths",
-      matches: ["[other]addEngineWithDetails:searchignore@mozilla.com"],
-      _status: "synced",
-    },
-    {
-      id: "submission-urls",
-      matches: ["ignore=true"],
-      _status: "synced",
-    },
-  ]);
-
-  const updatePromise = SearchTestUtils.promiseSearchNotification(
-    "settings-update-complete"
-  );
-
-  await Services.search.addEngineWithDetails(kSearchEngineID1, {
-    method: "get",
-    template: kSearchEngineURL1,
-  });
-
-  await updatePromise;
-
-  const engine = Services.search.getEngineByName(kSearchEngineID1);
-  Assert.equal(
-    engine,
-    null,
-    "Engine with ignored search params should not exist"
-  );
-
-  const databaseEntries = await collection.list();
-  Assert.equal(
-    databaseEntries.data.length,
-    0,
-    "Should have cleared the database."
-  );
-});

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

@@ -58,7 +58,6 @@ skip-if = true # Is confusing
 [test_engine_set_alias.js]
 [test_hasEngineWithURL.js]
 [test_identifiers.js]
-[test_ignorelist_db_modification.js]
 [test_ignorelist_update.js]
 [test_ignorelist.js]
 [test_invalid_engine_from_dir.js]

toolkit/modules/IgnoreLists.jsm

@@ -0,0 +1,99 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  RemoteSettings: "resource://services-settings/remote-settings.js",
+  RemoteSettingsClient: "resource://services-settings/RemoteSettingsClient.jsm",
+});
+
+var EXPORTED_SYMBOLS = ["IgnoreLists"];
+
+const SETTINGS_IGNORELIST_KEY = "hijack-blocklists";
+
+class IgnoreListsManager {
+  async init() {
+    if (!this._ignoreListSettings) {
+      this._ignoreListSettings = RemoteSettings(SETTINGS_IGNORELIST_KEY);
+    }
+  }
+
+  async getAndSubscribe(listener) {
+    await this.init();
+
+    // Trigger a get of the initial value.
+    const settings = await this._getIgnoreList();
+
+    // Listen for future updates after we first get the values.
+    this._ignoreListSettings.on("sync", listener);
+
+    return settings;
+  }
+
+  unsubscribe(listener) {
+    if (!this._ignoreListSettings) {
+      return;
+    }
+
+    this._ignoreListSettings.off("sync", listener);
+  }
+
+  async _getIgnoreList() {
+    if (this._getSettingsPromise) {
+      return this._getSettingsPromise;
+    }
+
+    const settings = await (this._getSettingsPromise = this._getIgnoreListSettings());
+    delete this._getSettingsPromise;
+    return settings;
+  }
+
+  /**
+   * Obtains the current ignore list from remote settings. This includes
+   * verifying the signature of the ignore list within the database.
+   *
+   * If the signature in the database is invalid, the database will be wiped
+   * and the stored dump will be used, until the settings next update.
+   *
+   * Note that this may cause a network check of the certificate, but that
+   * should generally be quick.
+   *
+   * @param {RemoteSettings} ignoreListSettings
+   *   The remote settings object associated with the ignore list.
+   * @param {boolean} [firstTime]
+   *   Internal boolean to indicate if this is the first time check or not.
+   * @returns {array}
+   *   An array of objects in the database, or an empty array if none
+   *   could be obtained.
+   */
+  async _getIgnoreListSettings(firstTime = true) {
+    try {
+      return this._ignoreListSettings.get({
+        verifySignature: true,
+      });
+    } catch (ex) {
+      if (
+        ex instanceof RemoteSettingsClient.InvalidSignatureError &&
+        firstTime
+      ) {
+        // The local database is invalid, try and reset it.
+        const collection = await this._ignoreListSettings.openCollection();
+        await collection.clear();
+        await collection.db.close();
+        // Now call this again.
+        return this._getIgnoreListSettings(this._ignoreListSettings, false);
+      }
+      // Don't throw an error just log it, just continue with no data, and hopefully
+      // a sync will fix things later on.
+      Cu.reportError(ex);
+    }
+    return [];
+  }
+}
+
+const IgnoreLists = new IgnoreListsManager();

toolkit/modules/moz.build

@@ -195,6 +195,7 @@ EXTRA_JS_MODULES += [
     'GMPInstallManager.jsm',
     'GMPUtils.jsm',
     'Http.jsm',
+    'IgnoreLists.jsm',
     'IndexedDB.jsm',
     'InlineSpellChecker.jsm',
     'InlineSpellCheckerContent.jsm',

toolkit/modules/tests/xpcshell/test_IgnoreList.js

@@ -0,0 +1,205 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const kSearchEngineID1 = "ignorelist_test_engine1";
+const kSearchEngineURL1 =
+  "http://example.com/?search={searchTerms}&ignore=true";
+const kExtensionID = "searchignore@mozilla.com";
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  IgnoreLists: "resource://gre/modules/IgnoreLists.jsm",
+  Promise: "resource://gre/modules/Promise.jsm",
+  RemoteSettings: "resource://services-settings/remote-settings.js",
+  RemoteSettingsClient: "resource://services-settings/RemoteSettingsClient.jsm",
+  sinon: "resource://testing-common/Sinon.jsm",
+});
+
+const IGNORELIST_KEY = "hijack-blocklists";
+
+const IGNORELIST_TEST_DATA = [
+  {
+    id: "load-paths",
+    matches: ["[other]addEngineWithDetails:searchignore@mozilla.com"],
+    _status: "synced",
+  },
+  {
+    id: "submission-urls",
+    matches: ["ignore=true"],
+    _status: "synced",
+  },
+];
+
+let getStub;
+
+add_task(async function setup() {
+  const ignoreListSettings = RemoteSettings(IGNORELIST_KEY);
+  getStub = sinon.stub(ignoreListSettings, "get");
+});
+
+add_task(async function test_ignoreList_basic_get() {
+  getStub.onFirstCall().returns(IGNORELIST_TEST_DATA);
+
+  const settings = await IgnoreLists.getAndSubscribe(() => {});
+
+  Assert.deepEqual(
+    settings,
+    IGNORELIST_TEST_DATA,
+    "Should have obtained the correct data from the database."
+  );
+});
+
+add_task(async function test_ignoreList_reentry() {
+  let promise = Promise.defer();
+  getStub.resetHistory();
+  getStub.onFirstCall().returns(promise.promise);
+
+  let firstResult;
+  let secondResult;
+
+  const firstCallPromise = IgnoreLists.getAndSubscribe(() => {}).then(
+    result => (firstResult = result)
+  );
+  const secondCallPromise = IgnoreLists.getAndSubscribe(() => {}).then(
+    result => (secondResult = result)
+  );
+
+  Assert.strictEqual(
+    firstResult,
+    undefined,
+    "Should not have returned the first result yet."
+  );
+  Assert.strictEqual(
+    secondResult,
+    undefined,
+    "Should not have returned the second result yet."
+  );
+
+  promise.resolve(IGNORELIST_TEST_DATA);
+
+  await Promise.all([firstCallPromise, secondCallPromise]);
+
+  Assert.deepEqual(
+    firstResult,
+    IGNORELIST_TEST_DATA,
+    "Should have returned the correct data to the first call."
+  );
+  Assert.deepEqual(
+    secondResult,
+    IGNORELIST_TEST_DATA,
+    "Should have returned the correct data to the second call."
+  );
+});
+
+add_task(async function test_ignoreList_updates() {
+  getStub.onFirstCall().returns([]);
+
+  let updateData;
+  let listener = eventData => {
+    updateData = eventData.data.current;
+  };
+
+  await IgnoreLists.getAndSubscribe(listener);
+
+  Assert.ok(!updateData, "Should not have given an update yet");
+
+  const NEW_DATA = [
+    {
+      id: "load-paths",
+      schema: 1553857697843,
+      last_modified: 1553859483588,
+      matches: ["[other]addEngineWithDetails:searchignore@mozilla.com"],
+    },
+    {
+      id: "submission-urls",
+      schema: 1553857697843,
+      last_modified: 1553859435500,
+      matches: ["ignore=true"],
+    },
+  ];
+
+  // Simulate an ignore list update.
+  await RemoteSettings("hijack-blocklists").emit("sync", {
+    data: {
+      current: NEW_DATA,
+    },
+  });
+
+  Assert.deepEqual(
+    updateData,
+    NEW_DATA,
+    "Should have updated the listener with the correct data"
+  );
+
+  IgnoreLists.unsubscribe(listener);
+
+  await RemoteSettings("hijack-blocklists").emit("sync", {
+    data: {
+      current: [
+        {
+          id: "load-paths",
+          schema: 1553857697843,
+          last_modified: 1553859483589,
+          matches: [],
+        },
+        {
+          id: "submission-urls",
+          schema: 1553857697843,
+          last_modified: 1553859435501,
+          matches: [],
+        },
+      ],
+    },
+  });
+
+  Assert.deepEqual(
+    updateData,
+    NEW_DATA,
+    "Should not have updated the listener"
+  );
+});
+
+add_task(async function test_ignoreList_db_modification() {
+  // Fill the database with some values that we can use to test that it is cleared.
+  const collection = await RemoteSettings(IGNORELIST_KEY).openCollection();
+  await collection.clear();
+  for (const data of IGNORELIST_TEST_DATA) {
+    await collection.create(
+      {
+        id: data.id,
+        matches: data.matches,
+      },
+      { synced: data._status == "synced" }
+    );
+  }
+  await collection.db.saveLastModified(42);
+
+  // Stub the get() so that the first call simulates a signature error, and
+  // the second simulates success reading from the dump.
+  getStub.resetHistory();
+  getStub
+    .onFirstCall()
+    .throws(new RemoteSettingsClient.InvalidSignatureError("abc"));
+  getStub.onSecondCall().returns(IGNORELIST_TEST_DATA);
+
+  let result = await IgnoreLists.getAndSubscribe(() => {});
+
+  Assert.ok(
+    getStub.calledTwice,
+    "Should have called the get() function twice."
+  );
+
+  const databaseEntries = await collection.list();
+  Assert.equal(
+    databaseEntries.data.length,
+    0,
+    "Should have cleared the database."
+  );
+
+  Assert.deepEqual(
+    result,
+    IGNORELIST_TEST_DATA,
+    "Should have returned the correct data."
+  );
+});

toolkit/modules/tests/xpcshell/xpcshell.ini

@@ -22,6 +22,7 @@ skip-if = toolkit == 'android'
 skip-if = toolkit == 'android'
 [test_Http.js]
 skip-if = toolkit == 'android'
+[test_IgnoreList.js]
 [test_Integration.js]
 [test_jsesc.js]
 skip-if = toolkit == 'android'