Bug 1463782 - allow toolkit apps in kinto blocklist, r?leplatrem draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 23 May 2018 18:01:36 +0100
changeset 799001 8029328a06c1061b4c7b168daf13ab7c8c79b02d
parent 798691 d36cd8bdbc5c0df1d1d7a167f5fedb95c3a3648e
push id110922
push userbmo:gijskruitbosch+bugs@gmail.com
push dateWed, 23 May 2018 22:35:26 +0000
reviewersleplatrem
bugs1463782
milestone62.0a1
Bug 1463782 - allow toolkit apps in kinto blocklist, r?leplatrem MozReview-Commit-ID: G1uqNw1Njni
services/common/blocklist-clients.js
services/common/remote-settings.js
services/common/tests/unit/test_blocklist_clients.js
services/common/tests/unit/test_blocklist_targetapp_filter.js
--- a/services/common/blocklist-clients.js
+++ b/services/common/blocklist-clients.js
@@ -142,24 +142,27 @@ async function targetAppFilter(entry, en
     return jexlFilterFunc(entry, environment);
   }
 
   // Keep entries without target information.
   if (!("versionRange" in entry)) {
     return entry;
   }
 
-  const { appID, version: appVersion } = environment;
+  const { appID, version: appVersion, toolkitVersion } = environment;
   const { versionRange } = entry;
 
+  // Everywhere in this method, we avoid checking the minVersion, because
+  // we want to retain items whose minVersion is higher than the current
+  // app version, so that we have the items around for app updates.
+
   // Gfx blocklist has a specific versionRange object, which is not a list.
   if (!Array.isArray(versionRange)) {
-    const { minVersion = "0", maxVersion = "*" } = versionRange;
-    const matchesRange = (Services.vc.compare(appVersion, minVersion) >= 0 &&
-                          Services.vc.compare(appVersion, maxVersion) <= 0);
+    const { maxVersion = "*" } = versionRange;
+    const matchesRange = (Services.vc.compare(appVersion, maxVersion) <= 0);
     return matchesRange ? entry : null;
   }
 
   // Iterate the targeted applications, at least one of them must match.
   // If no target application, keep the entry.
   if (versionRange.length == 0) {
     return entry;
   }
@@ -168,22 +171,25 @@ async function targetAppFilter(entry, en
     if (targetApplication.length == 0) {
       return entry;
     }
     for (const ta of targetApplication) {
       const { guid } = ta;
       if (!guid) {
         return entry;
       }
-      const { minVersion = "0", maxVersion = "*" } = ta;
+      const { maxVersion = "*" } = ta;
       if (guid == appID &&
-          Services.vc.compare(appVersion, minVersion) >= 0 &&
           Services.vc.compare(appVersion, maxVersion) <= 0) {
         return entry;
       }
+      if (guid == "toolkit@mozilla.org" &&
+          Services.vc.compare(toolkitVersion, maxVersion) <= 0) {
+        return entry;
+      }
     }
   }
   // Skip this entry.
   return null;
 }
 
 var AddonBlocklistClient;
 var GfxBlocklistClient;
--- a/services/common/remote-settings.js
+++ b/services/common/remote-settings.js
@@ -59,16 +59,21 @@ function cacheProxy(target) {
 }
 
 class ClientEnvironment extends ClientEnvironmentBase {
   static get appID() {
     // eg. Firefox is "{ec8030f7-c20a-464f-9b0e-13a3a9e97384}".
     Services.appinfo.QueryInterface(Ci.nsIXULAppInfo);
     return Services.appinfo.ID;
   }
+
+  static get toolkitVersion() {
+    Services.appinfo.QueryInterface(Ci.nsIPlatformInfo);
+    return Services.appinfo.platformVersion;
+  }
 }
 
 /**
  * Default entry filtering function, in charge of excluding remote settings entries
  * where the JEXL expression evaluates into a falsy value.
  */
 async function jexlFilterFunc(entry, environment) {
   const { filters } = entry;
--- a/services/common/tests/unit/test_blocklist_clients.js
+++ b/services/common/tests/unit/test_blocklist_clients.js
@@ -222,17 +222,17 @@ add_task(async function test_sync_event_
     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}`);
+    equal(called, false, `shouldn't have 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}`);
 
@@ -541,17 +541,17 @@ function getSampleResponse(req, port) {
         "Etag: \"5000\""
       ],
       "status": {status: 200, statusText: "OK"},
       "responseBody": JSON.stringify({"data": [{
         "last_modified": 4001,
         "versionRange": [{
           "targetApplication": [{
             "guid": "xpcshell@tests.mozilla.org",
-            "minVersion": "99999"
+            "maxVersion": "20"
           }],
         }],
         "id": "86771771-e803-4006-95e9-c9275d58b3d1"
       }]})
     },
     "GET:/v1/buckets/blocklists/collections/addons/records?_sort=-last_modified&_since=5000": {
       "sampleHeaders": [
         "Access-Control-Allow-Origin: *",
--- a/services/common/tests/unit/test_blocklist_targetapp_filter.js
+++ b/services/common/tests/unit/test_blocklist_targetapp_filter.js
@@ -1,12 +1,13 @@
 const BlocklistClients = ChromeUtils.import("resource://services-common/blocklist-clients.js", {});
 const { RemoteSettings } = ChromeUtils.import("resource://services-common/remote-settings.js", {});
 
 const APP_ID = "xpcshell@tests.mozilla.org";
+const TOOLKIT_ID = "toolkit@mozilla.org";
 
 let client;
 
 async function clear_state() {
   // Clear local DB.
   const collection = await client.openCollection();
   await collection.clear();
 }
@@ -79,20 +80,27 @@ add_task(async function test_returns_wit
     }]
   }, {
     willMatch: true,
     versionRange: [{
       targetApplication: [{
         guid: APP_ID,
       }]
     }]
+  }, {
+    willMatch: true,
+    versionRange: [{
+      targetApplication: [{
+        guid: TOOLKIT_ID,
+      }]
+    }]
   }]);
 
   const list = await client.get();
-  equal(list.length, 2);
+  equal(list.length, 3);
   ok(list.every(e => e.willMatch));
 });
 add_task(clear_state);
 
 add_task(async function test_returns_without_app_version_or_with_matching_version() {
   await createRecords([{
     willMatch: true,
     versionRange: [{
@@ -121,20 +129,39 @@ add_task(async function test_returns_wit
     willMatch: false,
     versionRange: [{
       targetApplication: [{
         guid: APP_ID,
         minVersion: "0",
         maxVersion: "1",
       }]
     }]
+  }, {
+    willMatch: true,
+    versionRange: [{
+      targetApplication: [{
+        guid: TOOLKIT_ID,
+        minVersion: "0",
+      }]
+    }]
+  }, {
+    willMatch: true,
+    versionRange: [{
+      targetApplication: [{
+        guid: TOOLKIT_ID,
+        minVersion: "0",
+        maxVersion: "9999",
+      }]
+    }]
+    // We can't test the false case with maxVersion for toolkit, because the toolkit version
+    // is 0 in xpcshell.
   }]);
 
   const list = await client.get();
-  equal(list.length, 3);
+  equal(list.length, 5);
   ok(list.every(e => e.willMatch));
 });
 add_task(clear_state);
 
 add_task(async function test_multiple_version_and_target_applications() {
   await createRecords([{
     willMatch: true,
     versionRange: [{