Bug 1510627 - Split RemoteSettingsClient#maybeSync() into smaller chunks of code r=glasserc
authorMathieu Leplatre <mathieu@mozilla.com>
Tue, 15 Jan 2019 11:44:54 +0000
changeset 511040 67ac015ae0802854815cc136baf520791588e754
parent 511039 97ae7728e2432a8cbd9c682b6eebf689ae69bdcc
child 511041 83d4a8e7b55138fed13f382c47f20fe760c05158
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglasserc
bugs1510627
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1510627 - Split RemoteSettingsClient#maybeSync() into smaller chunks of code r=glasserc Split RemoteSettingsClient#maybeSync() into smaller chunks of code Differential Revision: https://phabricator.services.mozilla.com/D16468
services/settings/RemoteSettingsClient.jsm
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -328,99 +328,51 @@ class RemoteSettingsClient extends Event
         const { ok } = syncResult;
         if (!ok) {
           // Some synchronization conflicts occured.
           reportStatus = UptakeTelemetry.STATUS.CONFLICT_ERROR;
           throw new Error("Sync failed");
         }
       } catch (e) {
         if (e.message.includes(INVALID_SIGNATURE)) {
-          // Signature verification failed during synchronzation.
+          // Signature verification failed during synchronization.
           reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR;
-          // if sync fails with a signature error, it's likely that our
+          // If sync fails with a signature error, it's likely that our
           // local data has been modified in some way.
           // We will attempt to fix this by retrieving the whole
           // remote collection.
-          const payload = await fetchRemoteRecords(collection.bucket, collection.name, expectedTimestamp);
           try {
-            await this._validateCollectionSignature(payload.data,
-                                                    payload.last_modified,
-                                                    collection,
-                                                    { expectedTimestamp, ignoreLocal: true });
+            syncResult = await this._retrySyncFromScratch(collection, expectedTimestamp);
           } catch (e) {
+            // If the signature fails again, or if an error occured during wiping out the
+            // local data, then we report it as a *signature retry* error.
             reportStatus = UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR;
             throw e;
           }
-
-          // The signature is good (we haven't thrown).
-          // Now we will Inspect what we had locally.
-          const { data: oldData } = await collection.list({ order: "" }); // no need to sort.
-
-          // We build a sync result as if a diff-based sync was performed.
-          syncResult = { created: [], updated: [], deleted: [] };
-
-          // If the remote last_modified is newer than the local last_modified,
-          // replace the local data
-          const localLastModified = await collection.db.getLastModified();
-          if (payload.last_modified >= localLastModified) {
-            const { data: newData } = payload;
-            await collection.clear();
-            await collection.loadDump(newData);
-
-            // Compare local and remote to populate the sync result
-            const oldById = new Map(oldData.map(e => [e.id, e]));
-            for (const r of newData) {
-              const old = oldById.get(r.id);
-              if (old) {
-                if (old.last_modified != r.last_modified) {
-                  syncResult.updated.push({ old, new: r });
-                }
-                oldById.delete(r.id);
-              } else {
-                syncResult.created.push(r);
-              }
-            }
-            // Records that remain in our map now are those missing from remote
-            syncResult.deleted = Array.from(oldById.values());
-          }
-
         } else {
           // The sync has thrown, it can be related to metadata, network or a general error.
           if (e.message == MISSING_SIGNATURE) {
             // Collection metadata has no signature info, no need to retry.
             reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR;
           } else if (/NetworkError/.test(e.message)) {
             reportStatus = UptakeTelemetry.STATUS.NETWORK_ERROR;
           } else if (/Backoff/.test(e.message)) {
             reportStatus = UptakeTelemetry.STATUS.BACKOFF;
           } else {
             reportStatus = UptakeTelemetry.STATUS.SYNC_ERROR;
           }
           throw e;
         }
       }
 
-      // Handle the obtained records (ie. apply locally through events).
-      // Build the event data list. It should be filtered (ie. by application target)
-      const { created: allCreated, updated: allUpdated, deleted: allDeleted } = syncResult;
-      const [created, deleted, updatedFiltered] = await Promise.all(
-          [allCreated, allDeleted, allUpdated.map(e => e.new)].map(this._filterEntries.bind(this))
-        );
-      // For updates, keep entries whose updated form matches the target.
-      const updatedFilteredIds = new Set(updatedFiltered.map(e => e.id));
-      const updated = allUpdated.filter(({ new: { id } }) => updatedFilteredIds.has(id));
-
+      const filteredSyncResult = await this._filterSyncResult(collection, syncResult);
       // If every changed entry is filtered, we don't even fire the event.
-      if (created.length || updated.length || deleted.length) {
-        // Read local collection of records (also filtered).
-        const { data: allData } = await collection.list({ order: "" }); // no need to sort.
-        const current = await this._filterEntries(allData);
-        const payload = { data: { current, created, updated, deleted } };
+      if (filteredSyncResult) {
         try {
-          await this.emit("sync", payload);
+          await this.emit("sync", { data: filteredSyncResult });
         } catch (e) {
           reportStatus = UptakeTelemetry.STATUS.APPLY_ERROR;
           throw e;
         }
       }
 
     } catch (e) {
       // No specific error was tracked, mark it as unknown.
@@ -439,17 +391,17 @@ class RemoteSettingsClient extends Event
   }
 
   /**
    * Fetch the signature info from the collection metadata and verifies that the
    * local set of records has the same.
    *
    * @param {Array<Object>} remoteRecords   The list of changes to apply to the local database.
    * @param {int} timestamp                 The timestamp associated with the list of remote records.
-   * @param {Collection} collection         Kinto.js Collection instance.
+   * @param {Collection} kintoCollection    Kinto.js Collection instance.
    * @param {Object} options
    * @param {int} options.expectedTimestamp Cache busting of collection metadata
    * @param {Boolean} options.ignoreLocal   When the signature verification is retried, since we refetch
    *                                        the whole collection, we don't take into account the local
    *                                        data (default: `false`)
    * @returns {Promise}
    */
   async _validateCollectionSignature(remoteRecords, timestamp, kintoCollection, options = {}) {
@@ -474,16 +426,100 @@ class RemoteSettingsClient extends Event
                                          "p384ecdsa=" + signature,
                                          certChain,
                                          this.signerName)) {
       throw new Error(INVALID_SIGNATURE + ` (${bucket}/${collection})`);
     }
   }
 
   /**
+   * Fetch the whole list of records from the server, verify the signature again
+   * and then compute a synchronization result as if the diff-based sync happened.
+   * And eventually, wipe out the local data.
+   *
+   * @param {Collection} kintoCollection    Kinto.js Collection instance.
+   * @param {int}        expectedTimestamp  Cache busting of collection metadata
+   *
+   * @returns {Promise<Object>} the computed sync result.
+   */
+  async _retrySyncFromScratch(kintoCollection, expectedTimestamp) {
+    const payload = await fetchRemoteRecords(kintoCollection.bucket, kintoCollection.name, expectedTimestamp);
+    await this._validateCollectionSignature(payload.data,
+      payload.last_modified,
+      kintoCollection,
+      { expectedTimestamp, ignoreLocal: true });
+
+    // The signature is good (we haven't thrown).
+    // Now we will Inspect what we had locally.
+    const { data: oldData } = await kintoCollection.list({ order: "" }); // no need to sort.
+
+    // We build a sync result as if a diff-based sync was performed.
+    const syncResult = { created: [], updated: [], deleted: [] };
+
+    // If the remote last_modified is newer than the local last_modified,
+    // replace the local data
+    const localLastModified = await kintoCollection.db.getLastModified();
+    if (payload.last_modified >= localLastModified) {
+      const { data: newData } = payload;
+      await kintoCollection.clear();
+      await kintoCollection.loadDump(newData);
+
+      // Compare local and remote to populate the sync result
+      const oldById = new Map(oldData.map(e => [e.id, e]));
+      for (const r of newData) {
+        const old = oldById.get(r.id);
+        if (old) {
+          if (old.last_modified != r.last_modified) {
+            syncResult.updated.push({ old, new: r });
+          }
+          oldById.delete(r.id);
+        } else {
+          syncResult.created.push(r);
+        }
+      }
+      // Records that remain in our map now are those missing from remote
+      syncResult.deleted = Array.from(oldById.values());
+    }
+    return syncResult;
+  }
+
+  /**
+   * Use the filter func to filter the lists of changes obtained from synchronization,
+   * and return them along with the filtered list of local records.
+   *
+   * If the filtered lists of changes are all empty, we return null (and thus don't
+   * bother listing local DB).
+   *
+   * @param {Collection} kintoCollection  Kinto.js Collection instance.
+   * @param {Object}     syncResult       Synchronization result without filtering.
+   *
+   * @returns {Promise<Object>} the filtered list of local records, plus the filtered
+   *                            list of created, updated and deleted records.
+   */
+  async _filterSyncResult(kintoCollection, syncResult) {
+    // Handle the obtained records (ie. apply locally through events).
+    // Build the event data list. It should be filtered (ie. by application target)
+    const { created: allCreated, updated: allUpdated, deleted: allDeleted } = syncResult;
+    const [created, deleted, updatedFiltered] = await Promise.all(
+      [allCreated, allDeleted, allUpdated.map(e => e.new)].map(this._filterEntries.bind(this))
+    );
+    // For updates, keep entries whose updated form matches the target.
+    const updatedFilteredIds = new Set(updatedFiltered.map(e => e.id));
+    const updated = allUpdated.filter(({ new: { id } }) => updatedFilteredIds.has(id));
+
+    if (!created.length && !updated.length && !deleted.length) {
+      return null;
+    }
+    // Read local collection of records (also filtered).
+    const { data: allData } = await kintoCollection.list({ order: "" }); // no need to sort.
+    const current = await this._filterEntries(allData);
+    return { created, updated, deleted, current };
+  }
+
+  /**
    * Filter entries for which calls to `this.filterFunc` returns null.
    *
    * @param {Array<Objet>} data
    * @returns {Array<Object>}
    */
   async _filterEntries(data) {
     if (!this.filterFunc) {
       return data;