Bug 1458920 - Filter RemoteSettings sync event data r=Gijs,mgoodwin
authorMathieu Leplatre <mathieu@mozilla.com>
Tue, 08 May 2018 16:30:40 +0200
changeset 417835 cfba5652a29cc056bf3ce25e765a922abc7581a4
parent 417834 ce79538855b7e2f5b0f378865fc7878885e27c37
child 417836 1591f55d3e32fc488133d3ea9e7a2a3163066663
push id33980
push userebalazs@mozilla.com
push dateFri, 11 May 2018 09:35:12 +0000
treeherdermozilla-central@8e9a4a323f0c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, mgoodwin
bugs1458920
milestone62.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 1458920 - Filter RemoteSettings sync event data r=Gijs,mgoodwin MozReview-Commit-ID: Hw9CA5W2J26
services/common/docs/RemoteSettings.rst
services/common/remote-settings.js
services/common/tests/unit/test_blocklist_clients.js
services/common/tests/unit/test_blocklist_signatures.js
--- a/services/common/docs/RemoteSettings.rst
+++ b/services/common/docs/RemoteSettings.rst
@@ -63,16 +63,19 @@ The ``sync`` event allows to be notified
     RemoteSettings("a-key").on("sync", event => {
       const { data: { current } } = event;
       for(const entry of current) {
         // Do something with entry...
         // await InternalAPI.reload(entry.id, entry.label, entry.weight);
       }
     });
 
+.. important::
+    If one of the event handler fails, the others handlers for the same remote settings collection won't be executed.
+
 .. note::
     Currently, the update of remote settings is triggered by the `nsBlocklistService <https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js>`_ (~ every 24H).
 
 File attachments
 ----------------
 
 When an entry has a file attached to it, it has an ``attachment`` attribute, which contains the file related information (url, hash, size, mimetype, etc.). Remote files are not downloaded automatically.
 
--- a/services/common/remote-settings.js
+++ b/services/common/remote-settings.js
@@ -342,32 +342,44 @@ class RemoteSettingsClient {
           } else if (/Backoff/.test(e.message)) {
             reportStatus = UptakeTelemetry.STATUS.BACKOFF;
           } else {
             reportStatus = UptakeTelemetry.STATUS.SYNC_ERROR;
           }
           throw e;
         }
       }
-      // Read local collection of records.
-      const { data: current } = await collection.list();
 
-      // Handle the obtained records (ie. apply locally).
-      try {
-        // Execute callbacks in order and sequentially.
+      // 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 is matches the target.
+      const updatedFilteredIds = new Set(updatedFiltered.map(e => e.id));
+      const updated = allUpdated.filter(({ new: { id } }) => updatedFilteredIds.has(id));
+
+      // 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();
+        const current = await this._filterEntries(allData);
+        // Fire the event: execute callbacks in order and sequentially.
         // If one fails everything fails.
-        const { created, updated, deleted } = syncResult;
         const event = { data: { current, created, updated, deleted } };
         const callbacks = this._callbacks.get("sync");
-        for (const cb of callbacks) {
-          await cb(event);
+        try {
+          for (const cb of callbacks) {
+            await cb(event);
+          }
+        } catch (e) {
+          reportStatus = UptakeTelemetry.STATUS.APPLY_ERROR;
+          throw e;
         }
-      } catch (e) {
-        reportStatus = UptakeTelemetry.STATUS.APPLY_ERROR;
-        throw e;
       }
 
       // Track last update.
       this._updateLastCheck(serverTime);
 
     } catch (e) {
       // No specific error was tracked, mark it as unknown.
       if (reportStatus === null) {
--- a/services/common/tests/unit/test_blocklist_clients.js
+++ b/services/common/tests/unit/test_blocklist_clients.js
@@ -170,16 +170,55 @@ add_task(async function test_sends_reloa
       client.maybeSync(2000, Date.now() - 1000, {loadDump: false});
     });
 
     equal(received.data.filename, client.filename);
   }
 });
 add_task(clear_state);
 
+add_task(async function test_sync_event_data_is_filtered_for_target() {
+  // Here we will synchronize 4 times, the first two to initialize the local DB and
+  // the last two about event filtered data.
+  const timestamp1 = 2000;
+  const timestamp2 = 3000;
+  const timestamp3 = 4000;
+  const timestamp4 = 5000;
+  // Fake a date value obtained from server (used to store a pref, useless here).
+  const fakeServerTime = Date.now();
+
+  for (let {client} of gBlocklistClients) {
+    // Initialize the collection with some data (local is empty, thus no ?_since)
+    await client.maybeSync(timestamp1, fakeServerTime - 30, {loadDump: false});
+    // This will pick the data with ?_since=3000.
+    await client.maybeSync(timestamp2 + 1, fakeServerTime - 20);
+
+    // In ?_since=4000 entries, no target matches. The sync event is not called.
+    let called = false;
+    client.on("sync", e => called = true);
+    await client.maybeSync(timestamp3 + 1, fakeServerTime - 10);
+    equal(called, false, `no sync event for ${client.collectionName}`);
+
+    // In ?_since=5000 entries, only one entry matches.
+    let syncEventData;
+    client.on("sync", e => syncEventData = e.data);
+    await client.maybeSync(timestamp4 + 1, fakeServerTime);
+    const { current, created, updated, deleted } = syncEventData;
+    equal(created.length + updated.length + deleted.length, 1, `event filtered data for ${client.collectionName}`);
+
+    // Since we had entries whose target does not match, the internal storage list
+    // and the event current data should differ.
+    const collection = await client.openCollection();
+    const { data: internalData } = await collection.list();
+    ok(internalData.length > current.length, `event current data for ${client.collectionName}`);
+  }
+});
+add_task(clear_state);
+
+
 // get a response for a given request from sample data
 function getSampleResponse(req, port) {
   const responses = {
     "OPTIONS": {
       "sampleHeaders": [
         "Access-Control-Allow-Headers: Content-Length,Expires,Backoff,Retry-After,Last-Modified,Total-Records,ETag,Pragma,Cache-Control,authorization,content-type,if-none-match,Alert,Next-Page",
         "Access-Control-Allow-Methods: GET,HEAD,OPTIONS,POST,DELETE,OPTIONS",
         "Access-Control-Allow-Origin: *",
@@ -317,31 +356,30 @@ function getSampleResponse(req, port) {
       "responseBody": JSON.stringify({"data": [{
         "infoURL": "https://get.adobe.com/flashplayer/",
         "blockID": "p1044",
         "matchFilename": "libflashplayer\\.so",
         "last_modified": 4000,
         "versionRange": [{
           "targetApplication": [],
           "minVersion": "11.2.202.509",
-          "maxVersion": "11.2.202.539",
+          "maxVersion": "*",
           "severity": "0",
           "vulnerabilityStatus": "1"
         }],
         "os": "Linux",
         "id": "aabad965-e556-ffe7-4191-074f5dee3df3"
       }, {
         "matchFilename": "npViewpoint.dll",
         "blockID": "p32",
         "id": "1f48af42-c508-b8ef-b8d5-609d48e4f6c9",
         "last_modified": 3500,
         "versionRange": [{
           "targetApplication": [{
             "minVersion": "3.0",
-            "guid": "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}",
             "maxVersion": "*"
           }]
         }]
       }]})
     },
     "GET:/v1/buckets/blocklists/collections/gfx/records?_sort=-last_modified&_since=3000": {
       "sampleHeaders": [
         "Access-Control-Allow-Origin: *",
@@ -365,15 +403,172 @@ function getSampleResponse(req, port) {
         "blockID": "g200",
         "feature": "WEBGL_MSAA",
         "devices": [],
         "id": "c3a15ba9-e0e2-421f-e399-c995e5b8d14e",
         "last_modified": 3500,
         "os": "Darwin 11",
         "featureStatus": "BLOCKED_DEVICE"
       }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/addons/records?_sort=-last_modified&_since=4000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"5000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        "last_modified": 4001,
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "some-guid"
+          }],
+        }],
+        "id": "8f03b264-57b7-4263-9b15-ad91b033a034"
+      }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/plugins/records?_sort=-last_modified&_since=4000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"5000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        "last_modified": 4001,
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "0",
+            "maxVersion": "57.*"
+          }]
+        }],
+        "id": "cd3ea0b2-1ba8-4fb6-b242-976a87626116"
+      }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/gfx/records?_sort=-last_modified&_since=4000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"5000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        "last_modified": 4001,
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "99999"
+          }],
+        }],
+        "id": "86771771-e803-4006-95e9-c9275d58b3d1"
+      }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/addons/records?_sort=-last_modified&_since=5000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"6000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        // delete an entry with non matching target (see above)
+        "last_modified": 5001,
+        "deleted": true,
+        "id": "8f03b264-57b7-4263-9b15-ad91b033a034"
+      }, {
+        // delete entry with matching target (see above)
+        "last_modified": 5002,
+        "deleted": true,
+        "id": "9ccfac91-e463-c30c-f0bd-14143794a8dd"
+      }, {
+        // create an extra non matching
+        "last_modified": 5003,
+        "id": "75b36589-435a-48d4-8ee4-bacee3fb6119",
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "0",
+            "maxVersion": "57.*"
+          }]
+        }],
+      }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/plugins/records?_sort=-last_modified&_since=5000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"6000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        // entry with non matching target (see above)
+        "newAttribute": 42,
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "0",
+            "maxVersion": "57.*"
+          }]
+        }],
+        "id": "cd3ea0b2-1ba8-4fb6-b242-976a87626116"
+      }, {
+        // entry with matching target (see above)
+        "newAttribute": 42,
+        "matchFilename": "npViewpoint.dll",
+        "blockID": "p32",
+        "id": "1f48af42-c508-b8ef-b8d5-609d48e4f6c9",
+        "last_modified": 3500,
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "3.0",
+            "maxVersion": "*"
+          }]
+        }]
+      }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/gfx/records?_sort=-last_modified&_since=5000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"6000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data": [{
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "0",
+            "maxVersion": "*"
+          }]
+        }],
+        "id": "43031a81-5f36-4eef-9b35-52f0bbeba363"
+      }, {
+        "versionRange": [{
+          "targetApplication": [{
+            "guid": "xpcshell@tests.mozilla.org",
+            "minVersion": "0",
+            "maxVersion": "3"
+          }]
+        }],
+        "id": "75a06bd3-f906-427d-a448-02092ee589fc"
+      }]})
     }
   };
   return responses[`${req.method}:${req.path}?${req.queryString}`] ||
          responses[`${req.method}:${req.path}`] ||
          responses[req.method];
 
 }
--- a/services/common/tests/unit/test_blocklist_signatures.js
+++ b/services/common/tests/unit/test_blocklist_signatures.js
@@ -444,29 +444,26 @@ add_task(async function test_check_signa
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=id":
       [RESPONSE_COMPLETE_INITIAL_SORTED_BY_ID]
   };
 
   registerHandlers(badSigGoodSigResponses);
 
   startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
 
-  let retrySyncData;
-  OneCRLBlocklistClient.on("sync", ({ data }) => { retrySyncData = data; });
+  let syncEventSent = false;
+  OneCRLBlocklistClient.on("sync", ({ data }) => { syncEventSent = true; });
 
   await OneCRLBlocklistClient.maybeSync(5000, startTime);
 
   endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
 
-  // since we only fixed the signature, and no data was changed, the sync result
-  // will be called with empty lists of created/updated/deleted.
-  equal(retrySyncData.current.length, 2);
-  equal(retrySyncData.created.length, 0);
-  equal(retrySyncData.updated.length, 0);
-  equal(retrySyncData.deleted.length, 0);
+  // since we only fixed the signature, and no data was changed, the sync event
+  // was not sent.
+  equal(syncEventSent, false);
 
   // ensure that the failure count is incremented for a succesful sync with an
   // (initial) bad signature - only SERVICES_SETTINGS_SYNC_SIG_FAIL should
   // increment.
   expectedIncrements = {[UptakeTelemetry.STATUS.SIGNATURE_ERROR]: 1};
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 
 
@@ -488,27 +485,24 @@ add_task(async function test_check_signa
       [RESPONSE_EMPTY_INITIAL],
   };
 
   // ensure our collection hasn't been replaced with an older, empty one
   await checkRecordCount(OneCRLBlocklistClient, 2);
 
   registerHandlers(badSigGoodOldResponses);
 
-  let oldChangesData;
-  OneCRLBlocklistClient.on("sync", ({ data }) => { oldChangesData = data; });
+  syncEventSent = false;
+  OneCRLBlocklistClient.on("sync", ({ data }) => { syncEventSent = true; });
 
   await OneCRLBlocklistClient.maybeSync(5000, startTime);
 
-  // Local data was unchanged, since it was never than the one returned by the server.
-  equal(oldChangesData.current.length, 2);
-  equal(oldChangesData.created.length, 0);
-  equal(oldChangesData.updated.length, 0);
-  equal(oldChangesData.deleted.length, 0);
-
+  // Local data was unchanged, since it was never than the one returned by the server,
+  // thus the sync event is not sent.
+  equal(syncEventSent, false);
 
   const badLocalContentGoodSigResponses = {
     // In this test, we deliberately serve a bad signature initially. The
     // subsequent signature returned is a valid one for the three item
     // collection.
     "GET:/v1/buckets/blocklists/collections/certificates?":
       [RESPONSE_META_BAD_SIG, RESPONSE_META_THREE_ITEMS_SIG],
     // The next request is for the full collection. This will be checked