Bug 1646315 - Prevent concurrent load of JSON dump with .get() r=Gijs

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

services/settings/RemoteSettingsClient.jsm

@@ -342,24 +342,36 @@ class RemoteSettingsClient extends EventEmitter {
     let { verifySignature = false } = options;
 
     if (syncIfEmpty && !(await Utils.hasLocalData(this))) {
+      // .get() was called before we had the chance to synchronize the local database.
+      // We'll try to avoid returning an empty list.
+      if (!this._importingPromise) {
+        // Prevent parallel loading when .get() is called multiple times.
+        this._importingPromise = (async () => {
+          const importedFromDump = gLoadDump
+            ? await this._importJSONDump()
+            : -1;
+          if (importedFromDump < 0) {
+            // There is no JSON dump to load, force a synchronization from the server.
+            console.debug(
+              `${this.identifier} Local DB is empty, pull data from server`
+            );
+            await this.sync({ loadDump: false });
+          }
+        })();
+      }
       try {
-        // .get() was called before we had the chance to synchronize the local database.
-        // We'll try to avoid returning an empty list.
-        const importedFromDump = gLoadDump ? await this._importJSONDump() : -1;
-        if (importedFromDump < 0) {
-          // There is no JSON dump to load, force a synchronization from the server.
-          console.debug(
-            `${this.identifier} Local DB is empty, pull data from server`
-          );
-          await this.sync({ loadDump: false });
-        }
-        // Either from trusted dump, or already done during sync.
-        verifySignature = false;
+        await this._importingPromise;
       } catch (e) {
         // Report but return an empty list since there will be no data anyway.
         Cu.reportError(e);
         return [];
+      } finally {
+        // then delete this promise again, as now we should have local data:
+        delete this._importingPromise;
       }
+      // No need to verify signature, because either we've just load a trusted
+      // dump (here or in a parallel call), or it was verified during sync.
+      verifySignature = false;
     }
 
     // Read from the local DB.

services/settings/test/unit/test_remote_settings.js

@@ -9,8 +9,7 @@ const { AppConstants } = ChromeUtils.import(
 const { ObjectUtils } = ChromeUtils.import(
   "resource://gre/modules/ObjectUtils.jsm"
 );
-
-const IS_ANDROID = AppConstants.platform == "android";
+const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm");
 
 const { RemoteSettings } = ChromeUtils.import(
   "resource://services-settings/remote-settings.js"
@@ -23,6 +22,8 @@ const { TelemetryTestUtils } = ChromeUtils.import(
   "resource://testing-common/TelemetryTestUtils.jsm"
 );
 
+const IS_ANDROID = AppConstants.platform == "android";
+
 const BinaryInputStream = CC(
   "@mozilla.org/binaryinputstream;1",
   "nsIBinaryInputStream",
@@ -257,6 +258,21 @@ add_task(async function test_get_does_not_load_dump_when_pref_is_false() {
 });
 add_task(clear_state);
 
+add_task(async function test_get_loads_dump_only_once_if_called_in_parallel() {
+  const backup = clientWithDump._importJSONDump;
+  let callCount = 0;
+  clientWithDump._importJSONDump = async () => {
+    callCount++;
+    // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+    await new Promise(resolve => setTimeout(resolve, 100));
+    return 42;
+  };
+  await Promise.all([clientWithDump.get(), clientWithDump.get()]);
+  equal(callCount, 1, "JSON dump was called more than once");
+  clientWithDump._importJSONDump = backup;
+});
+add_task(clear_state);
+
 add_task(async function test_get_does_not_sync_if_empty_dump_is_provided() {
   if (IS_ANDROID) {
     // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
@@ -411,6 +427,26 @@ add_task(async function test_get_does_not_verify_signature_if_load_dump() {
 });
 add_task(clear_state);
 
+add_task(
+  async function test_get_does_verify_signature_if_json_loaded_in_parallel() {
+    const backup = clientWithDump._verifier;
+    let callCount = 0;
+    clientWithDump._verifier = {
+      async asyncVerifyContentSignature(serialized, signature) {
+        callCount++;
+        return true;
+      },
+    };
+    await Promise.all([
+      clientWithDump.get({ verifySignature: true }),
+      clientWithDump.get({ verifySignature: true }),
+    ]);
+    equal(callCount, 0, "No need to verify signatures if JSON dump is loaded");
+    clientWithDump._verifier = backup;
+  }
+);
+add_task(clear_state);
+
 add_task(async function test_sync_runs_once_only() {
   const backup = Utils.log.warn;
   const messages = [];