556842 - Switch the blocklist service to use the new APIs

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Dave Townsend [:mossop]]

Should be pretty straightforward just need a method on AddonManagerPrivate to tell it to update the blocklistState for all add-ons.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Switches the blocklist service to the new APIs and migrates the tests over for it. It looks like they cover everything I could think of already.

Added a new API to the private add-ons manager, updateAddonAppDisabledStates which makes all providers check and update appDisabled for their add-ons in response to any changes.

Comment 2

[Dave Townsend [:mossop]]

Landed on the project branch: http://hg.mozilla.org/projects/addonsmgr/rev/8221fa8f0870

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 439592 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/AddonManager.jsm b/toolkit/mozapps/extensions/AddonManager.jsm
>--- a/toolkit/mozapps/extensions/AddonManager.jsm
>+++ b/toolkit/mozapps/extensions/AddonManager.jsm
>@@ -361,16 +361,27 @@ var AddonManagerInternal = {
>    */
>   notifyAddonChanged: function AMI_notifyAddonChanged(aId, aType, aPendingRestart) {
>     this.providers.forEach(function(provider) {
>       callProvider(provider, "addonChanged", null, aId, aType, aPendingRestart);
>     });
>   },
> 
>   /**
>+   * Notifies all providers they need to update the appDisabled property for
>+   * their add-ons in response to some application change such as a blocklist
>+   * update.
nit: s/to some application/to an application/

>+   */
>+  updateAddonAppDisabledStates: function AMI_updateAddonAppDisabledStates() {
>+    this.providers.forEach(function(provider) {
>+      callProvider(provider, "updateAddonAppDisabledStates");
>+    });
>+  },

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 439592 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>...
>@@ -774,105 +775,124 @@ Blocklist.prototype = {
>     }
> 
>     return Ci.nsIBlocklistService.STATE_NOT_BLOCKED;
>   },
> 
>   _blocklistUpdated: function(oldAddonEntries, oldPluginEntries) {
>     var addonList = [];
> 
>-    var em = Cc["@mozilla.org/extensions/manager;1"].
>-             getService(Ci.nsIExtensionManager);
>-    var addons = em.updateAndGetNewBlocklistedItems();
>+    var self = this;
>+    AddonManager.getAddonsByTypes(["extension", "theme", "locale"], function(addons) {
> 
nit: extra newline

Seems like this should just use getAllAddons and filter out plugins or call getAllAddons and have a single loop handle both. Perhaps a followup?

>-    for (let i = 0; i < addons.length; i++) {
>-      let oldState = -1;
>-      if (oldAddonEntries)
>-        oldState = this._getAddonBlocklistState(addons[i].id, addons[i].version,
>-                                                oldAddonEntries);
>-      let state = this.getAddonBlocklistState(addons[i].id, addons[i].version);
>-      // We don't want to re-warn about items
>-      if (state == oldState)
>-        continue;
>+      for (let i = 0; i < addons.length; i++) {
>+        let oldState = Ci.nsIBlocklistService.STATE_NOTBLOCKED;
>+        if (oldAddonEntries)
>+          oldState = self._getAddonBlocklistState(addons[i].id, addons[i].version,
>+                                                  oldAddonEntries);
>+        let state = self.getAddonBlocklistState(addons[i].id, addons[i].version);
> 
>-      addonList.push({
>-        name: addons[i].name,
>-        version: addons[i].version,
>-        icon: addons[i].iconURL,
>-        disable: false,
>-        blocked: state == Ci.nsIBlocklistService.STATE_BLOCKED,
>-        item: addons[i]
>-      });
>-    }
>+        LOG("Blocklist state for " + addons[i].id + " changed from " + oldState + " to " + state);
nitty nit: the add-on blocklist state won't always change will it?

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 439592 [details] [diff] [review]
patch rev 1

r=me. I only scanned the tests since they were mostly wrapping existing code causing whitespace changes.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I just had the Blocklist warning box pop up, saying that some of my add-ons are known to cause stability or security problems.

But the list was empty.

Looking at the actual list( https://www.mozilla.com/en-US/blocklist/ ), I think my version of the Java Deployment Toolkit actually is in the blocklist, but the warning for bad add-ons was empty.

After Restarting Firefox, it doesn't look like anything was actually disabled.

Is this related to this bug? I'm running the latest trunk build with the new EM code.

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> Is this related to this bug? I'm running the latest trunk build with the new EM
> code.

Probably caused by this bug yes, please can you file a new bug report for what you're seeing.

Comment 8

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/bb4c7abafcf7

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre (.NET CLR 3.5.30729)

I have installed the MetaStream 3 Plugin (npViewpoint.dll) which is on the blocklist. After running Firefox for a while I got the blocklisting dialog and the plugin has been disabled.