Bug 1454378 - cache and inline things, avoid duplicate attribute or property requests, to make blocklist faster, r?florian draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 13 Jun 2018 17:06:49 -0700
changeset 810884 2d58c4a8e5820eeff80f6f626897bd121fd7f25a
parent 810883 82026d8bc8cea378e2d5740f7699b581f93bebf7
child 810885 cc7b04c1d78bf1884e210c2103a286d47e8c7c55
push id114146
push userbmo:gijskruitbosch+bugs@gmail.com
push dateTue, 26 Jun 2018 17:15:11 +0000
reviewersflorian
bugs1454378
milestone63.0a1
Bug 1454378 - cache and inline things, avoid duplicate attribute or property requests, to make blocklist faster, r?florian MozReview-Commit-ID: BwBhZr6sqx2
toolkit/mozapps/extensions/Blocklist.jsm
toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
--- a/toolkit/mozapps/extensions/Blocklist.jsm
+++ b/toolkit/mozapps/extensions/Blocklist.jsm
@@ -176,16 +176,24 @@ XPCOMUtils.defineLazyGetter(this, "gApp"
     // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
     if (!(ex instanceof Components.Exception) ||
         ex.result != Cr.NS_NOINTERFACE)
       throw ex;
   }
   return appinfo;
 });
 
+XPCOMUtils.defineLazyGetter(this, "gAppID", function() {
+  return gApp.ID;
+});
+
+XPCOMUtils.defineLazyGetter(this, "gAppVersion", function() {
+  return gApp.version;
+});
+
 XPCOMUtils.defineLazyGetter(this, "gABI", function() {
   let abi = null;
   try {
     abi = gApp.XPCOMABI;
   } catch (e) {
     LOG("BlockList Global gABI: XPCOM ABI unknown.");
   }
   return abi;
@@ -243,24 +251,26 @@ function restartApp() {
  * xpcomabi attribute.
  *
  * @param {Element} blocklistElement
  *        The blocklist element from an XML blocklist.
  * @returns {bool}
  *        Whether the entry matches the current OS.
  */
 function matchesOSABI(blocklistElement) {
-  if (blocklistElement.hasAttribute("os")) {
-    var choices = blocklistElement.getAttribute("os").split(",");
+  let os = blocklistElement.getAttribute("os");
+  if (os) {
+    let choices = os.split(",");
     if (choices.length > 0 && !choices.includes(gApp.OS))
       return false;
   }
 
-  if (blocklistElement.hasAttribute("xpcomabi")) {
-    choices = blocklistElement.getAttribute("xpcomabi").split(",");
+  let xpcomabi = blocklistElement.getAttribute("xpcomabi");
+  if (xpcomabi) {
+    let choices = xpcomabi.split(",");
     if (choices.length > 0 && !choices.includes(gApp.XPCOMABI))
       return false;
   }
 
   return true;
 }
 
 /**
@@ -275,32 +285,16 @@ function getLocale() {
 }
 
 /* Get the distribution pref values, from defaults only */
 function getDistributionPrefValue(aPrefName) {
   return Services.prefs.getDefaultBranch(null).getCharPref(aPrefName, "default");
 }
 
 /**
- * Parse a string representation of a regular expression. Needed because we
- * use the /pattern/flags form (because it's detectable), which is only
- * supported as a literal in JS.
- *
- * @param {string} aStr
- *         String representation of regexp
- * @return {RegExp} instance
- */
-function parseRegExp(aStr) {
-  let lastSlash = aStr.lastIndexOf("/");
-  let pattern = aStr.slice(1, lastSlash);
-  let flags = aStr.slice(lastSlash + 1);
-  return new RegExp(pattern, flags);
-}
-
-/**
  * Manages the Blocklist. The Blocklist is a representation of the contents of
  * blocklist.xml and allows us to remotely disable / re-enable blocklisted
  * items managed by the Extension Manager with an item's appDisabled property.
  * It also blocklists plugins with data from blocklist.xml.
  */
 var Blocklist = {
   _init() {
     Services.obs.addObserver(this, "xpcom-shutdown");
@@ -436,21 +430,21 @@ var Blocklist = {
    *          properties indicating the block state and URL, if there is
    *          a matching blocklist entry, or null otherwise.
    */
   _getAddonBlocklistEntry(addon, addonEntries, appVersion, toolkitVersion) {
     if (!gBlocklistEnabled)
       return null;
 
     // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
-    if (!appVersion && !gApp.version)
+    if (!appVersion && !gAppVersion)
       return null;
 
     if (!appVersion)
-      appVersion = gApp.version;
+      appVersion = gAppVersion;
     if (!toolkitVersion)
       toolkitVersion = gApp.platformVersion;
 
     var blItem = this._findMatchingAddonEntry(addonEntries, addon);
     if (!blItem)
       return null;
 
     for (let currentblItem of blItem.versions) {
@@ -531,17 +525,17 @@ var Blocklist = {
 
   _findMatchingAddonEntry(aAddonEntries, aAddon) {
     if (!aAddon)
       return null;
     // Returns true if the params object passes the constraints set by entry.
     // (For every non-null property in entry, the same key must exist in
     // params and value must be the same)
     function checkEntry(entry, params) {
-      for (let [key, value] of entry) {
+      for (let [key, value] of Object.entries(entry)) {
         if (value === null || value === undefined)
           continue;
         if (params[key]) {
           if (value instanceof RegExp) {
             if (!value.test(params[key])) {
               return false;
             }
           } else if (value !== params[key]) {
@@ -607,37 +601,43 @@ var Blocklist = {
       }
     }
 
     if (pingCountVersion < 1)
       pingCountVersion = 1;
     if (pingCountTotal < 1)
       pingCountTotal = 1;
 
-    dsURI = dsURI.replace(/%APP_ID%/g, gApp.ID);
-    // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
-    if (gApp.version)
-      dsURI = dsURI.replace(/%APP_VERSION%/g, gApp.version);
-    dsURI = dsURI.replace(/%PRODUCT%/g, gApp.name);
-    // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
-    if (gApp.version)
-      dsURI = dsURI.replace(/%VERSION%/g, gApp.version);
-    dsURI = dsURI.replace(/%BUILD_ID%/g, gApp.appBuildID);
-    dsURI = dsURI.replace(/%BUILD_TARGET%/g, gApp.OS + "_" + gABI);
-    dsURI = dsURI.replace(/%OS_VERSION%/g, gOSVersion);
-    dsURI = dsURI.replace(/%LOCALE%/g, getLocale());
-    dsURI = dsURI.replace(/%CHANNEL%/g, UpdateUtils.UpdateChannel);
-    dsURI = dsURI.replace(/%PLATFORM_VERSION%/g, gApp.platformVersion);
-    dsURI = dsURI.replace(/%DISTRIBUTION%/g,
-                      getDistributionPrefValue(PREF_APP_DISTRIBUTION));
-    dsURI = dsURI.replace(/%DISTRIBUTION_VERSION%/g,
-                      getDistributionPrefValue(PREF_APP_DISTRIBUTION_VERSION));
-    dsURI = dsURI.replace(/%PING_COUNT%/g, pingCountVersion);
-    dsURI = dsURI.replace(/%TOTAL_PING_COUNT%/g, pingCountTotal);
-    dsURI = dsURI.replace(/%DAYS_SINCE_LAST_PING%/g, daysSinceLastPing);
+    let replacements = {
+      APP_ID: gAppID,
+      PRODUCT: gApp.name,
+      BUILD_ID: gApp.appBuildID,
+      BUILD_TARGET: gApp.OS + "_" + gABI,
+      OS_VERSION: gOSVersion,
+      LOCALE: getLocale(),
+      CHANNEL: UpdateUtils.UpdateChannel,
+      PLATFORM_VERSION: gApp.platformVersion,
+      DISTRIBUTION: getDistributionPrefValue(PREF_APP_DISTRIBUTION),
+      DISTRIBUTION_VERSION: getDistributionPrefValue(PREF_APP_DISTRIBUTION_VERSION),
+      PING_COUNT: pingCountVersion,
+      TOTAL_PING_COUNT: pingCountTotal,
+      DAYS_SINCE_LAST_PING: daysSinceLastPing,
+    };
+    dsURI = dsURI.replace(/%([A-Z_]+)%/g, function(fullMatch, name) {
+      // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
+      if (gAppVersion && (name == "APP_VERSION" || name == "VERSION")) {
+        return gAppVersion;
+      }
+      // Some items, like DAYS_SINCE_LAST_PING, can be undefined, so we can't just
+      // `return replacements[name] || fullMatch` or something like that.
+      if (!replacements.hasOwnProperty(name)) {
+        return fullMatch;
+      }
+      return replacements[name];
+    });
     dsURI = dsURI.replace(/\+/g, "%2B");
 
     // Under normal operations it will take around 5,883,516 years before the
     // preferences used to store pingCountVersion and pingCountTotal will rollover
     // so this code doesn't bother trying to do the "right thing" here.
     if (pingCountVersion != "invalid") {
       pingCountVersion++;
       if (pingCountVersion > 2147483647) {
@@ -917,43 +917,48 @@ var Blocklist = {
   _handleEmItemNode(blocklistElement, result) {
     if (!matchesOSABI(blocklistElement))
       return;
 
     let blockEntry = {
       versions: [],
       prefs: [],
       blockID: null,
-      attributes: new Map()
+      attributes: {},
       // Atleast one of EXTENSION_BLOCK_FILTERS must get added to attributes
     };
 
-    // Any filter starting with '/' is interpreted as a regex. So if an attribute
-    // starts with a '/' it must be checked via a regex.
-    function regExpCheck(attr) {
-      return attr.startsWith("/") ? parseRegExp(attr) : attr;
-    }
-
     for (let filter of EXTENSION_BLOCK_FILTERS) {
       let attr = blocklistElement.getAttribute(filter);
-      if (attr)
-        blockEntry.attributes.set(filter, regExpCheck(attr));
+      if (attr) {
+        // Any filter starting with '/' is interpreted as a regex. So if an attribute
+        // starts with a '/' it must be checked via a regex.
+        if (attr.startsWith("/")) {
+          let lastSlash = attr.lastIndexOf("/");
+          let pattern = attr.slice(1, lastSlash);
+          let flags = attr.slice(lastSlash + 1);
+          blockEntry.attributes[filter] = new RegExp(pattern, flags);
+        } else {
+          blockEntry.attributes[filter] = attr;
+        }
+      }
     }
 
     var children = blocklistElement.children;
 
     for (let childElement of children) {
-      if (childElement.localName === "prefs") {
+      let localName = childElement.localName;
+      if (localName == "prefs" && childElement.hasChildNodes) {
         let prefElements = childElement.children;
         for (let prefElement of prefElements) {
           if (prefElement.localName == "pref") {
             blockEntry.prefs.push(prefElement.textContent);
           }
         }
-      } else if (childElement.localName === "versionRange") {
+      } else if (localName == "versionRange") {
         blockEntry.versions.push(new BlocklistItemData(childElement));
       }
     }
     // if only the extension ID is specified block all versions of the
     // extension for the current application.
     if (blockEntry.versions.length == 0)
       blockEntry.versions.push(new BlocklistItemData(null));
 
@@ -970,29 +975,33 @@ var Blocklist = {
     var blockEntry = {
       matches: {},
       versions: [],
       blockID: null,
       infoURL: null,
     };
     var hasMatch = false;
     for (let childElement of children) {
-      if (childElement.localName == "match") {
-        var name = childElement.getAttribute("name");
-        var exp = childElement.getAttribute("exp");
-        try {
-          blockEntry.matches[name] = new RegExp(exp, "m");
-          hasMatch = true;
-        } catch (e) {
-          // Ignore invalid regular expressions
-        }
-      } else if (childElement.localName == "versionRange") {
-        blockEntry.versions.push(new BlocklistItemData(childElement));
-      } else if (childElement.localName == "infoURL") {
-        blockEntry.infoURL = childElement.textContent;
+      switch (childElement.localName) {
+        case "match":
+          var name = childElement.getAttribute("name");
+          var exp = childElement.getAttribute("exp");
+          try {
+            blockEntry.matches[name] = new RegExp(exp, "m");
+            hasMatch = true;
+          } catch (e) {
+            // Ignore invalid regular expressions
+          }
+          break;
+        case "versionRange":
+          blockEntry.versions.push(new BlocklistItemData(childElement));
+          break;
+        case "infoURL":
+          blockEntry.infoURL = childElement.textContent;
+          break;
       }
     }
     // Plugin entries require *something* to match to an actual plugin
     if (!hasMatch)
       return;
     // Add a default versionRange if there wasn't one specified
     if (blockEntry.versions.length == 0)
       blockEntry.versions.push(new BlocklistItemData(null));
@@ -1090,21 +1099,21 @@ var Blocklist = {
    *        {entry: blocklistEntry, version: blocklistEntryVersion},
    *        or null if there is no matching entry.
    */
   _getPluginBlocklistEntry(plugin, pluginEntries, appVersion, toolkitVersion) {
     if (!gBlocklistEnabled)
       return null;
 
     // Not all applications implement nsIXULAppInfo (e.g. xpcshell doesn't).
-    if (!appVersion && !gApp.version)
+    if (!appVersion && !gAppVersion)
       return Ci.nsIBlocklistService.STATE_NOT_BLOCKED;
 
     if (!appVersion)
-      appVersion = gApp.version;
+      appVersion = gAppVersion;
     if (!toolkitVersion)
       toolkitVersion = gApp.platformVersion;
 
     const pluginProperties = {
       description: plugin.description,
       filename: plugin.filename,
       name: plugin.name,
       version: plugin.version,
@@ -1404,45 +1413,45 @@ var Blocklist = {
       blocklistWindow.addEventListener("unload", blocklistUnloadHandler);
   },
 };
 
 /*
  * Helper for constructing a blocklist.
  */
 function BlocklistItemData(versionRangeElement) {
-  var versionRange = this.getBlocklistVersionRange(versionRangeElement);
-  this.minVersion = versionRange.minVersion;
-  this.maxVersion = versionRange.maxVersion;
-  if (versionRangeElement && versionRangeElement.hasAttribute("severity"))
-    this.severity = versionRangeElement.getAttribute("severity");
-  else
-    this.severity = DEFAULT_SEVERITY;
-  if (versionRangeElement && versionRangeElement.hasAttribute("vulnerabilitystatus")) {
-    this.vulnerabilityStatus = versionRangeElement.getAttribute("vulnerabilitystatus");
+  this.targetApps = {};
+  let foundTarget = false;
+  this.severity = DEFAULT_SEVERITY;
+  this.vulnerabilityStatus = VULNERABILITYSTATUS_NONE;
+  if (versionRangeElement) {
+    let versionRange = this.getBlocklistVersionRange(versionRangeElement);
+    this.minVersion = versionRange.minVersion;
+    this.maxVersion = versionRange.maxVersion;
+    if (versionRangeElement.hasAttribute("severity"))
+      this.severity = versionRangeElement.getAttribute("severity");
+    if (versionRangeElement.hasAttribute("vulnerabilitystatus")) {
+      this.vulnerabilityStatus = versionRangeElement.getAttribute("vulnerabilitystatus");
+    }
+    for (let targetAppElement of versionRangeElement.children) {
+      if (targetAppElement.localName == "targetApplication") {
+        foundTarget = true;
+        // default to the current application if id is not provided.
+        let appID = targetAppElement.id || gAppID;
+        this.targetApps[appID] = this.getBlocklistAppVersions(targetAppElement);
+      }
+    }
   } else {
-    this.vulnerabilityStatus = VULNERABILITYSTATUS_NONE;
+    this.minVersion = this.maxVersion = null;
   }
-  this.targetApps = { };
-  var found = false;
 
-  if (versionRangeElement) {
-    for (let targetAppElement of versionRangeElement.children) {
-      if (targetAppElement.localName != "targetApplication")
-        continue;
-      found = true;
-      // default to the current application if id is not provided.
-      var appID = targetAppElement.hasAttribute("id") ? targetAppElement.getAttribute("id") : gApp.ID;
-      this.targetApps[appID] = this.getBlocklistAppVersions(targetAppElement);
-    }
-  }
   // Default to all versions of the current application when no targetApplication
   // elements were found
-  if (!found)
-    this.targetApps[gApp.ID] = this.getBlocklistAppVersions(null);
+  if (!foundTarget)
+    this.targetApps[gAppID] = [{minVersion: null, maxVersion: null}];
 }
 
 BlocklistItemData.prototype = {
   /**
    * Tests if a version of an item is included in the version range and target
    * application information represented by this BlocklistItemData using the
    * provided application and toolkit versions.
    * @param {string} version
@@ -1461,17 +1470,17 @@ BlocklistItemData.prototype = {
     if (!version && (this.minVersion || this.maxVersion))
       return false;
 
     // Check if the item version matches
     if (!this.matchesRange(version, this.minVersion, this.maxVersion))
       return false;
 
     // Check if the application version matches
-    if (this.matchesTargetRange(gApp.ID, appVersion))
+    if (this.matchesTargetRange(gAppID, appVersion))
       return true;
 
     // Check if the toolkit version matches
     return this.matchesTargetRange(TOOLKIT_ID, toolkitVersion);
   },
 
   /**
    * Checks if a version is higher than or equal to the minVersion (if provided)
@@ -1536,40 +1545,34 @@ BlocklistItemData.prototype = {
         if (versionRangeElement.localName == "versionRange") {
           appVersions.push(this.getBlocklistVersionRange(versionRangeElement));
         }
       }
     }
     // return minVersion = null and maxVersion = null if no specific versionRange
     // elements were found
     if (appVersions.length == 0)
-      appVersions.push(this.getBlocklistVersionRange(null));
+      appVersions.push({minVersion: null, maxVersion: null});
     return appVersions;
   },
 
   /**
    * Retrieves a version range (e.g. minVersion and maxVersion) for a blocklist
    * versionRange element.
    *
    * @param {Element} versionRangeElement
    *        The versionRange blocklist element.
    *
    * @returns {Object}
    *        A JS object with the following properties:
    *          "minVersion"  The minimum version in a version range (default = null).
    *          "maxVersion"  The maximum version in a version range (default = null).
    */
   getBlocklistVersionRange(versionRangeElement) {
-    var minVersion = null;
-    var maxVersion = null;
-    if (!versionRangeElement)
-      return { minVersion, maxVersion };
-
-    if (versionRangeElement.hasAttribute("minVersion"))
-      minVersion = versionRangeElement.getAttribute("minVersion");
-    if (versionRangeElement.hasAttribute("maxVersion"))
-      maxVersion = versionRangeElement.getAttribute("maxVersion");
+    // getAttribute returns null if the attribute is not present.
+    let minVersion = versionRangeElement.getAttribute("minVersion");
+    let maxVersion = versionRangeElement.getAttribute("maxVersion");
 
     return { minVersion, maxVersion };
   }
 };
 
 Blocklist._init();
--- a/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
+++ b/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm
@@ -738,18 +738,23 @@ var AddonTestUtils = {
    *        If provided, the application version is changed to this string
    *        before the AddonManager is started.
    */
   async promiseStartupManager(newVersion) {
     if (this.addonIntegrationService)
       throw new Error("Attempting to startup manager that was already started.");
 
 
-    if (newVersion)
+    if (newVersion) {
       this.appInfo.version = newVersion;
+      if (Cu.isModuleLoaded("resource://gre/modules/Blocklist.jsm")) {
+        let bsPassBlocklist = ChromeUtils.import("resource://gre/modules/Blocklist.jsm", {});
+        Object.defineProperty(bsPassBlocklist, "gAppVersion", {value: newVersion});
+      }
+    }
 
     let XPIScope = ChromeUtils.import("resource://gre/modules/addons/XPIProvider.jsm", null);
     XPIScope.AsyncShutdown = MockAsyncShutdown;
 
     XPIScope.XPIInternal.BootstrapScope.prototype
       ._beforeCallBootstrapMethod = (method, params, reason) => {
         try {
           this.emit("bootstrap-method", {method, params, reason});
--- a/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_blocklistchange.js
@@ -786,16 +786,18 @@ add_task(async function run_app_update_s
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
 });
 
 add_task(async function update_schema_2() {
   await promiseShutdownManager();
 
   await changeXPIDBVersion(100);
   gAppInfo.version = "2";
+  let bsPassBlocklist = ChromeUtils.import("resource://gre/modules/Blocklist.jsm", {});
+  Object.defineProperty(bsPassBlocklist, "gAppVersion", {value: "2"});
   await promiseStartupManager();
 
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s2, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s3, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
@@ -810,16 +812,18 @@ add_task(async function update_schema_2(
 });
 
 add_task(async function update_schema_3() {
   await promiseRestartManager();
 
   await promiseShutdownManager();
   await changeXPIDBVersion(100);
   gAppInfo.version = "2.5";
+  let bsPassBlocklist = ChromeUtils.import("resource://gre/modules/Blocklist.jsm", {});
+  Object.defineProperty(bsPassBlocklist, "gAppVersion", {value: "2.5"});
   await promiseStartupManager();
 
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", true, true, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s2, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_SOFTBLOCKED);
@@ -843,16 +847,18 @@ add_task(async function update_schema_4(
   check_addon(r, "1.0", false, false, Ci.nsIBlocklistService.STATE_BLOCKED);
 });
 
 add_task(async function update_schema_5() {
   await promiseShutdownManager();
 
   await changeXPIDBVersion(100);
   gAppInfo.version = "1";
+  let bsPassBlocklist = ChromeUtils.import("resource://gre/modules/Blocklist.jsm", {});
+  Object.defineProperty(bsPassBlocklist, "gAppVersion", {value: "1"});
   await promiseStartupManager();
 
   let [s1, s2, s3, s4, h, r] = await promiseAddonsByIDs(ADDON_IDS);
 
   check_addon(s1, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s2, "1.0", true, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s3, "1.0", false, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);
   check_addon(s4, "1.0", true, false, Ci.nsIBlocklistService.STATE_NOT_BLOCKED);