Bug 1653988 - Extend the loading of dump fallback for Remote Settings to cover more indexedDB failure areas. r=leplatrem

This extends the current try/catch to cover writing of the dump to the database and reading it back again. It does not cover the verify signature section, as it is expected that it will need to be known if the signature fails.

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

services/settings/RemoteSettingsClient.jsm

@@ -348,9 +348,45 @@ class RemoteSettingsClient extends EventEmitter {
     } = options;
     let { verifySignature = false } = options;
 
-    let hasLocalData;
+    let data;
     try {
-      hasLocalData = await Utils.hasLocalData(this);
+      let hasLocalData = await Utils.hasLocalData(this);
+
+      if (syncIfEmpty && !hasLocalData) {
+        // .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 {
+          await this._importingPromise;
+        } catch (e) {
+          // Report error, but continue because there could have been data
+          // loaded from a parrallel call.
+          Cu.reportError(e);
+        } 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.
+      data = await this.db.list({ filters, order });
     } catch (e) {
       // If the local DB cannot be read (for unknown reasons, Bug 1649393)
       // We fallback to the packaged data, and filter/sort in memory.
@@ -380,41 +416,6 @@ class RemoteSettingsClient extends EventEmitter {
       return this._filterEntries(data);
     }
 
-    if (syncIfEmpty && !hasLocalData) {
-      // .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 {
-        await this._importingPromise;
-      } catch (e) {
-        // Report error, but continue because there could have been data
-        // loaded from a parrallel call.
-        Cu.reportError(e);
-      } 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.
-    const data = await this.db.list({ filters, order });
     console.debug(
       `${this.identifier} ${data.length} records before filtering.`
     );

services/settings/test/unit/test_remote_settings.js

@@ -286,7 +286,7 @@ add_task(async function test_get_falls_back_to_dump_if_db_fails() {
     throw new Error("Unknown error");
   };
 
-  const records = await clientWithDump.get();
+  const records = await clientWithDump.get({ dumpFallback: true });
   ok(records.length > 0, "dump content is returned");
 
   // If fallback is disabled, error is thrown.
@@ -302,6 +302,32 @@ add_task(async function test_get_falls_back_to_dump_if_db_fails() {
 });
 add_task(clear_state);
 
+add_task(async function test_get_falls_back_to_dump_if_db_fails_later() {
+  if (IS_ANDROID) {
+    // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
+    return;
+  }
+  const backup = clientWithDump.db.list;
+  clientWithDump.db.list = () => {
+    throw new Error("Unknown error");
+  };
+
+  const records = await clientWithDump.get({ dumpFallback: true });
+  ok(records.length > 0, "dump content is returned");
+
+  // If fallback is disabled, error is thrown.
+  let error;
+  try {
+    await clientWithDump.get({ dumpFallback: false });
+  } catch (e) {
+    error = e;
+  }
+  equal(error.message, "Unknown error");
+
+  clientWithDump.db.list = 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).