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

--HG--
extra : moz-landing-system : lando

toolkit/components/url-classifier/SafeBrowsing.jsm

@@ -410,88 +410,66 @@ var SafeBrowsing = {
     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();

toolkit/components/url-classifier/nsIUrlListManager.idl

@@ -51,7 +51,12 @@ interface nsIUrlListManager : nsISupports
     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);
 

toolkit/components/url-classifier/nsUrlClassifierListManager.js

@@ -168,8 +168,7 @@ PROT_ListManager.prototype.getUpdateUrl = function(tableName) {
 };
 
 /**
- * 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];
@@ -194,8 +193,17 @@ PROT_ListManager.prototype.updatesNeeded_ = function(updateUrl) {
 };
 
 /**
- * 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];