Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file, r?florian draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 12 Mar 2018 23:49:28 +0000
changeset 766533 1ee62000ee536221d5d9573bcbf97249da4f4597
parent 766373 3700864a1940c0dfc94a8002f0be6d99008740b5
push id102346
push usergijskruitbosch@gmail.com
push dateMon, 12 Mar 2018 23:59:15 +0000
reviewersflorian
bugs1371888
milestone61.0a1
Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file, r?florian When I refactored this test, it still failed. The cause was that the previous commits change the organization of the code such that we no longer call _loadBlocklist to actually load the content, if loading it async, and it omitted resetting the 'Entries' properties. This wouldn't be a problem in practice because our blocklist has entries of all these kinds, but the test blocklist file is empty, and so triggered issues because these properties remained null. MozReview-Commit-ID: 8zNr8R97fyR
toolkit/mozapps/extensions/nsBlocklistService.js
toolkit/mozapps/extensions/test/xpcshell/test_asyncBlocklistLoad.js
--- a/toolkit/mozapps/extensions/nsBlocklistService.js
+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
@@ -828,21 +828,27 @@ Blocklist.prototype = {
 
     if (!gBlocklistEnabled) {
       LOG("Blocklist::_preloadBlocklistFile: blocklist is disabled");
       return;
     }
 
     let text = await OS.File.read(path, { encoding: "utf-8" });
 
-    Services.tm.idleDispatchToMainThread(() => {
-      if (!this.isLoaded) {
-        Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false);
-        this._loadBlocklistFromString(text);
-      }
+    await new Promise(resolve => {
+      Services.tm.idleDispatchToMainThread(() => {
+        if (!this.isLoaded) {
+          Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false);
+          this._gfxEntries = [];
+          this._addonEntries = [];
+          this._pluginEntries = [];
+          this._loadBlocklistFromString(text);
+        }
+        resolve();
+      });
     });
   },
 
   _loadBlocklistFromString(text) {
     try {
       var parser = Cc["@mozilla.org/xmlextras/domparser;1"].
                    createInstance(Ci.nsIDOMParser);
       var doc = parser.parseFromString(text, "text/xml");
--- a/toolkit/mozapps/extensions/test/xpcshell/test_asyncBlocklistLoad.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_asyncBlocklistLoad.js
@@ -2,39 +2,46 @@
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
 add_task(async function() {
   let blocklist = AM_Cc["@mozilla.org/extensions/blocklist;1"].
                   getService().wrappedJSObject;
   let scope = ChromeUtils.import("resource://gre/modules/osfile.jsm", {});
 
-  // sync -> async
+  // sync -> async. Check that async code doesn't try to read the file
+  // once it's already been read synchronously.
+  let read = scope.OS.File.read;
+  let triedToRead = false;
+  scope.OS.File.read = () => triedToRead = true;
   blocklist._loadBlocklist();
   Assert.ok(blocklist.isLoaded);
   await blocklist._preloadBlocklist();
-  Assert.ok(!blocklist._isBlocklistPreloaded());
+  Assert.ok(!triedToRead);
+  scope.OS.File.read = read;
   blocklist._clear();
 
-  // async -> sync
+  info("sync -> async complete");
+
+  // async first. Check that once we preload the content, that is sufficient.
   await blocklist._preloadBlocklist();
-  Assert.ok(!blocklist.isLoaded);
-  Assert.ok(blocklist._isBlocklistPreloaded());
-  blocklist._loadBlocklist();
   Assert.ok(blocklist.isLoaded);
-  Assert.ok(!blocklist._isBlocklistPreloaded());
+  // Calling _loadBlocklist now would just re-load the list sync.
+
+  info("async test complete");
   blocklist._clear();
 
   // async -> sync -> async
-  let read = scope.OS.File.read;
   scope.OS.File.read = function(...args) {
     return new Promise((resolve, reject) => {
       executeSoon(() => {
         blocklist._loadBlocklist();
+        // Now do the async bit after all:
         resolve(read(...args));
       });
     });
   };
 
   await blocklist._preloadBlocklist();
+  // We're mostly just checking this doesn't error out.
   Assert.ok(blocklist.isLoaded);
-  Assert.ok(!blocklist._isBlocklistPreloaded());
+  info("mixed async/sync test complete");
 });