Bug 1541167 Stop doing synchronous main-thread I/O to validate the blocklist r=Gijs
authorAndrew Swan <aswan@mozilla.com>
Tue, 23 Apr 2019 16:11:13 +0000
changeset 470511 6569e4cca483da6fb943a60e3e60ed4c052f7fc6
parent 470510 17b4c383ffbd54407ed6b16de81881c2bace8ca2
child 470512 c31ccea52937e0c889f392b8d52094ad367c5f60
push id35906
push useraciure@mozilla.com
push dateTue, 23 Apr 2019 22:14:56 +0000
treeherdermozilla-central@0ce3633f8b80 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1541167
milestone68.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 1541167 Stop doing synchronous main-thread I/O to validate the blocklist r=Gijs The main change here is moving the old AddonManager.validateBlocklist() into Blocklist.jsm and getting rid of any main-thread I/O. This patch also includes a small functional change: previously we would always copy the application-supplied blocklist.xml into the profile directory. With this change, blocklist.xml is not copied from the application into the profile. This entails an extra open() when we're falling back to the application-supplied blocklist file but saves all the I/O required to actually copy the file. Differential Revision: https://phabricator.services.mozilla.com/D27829
toolkit/mozapps/extensions/AddonManager.jsm
toolkit/mozapps/extensions/Blocklist.jsm
toolkit/mozapps/extensions/test/xpcshell/test_overrideblocklist.js
--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -31,22 +31,16 @@ const PREF_EM_CHECK_UPDATE_SECURITY   = 
 const PREF_SYS_ADDON_UPDATE_ENABLED   = "extensions.systemAddon.update.enabled";
 
 const PREF_MIN_WEBEXT_PLATFORM_VERSION = "extensions.webExtensionsMinPlatformVersion";
 const PREF_WEBAPI_TESTING             = "extensions.webapi.testing";
 const PREF_WEBEXT_PERM_PROMPTS        = "extensions.webextPermissionPrompts";
 
 const UPDATE_REQUEST_VERSION          = 2;
 
-const XMLURI_BLOCKLIST                = "http://www.mozilla.org/2006/addons-blocklist";
-
-const KEY_PROFILEDIR                  = "ProfD";
-const KEY_APPDIR                      = "XCurProcD";
-const FILE_BLOCKLIST                  = "blocklist.xml";
-
 const BRANCH_REGEXP                   = /^([^\.]+\.[0-9]+[a-z]*).*/gi;
 const PREF_EM_CHECK_COMPATIBILITY_BASE = "extensions.checkCompatibility";
 var PREF_EM_CHECK_COMPATIBILITY = MOZ_COMPATIBILITY_NIGHTLY ?
                                   PREF_EM_CHECK_COMPATIBILITY_BASE + ".nightly" :
                                   undefined;
 
 const VALID_TYPES_REGEXP = /^[\w\-]+$/;
 
@@ -57,22 +51,21 @@ const WEBAPI_TEST_INSTALL_HOSTS = [
 ];
 
 const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 // This global is overridden by xpcshell tests, and therefore cannot be
 // a const.
 var {AsyncShutdown} = ChromeUtils.import("resource://gre/modules/AsyncShutdown.jsm");
 
-XPCOMUtils.defineLazyGlobalGetters(this, ["DOMParser", "Element"]);
+XPCOMUtils.defineLazyGlobalGetters(this, ["Element"]);
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AddonRepository: "resource://gre/modules/addons/AddonRepository.jsm",
   Extension: "resource://gre/modules/Extension.jsm",
-  FileUtils: "resource://gre/modules/FileUtils.jsm",
 });
 
 XPCOMUtils.defineLazyPreferenceGetter(this, "WEBEXT_PERMISSION_PROMPTS",
                                       PREF_WEBEXT_PERM_PROMPTS, false);
 
 // Initialize the WebExtension process script service as early as possible,
 // since it needs to be able to track things like new frameLoader globals that
 // are created before other framework code has been initialized.
@@ -551,92 +544,16 @@ var AddonManagerInternal = {
   telemetryDetails: {},
   upgradeListeners: new Map(),
   externalExtensionLoaders: new Map(),
 
   recordTimestamp(name, value) {
     this.TelemetryTimestamps.add(name, value);
   },
 
-  validateBlocklist() {
-    let appBlocklist = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]);
-
-    // If there is no application shipped blocklist then there is nothing to do
-    if (!appBlocklist.exists())
-      return;
-
-    let profileBlocklist = FileUtils.getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]);
-
-    // If there is no blocklist in the profile then copy the application shipped
-    // one there
-    if (!profileBlocklist.exists()) {
-      try {
-        appBlocklist.copyTo(profileBlocklist.parent, FILE_BLOCKLIST);
-      } catch (e) {
-        logger.warn("Failed to copy the application shipped blocklist to the profile", e);
-      }
-      return;
-    }
-
-    let fileStream = Cc["@mozilla.org/network/file-input-stream;1"].
-                     createInstance(Ci.nsIFileInputStream);
-    try {
-      let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].
-                    createInstance(Ci.nsIConverterInputStream);
-      fileStream.init(appBlocklist, FileUtils.MODE_RDONLY, FileUtils.PERMS_FILE, 0);
-      cstream.init(fileStream, "UTF-8", 0, 0);
-
-      let data = "";
-      let str = {};
-      let read = 0;
-      do {
-        read = cstream.readString(0xffffffff, str);
-        data += str.value;
-      } while (read != 0);
-
-      let parser = new DOMParser();
-      var doc = parser.parseFromString(data, "text/xml");
-    } catch (e) {
-      logger.warn("Application shipped blocklist could not be loaded", e);
-      return;
-    } finally {
-      try {
-        fileStream.close();
-      } catch (e) {
-        logger.warn("Unable to close blocklist file stream", e);
-      }
-    }
-
-    // If the namespace is incorrect then ignore the application shipped
-    // blocklist
-    if (doc.documentElement.namespaceURI != XMLURI_BLOCKLIST) {
-      logger.warn("Application shipped blocklist has an unexpected namespace (" +
-                  doc.documentElement.namespaceURI + ")");
-      return;
-    }
-
-    // If there is no lastupdate information then ignore the application shipped
-    // blocklist
-    if (!doc.documentElement.hasAttribute("lastupdate"))
-      return;
-
-    // If the application shipped blocklist is older than the profile blocklist
-    // then do nothing
-    if (doc.documentElement.getAttribute("lastupdate") <=
-        profileBlocklist.lastModifiedTime)
-      return;
-
-    // Otherwise copy the application shipped blocklist to the profile
-    try {
-      appBlocklist.copyTo(profileBlocklist.parent, FILE_BLOCKLIST);
-    } catch (e) {
-      logger.warn("Failed to copy the application shipped blocklist to the profile", e);
-    }
-  },
-
   /**
    * Start up a provider, and register its shutdown hook if it has one
    *
    * @param {string} aProvider - An add-on provider.
    * @param {boolean} aAppChanged - Whether or not the app version has changed since last session.
    * @param {string} aOldAppVersion - Previous application version, if changed.
    * @param {string} aOldPlatformVersion - Previous platform version, if changed.
    *
@@ -721,17 +638,16 @@ var AddonManagerInternal = {
       if (appChanged !== false) {
         logger.debug("Application has been upgraded");
         Services.prefs.setCharPref(PREF_EM_LAST_APP_VERSION,
                                    Services.appinfo.version);
         Services.prefs.setCharPref(PREF_EM_LAST_PLATFORM_VERSION,
                                    Services.appinfo.platformVersion);
         Services.prefs.setIntPref(PREF_BLOCKLIST_PINGCOUNTVERSION,
                                   (appChanged === undefined ? 0 : -1));
-        this.validateBlocklist();
       }
 
       if (!MOZ_COMPATIBILITY_NIGHTLY) {
         PREF_EM_CHECK_COMPATIBILITY = PREF_EM_CHECK_COMPATIBILITY_BASE + "." +
                                       Services.appinfo.version.replace(BRANCH_REGEXP, "$1");
       }
 
       gCheckCompatibility = Services.prefs.getBoolPref(PREF_EM_CHECK_COMPATIBILITY,
--- a/toolkit/mozapps/extensions/Blocklist.jsm
+++ b/toolkit/mozapps/extensions/Blocklist.jsm
@@ -124,16 +124,18 @@ const VULNERABILITYSTATUS_UPDATE_AVAILAB
 const VULNERABILITYSTATUS_NO_UPDATE        = 2;
 
 const EXTENSION_BLOCK_FILTERS = ["id", "name", "creator", "homepageURL", "updateURL"];
 
 var gLoggingEnabled = null;
 var gBlocklistEnabled = true;
 var gBlocklistLevel = DEFAULT_LEVEL;
 
+class BlocklistError extends Error {}
+
 /**
  * @class nsIBlocklistPrompt
  *
  * nsIBlocklistPrompt is used, if available, by the default implementation of
  * nsIBlocklistService to display a confirmation UI to the user before blocking
  * extensions/plugins.
  */
 /**
@@ -336,16 +338,22 @@ var Blocklist = {
    *                                 (default = 0)
    *                   "maxVersion"  The maximum version in a version range
    *                                 (default = *)
    */
   _addonEntries: null,
   _gfxEntries: null,
   _pluginEntries: null,
 
+  get profileBlocklistPath() {
+    let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
+    Object.defineProperty(this, "profileBlocklistPath", {value: path});
+    return path;
+  },
+
   shutdown() {
     Services.obs.removeObserver(this, "xpcom-shutdown");
     Services.prefs.removeObserver("extensions.blocklist.", this);
     Services.prefs.removeObserver(PREF_EM_LOGGING_ENABLED, this);
   },
 
   observe(aSubject, aTopic, aData) {
     switch (aTopic) {
@@ -724,17 +732,17 @@ var Blocklist = {
     var oldPluginEntries = this._pluginEntries;
 
     await this._loadBlocklistFromXML(responseXML);
     // We don't inform the users when the graphics blocklist changed at runtime.
     // However addons and plugins blocking status is refreshed.
     this._blocklistUpdated(oldAddonEntries, oldPluginEntries);
 
     try {
-      let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
+      let path = this.profileBlocklistPath;
       await OS.File.writeAtomic(path, request.responseText, {tmpPath: path + ".tmp"});
     } catch (e) {
       LOG("Blocklist::onXMLLoad: " + e);
     }
   },
 
   onXMLError(aEvent) {
     try {
@@ -781,84 +789,123 @@ var Blocklist = {
     }
     if (!this._loadPromise) {
       this._loadPromise = this._loadBlocklistAsyncInternal();
     }
     await this._loadPromise;
   },
 
   async _loadBlocklistAsyncInternal() {
+    if (this.isLoaded) {
+      return;
+    }
+
+    if (!gBlocklistEnabled) {
+      LOG("Blocklist::loadBlocklist: blocklist is disabled");
+      return;
+    }
+
+    let xmlDoc;
     try {
       // Get the path inside the try...catch because there's no profileDir in e.g. xpcshell tests.
       let profFile = FileUtils.getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]);
-      await this._loadFileInternal(profFile);
-      return;
+      xmlDoc = await this._loadFile(profFile);
     } catch (e) {
       LOG("Blocklist::loadBlocklistAsync: Failed to load XML file " + e);
     }
 
-    var appFile = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]);
-    try {
-      await this._loadFileInternal(appFile);
+    // If there's no blocklist.xml in the profile, read it from the
+    // application.  Even if blocklist.xml exists in the profile, if this
+    // is the first run of a new application version, read the application
+    // version anyway and check if it is newer.
+    if (!xmlDoc || AddonManagerPrivate.browserUpdated) {
+      var appFile = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]);
+      let appDoc;
+      try {
+        appDoc = await this._loadFile(appFile);
+      } catch (e) {
+        LOG("Blocklist::loadBlocklistAsync: Failed to load XML file " + e);
+      }
+
+      if (xmlDoc && appDoc) {
+        // If we have a new blocklist.xml in the application directory,
+        // delete the older one from the profile so we'll fall back to
+        // the app version on subsequent startups.
+        let clearProfile = false;
+        if (!xmlDoc.documentElement.hasAttribute("lastupdate")) {
+          clearProfile = true;
+        } else {
+          let profileTS = parseInt(xmlDoc.documentElement.getAttribute("lastupdate"), 10);
+          let appTS = parseInt(appDoc.documentElement.getAttribute("lastupdate"), 10);
+          if (appTS > profileTS) {
+            clearProfile = true;
+          }
+        }
+
+        if (clearProfile) {
+          await OS.File.remove(this.profileBlocklistPath);
+          xmlDoc = null;
+        }
+      }
+
+      if (!xmlDoc) {
+        // If we get here, either there was not a blocklist.xml in the
+        // profile, or it was deleted for being too old.  Fall back to
+        // the version from the application instead.
+        xmlDoc = appDoc;
+      }
+    }
+
+    if (xmlDoc) {
+      await new Promise(resolve => {
+        ChromeUtils.idleDispatch(async () => {
+          if (!this.isLoaded) {
+            await this._loadBlocklistFromXML(xmlDoc);
+          }
+          resolve();
+        });
+      });
       return;
-    } catch (e) {
-      LOG("Blocklist::loadBlocklistAsync: Failed to load XML file " + e);
     }
 
     LOG("Blocklist::loadBlocklistAsync: no XML File found");
     // Neither file is present, so we just add empty lists, to avoid JS errors fetching
     // blocklist information otherwise.
     this._addonEntries = [];
     this._gfxEntries = [];
     this._pluginEntries = [];
   },
 
-  async _loadFileInternal(file) {
-    if (this.isLoaded) {
-      return;
-    }
-
-    if (!gBlocklistEnabled) {
-      LOG("Blocklist::_loadFileInternal: blocklist is disabled");
-      return;
-    }
-
-    let xmlDoc = await new Promise((resolve, reject) => {
+  // Loads the given file as xml, resolving with an XMLDocument.
+  // Rejects if the file cannot be loaded or if it cannot be parsed as xml.
+  _loadFile(file) {
+    return new Promise((resolve, reject) => {
       let request = new XMLHttpRequest();
       request.open("GET", Services.io.newFileURI(file).spec, true);
       request.overrideMimeType("text/xml");
       request.addEventListener("error", reject);
       request.addEventListener("load", function() {
         let {status} = request;
         if (status != 200 && status != 0) {
-          LOG("_loadFileInternal: there was an error during load, got status: " + status);
-          reject(new Error("Couldn't load blocklist file"));
+          LOG("_loadFile: there was an error during load, got status: " + status);
+          reject(new BlocklistError("Couldn't load blocklist file"));
           return;
         }
         let doc = request.responseXML;
         if (doc.documentElement.namespaceURI != XMLURI_BLOCKLIST) {
-          LOG("Blocklist::_loadBlocklistFromString: aborting due to incorrect " +
-              "XML Namespace.\nExpected: " + XMLURI_BLOCKLIST + "\n" +
-              "Received: " + doc.documentElement.namespaceURI);
-          reject(new Error("Local blocklist file has the wrong namespace!"));
+          LOG("_loadFile: aborting due to incorrect XML Namespace.\n" +
+              `Expected: ${XMLURI_BLOCKLIST}\n` +
+              `Received: ${doc.documentElement.namespaceURI}`);
+          reject(new BlocklistError("Local blocklist file has the wrong namespace!"));
           return;
         }
         resolve(doc);
       });
       request.send(null);
     });
-
-    await new Promise(resolve => {
-      ChromeUtils.idleDispatch(async () => {
-        if (!this.isLoaded) {
-          await this._loadBlocklistFromXML(xmlDoc);
-        }
-        resolve();
-      });
-    });
   },
 
   async _loadBlocklistFromXML(doc) {
     this._addonEntries = [];
     this._gfxEntries = [];
     this._pluginEntries = [];
     try {
       var children = doc.documentElement.children;
--- a/toolkit/mozapps/extensions/test/xpcshell/test_overrideblocklist.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_overrideblocklist.js
@@ -6,18 +6,16 @@ const KEY_PROFILEDIR                  = 
 const KEY_APPDIR                      = "XCurProcD";
 const FILE_BLOCKLIST                  = "blocklist.xml";
 
 const PREF_BLOCKLIST_ENABLED          = "extensions.blocklist.enabled";
 
 const OLD = do_get_file("data/test_overrideblocklist/old.xml");
 const NEW = do_get_file("data/test_overrideblocklist/new.xml");
 const ANCIENT = do_get_file("data/test_overrideblocklist/ancient.xml");
-const OLD_TSTAMP = 1296046918000;
-const NEW_TSTAMP = 1396046918000;
 
 const gAppDir = FileUtils.getFile(KEY_APPDIR, []);
 
 var oldAddon = {
   id: "old@tests.mozilla.org",
   version: 1,
 };
 var newAddon = {
@@ -52,22 +50,19 @@ async function reloadBlocklist() {
   Services.prefs.setBoolPref(PREF_BLOCKLIST_ENABLED, true);
   await Blocklist._lastUpdate;
 }
 
 function copyToApp(file) {
   file.clone().copyTo(gAppDir, FILE_BLOCKLIST);
 }
 
-function copyToProfile(file, tstamp) {
+function copyToProfile(file) {
   file = file.clone();
   file.copyTo(gProfD, FILE_BLOCKLIST);
-  file = gProfD.clone();
-  file.append(FILE_BLOCKLIST);
-  file.lastModifiedTime = tstamp;
 }
 
 function run_test() {
   createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1");
 
   let appBlocklist = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]);
   if (appBlocklist.exists()) {
     try {
@@ -108,17 +103,17 @@ add_test(async function test_copy() {
 
   run_next_test();
 });
 
 // An ancient blocklist should be ignored
 add_test(async function test_ancient() {
   clearBlocklists();
   copyToApp(ANCIENT);
-  copyToProfile(OLD, OLD_TSTAMP);
+  copyToProfile(OLD);
 
   incrementAppVersion();
   await promiseStartupManager();
 
   await reloadBlocklist();
 
   Assert.ok(!(await isBlocklisted(invalidAddon)));
   Assert.ok(!(await isBlocklisted(ancientAddon)));
@@ -129,17 +124,17 @@ add_test(async function test_ancient() {
 
   run_next_test();
 });
 
 // A new blocklist should override an old blocklist
 add_test(async function test_override() {
   clearBlocklists();
   copyToApp(NEW);
-  copyToProfile(OLD, OLD_TSTAMP);
+  copyToProfile(OLD);
 
   incrementAppVersion();
   await promiseStartupManager();
 
   await reloadBlocklist();
 
   Assert.ok(!(await isBlocklisted(invalidAddon)));
   Assert.ok(!(await isBlocklisted(ancientAddon)));
@@ -150,17 +145,17 @@ add_test(async function test_override() 
 
   run_next_test();
 });
 
 // An old blocklist shouldn't override a new blocklist
 add_test(async function test_retain() {
   clearBlocklists();
   copyToApp(OLD);
-  copyToProfile(NEW, NEW_TSTAMP);
+  copyToProfile(NEW);
 
   incrementAppVersion();
   await promiseStartupManager();
 
   await reloadBlocklist();
 
   Assert.ok(!(await isBlocklisted(invalidAddon)));
   Assert.ok(!(await isBlocklisted(ancientAddon)));
@@ -171,17 +166,17 @@ add_test(async function test_retain() {
 
   run_next_test();
 });
 
 // A missing blocklist in the profile should still load an app-shipped blocklist
 add_test(async function test_missing() {
   clearBlocklists();
   copyToApp(OLD);
-  copyToProfile(NEW, NEW_TSTAMP);
+  copyToProfile(NEW);
 
   incrementAppVersion();
   await promiseStartupManager();
   await promiseShutdownManager();
 
   let blocklist = FileUtils.getFile(KEY_PROFILEDIR, [FILE_BLOCKLIST]);
   blocklist.remove(true);
   await promiseStartupManager();