Bug 1482306 - Ensure that tables are enabled when shared between features. r=dimi!
authorFrancois Marier <francois@mozilla.com>
Wed, 15 Aug 2018 12:13:16 +0000
changeset 486735 6560b92dbe64151387d6dd2e73167912a17d3af2
parent 486734 68faa8b52f60394faf626d19dc3c439c48e23cfb
child 486736 aaabe6d53de26960892d20922454b64708ad2319
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdimi
bugs1482306
milestone63.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 1482306 - Ensure that tables are enabled when shared between features. r=dimi! The enable/disable logic of the list manager was wrong. If multiple features shared a given table (e.g. tracking protection and tracking annotations) and only one of them was enabled, then updates could either be disabled or enabled depending on which feature was checked first. By disabling everything at the beginning and selectively re-enabling tables, we can ensure that no table gets both enabled and disabled by different feature toggles. Differential Revision: https://phabricator.services.mozilla.com/D3259
toolkit/components/url-classifier/SafeBrowsing.jsm
toolkit/components/url-classifier/nsIUrlListManager.idl
toolkit/components/url-classifier/nsUrlClassifierListManager.js
--- a/toolkit/components/url-classifier/SafeBrowsing.jsm
+++ b/toolkit/components/url-classifier/SafeBrowsing.jsm
@@ -405,98 +405,76 @@ var SafeBrowsing = {
         "blockedEnabled:", this.blockedEnabled,
         "trackingAnnotations", this.trackingAnnotations,
         "flashBlockEnabled", this.flashBlockEnabled,
         "flashInfobarListEnabled:", this.flashInfobarListEnabled);
 
     let listManager = Cc["@mozilla.org/url-classifier/listmanager;1"].
                       getService(Ci.nsIUrlListManager);
 
+    listManager.disableAllUpdates();
+
     for (let i = 0; i < this.phishingLists.length; ++i) {
       if (this.phishingEnabled) {
         listManager.enableUpdate(this.phishingLists[i]);
-      } else {
-        listManager.disableUpdate(this.phishingLists[i]);
       }
     }
     for (let i = 0; i < this.malwareLists.length; ++i) {
       if (this.malwareEnabled) {
         listManager.enableUpdate(this.malwareLists[i]);
-      } else {
-        listManager.disableUpdate(this.malwareLists[i]);
       }
     }
     for (let i = 0; i < this.downloadBlockLists.length; ++i) {
       if (this.malwareEnabled && this.downloadsEnabled) {
         listManager.enableUpdate(this.downloadBlockLists[i]);
-      } else {
-        listManager.disableUpdate(this.downloadBlockLists[i]);
       }
     }
     for (let i = 0; i < this.downloadAllowLists.length; ++i) {
       if (this.malwareEnabled && this.downloadsEnabled) {
         listManager.enableUpdate(this.downloadAllowLists[i]);
-      } else {
-        listManager.disableUpdate(this.downloadAllowLists[i]);
       }
     }
     for (let i = 0; i < this.passwordAllowLists.length; ++i) {
       if (this.passwordsEnabled) {
         listManager.enableUpdate(this.passwordAllowLists[i]);
-      } else {
-        listManager.disableUpdate(this.passwordAllowLists[i]);
       }
     }
     for (let i = 0; i < this.trackingAnnotationLists.length; ++i) {
       if (this.trackingAnnotations) {
         listManager.enableUpdate(this.trackingAnnotationLists[i]);
-      } else {
-        listManager.disableUpdate(this.trackingAnnotationLists[i]);
       }
     }
     for (let i = 0; i < this.trackingAnnotationWhitelists.length; ++i) {
       if (this.trackingAnnotations) {
         listManager.enableUpdate(this.trackingAnnotationWhitelists[i]);
-      } else {
-        listManager.disableUpdate(this.trackingAnnotationWhitelists[i]);
       }
     }
     for (let i = 0; i < this.trackingProtectionLists.length; ++i) {
       if (this.trackingEnabled) {
         listManager.enableUpdate(this.trackingProtectionLists[i]);
-      } else {
-        listManager.disableUpdate(this.trackingProtectionLists[i]);
       }
     }
     for (let i = 0; i < this.trackingProtectionWhitelists.length; ++i) {
       if (this.trackingEnabled) {
         listManager.enableUpdate(this.trackingProtectionWhitelists[i]);
-      } else {
-        listManager.disableUpdate(this.trackingProtectionWhitelists[i]);
       }
     }
     for (let i = 0; i < this.blockedLists.length; ++i) {
       if (this.blockedEnabled) {
         listManager.enableUpdate(this.blockedLists[i]);
-      } else {
-        listManager.disableUpdate(this.blockedLists[i]);
       }
     }
     for (let i = 0; i < this.flashLists.length; ++i) {
       if (this.flashBlockEnabled) {
         listManager.enableUpdate(this.flashLists[i]);
-      } else {
-        listManager.disableUpdate(this.flashLists[i]);
       }
     }
     for (let i = 0; i < this.flashInfobarLists.length; ++i) {
       if (this.flashInfobarListEnabled) {
         listManager.enableUpdate(this.flashInfobarLists[i]);
-      } else {
-        listManager.disableUpdate(this.flashInfobarLists[i]);
       }
     }
     listManager.maybeToggleUpdateChecking();
   },
 
 
   addMozEntries() {
     // Add test entries to the DB.
--- a/toolkit/components/url-classifier/nsIUrlListManager.idl
+++ b/toolkit/components/url-classifier/nsIUrlListManager.idl
@@ -46,17 +46,22 @@ interface nsIUrlListManager : nsISupport
 
     /**
      * Turn on update checking for a table. I.e., during the next server
      * check, download updates for this table.
      */
     void enableUpdate(in ACString tableName);
 
     /**
-     * Turn off update checking for a table.
+     * Turn off update checking for all tables.
+     */
+    void disableAllUpdates();
+
+    /**
+     * Turn off update checking for a single table. Only used in tests.
      */
     void disableUpdate(in ACString tableName);
 
     /**
      * Toggle update checking, if necessary.
      */
     void maybeToggleUpdateChecking();
 
--- a/toolkit/components/url-classifier/nsUrlClassifierListManager.js
+++ b/toolkit/components/url-classifier/nsUrlClassifierListManager.js
@@ -163,18 +163,17 @@ PROT_ListManager.prototype.getGethashUrl
 PROT_ListManager.prototype.getUpdateUrl = function(tableName) {
   if (this.tablesData[tableName] && this.tablesData[tableName].updateUrl) {
     return this.tablesData[tableName].updateUrl;
   }
   return "";
 };
 
 /**
- * Enable updates for some tables
- * @param tables - an array of table names that need updating
+ * Enable updates for a single table.
  */
 PROT_ListManager.prototype.enableUpdate = function(tableName) {
   var table = this.tablesData[tableName];
   if (table) {
     log("Enabling table updates for " + tableName);
     this.needsUpdate_[table.updateUrl][tableName] = true;
   }
 };
@@ -189,18 +188,27 @@ PROT_ListManager.prototype.updatesNeeded
     if (this.needsUpdate_[updateUrl][tableName]) {
       updatesNeeded = true;
     }
   }
   return updatesNeeded;
 };
 
 /**
- * Disables updates for some tables
- * @param tables - an array of table names that no longer need updating
+ * Disable updates for all tables.
+ */
+PROT_ListManager.prototype.disableAllUpdates = function() {
+  for (const tableName of Object.keys(this.tablesData)) {
+    this.disableUpdate(tableName);
+  }
+};
+
+/**
+ * Disables updates for a single table. Avoid this internal function
+ * and use disableAllUpdates() instead.
  */
 PROT_ListManager.prototype.disableUpdate = function(tableName) {
   var table = this.tablesData[tableName];
   if (table) {
     log("Disabling table updates for " + tableName);
     this.needsUpdate_[table.updateUrl][tableName] = false;
     if (!this.updatesNeeded_(table.updateUrl) &&
         this.updateCheckers_[table.updateUrl]) {