Last Comment Bug 556842 - Switch the blocklist service to use the new APIs
: Switch the blocklist service to use the new APIs
Status: VERIFIED FIXED
[rewrite]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.3a5
Assigned To: Dave Townsend [:mossop]
:
Mentors:
: 553378 (view as bug list)
Depends on:
Blocks: 461973 563168
  Show dependency treegraph
 
Reported: 2010-04-02 12:20 PDT by Dave Townsend [:mossop]
Modified: 2010-05-20 08:01 PDT (History)
4 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch rev 1 (38.52 KB, patch)
2010-04-16 12:06 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2010-04-02 12:20:12 PDT
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] 2010-04-16 12:06:17 PDT
Created attachment 439592 [details] [diff] [review]
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] 2010-04-16 12:06:54 PDT
Landed on the project branch: http://hg.mozilla.org/projects/addonsmgr/rev/8221fa8f0870
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2010-04-19 16:13:54 PDT
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 [:rstrong] (use needinfo to contact me) 2010-04-20 16:24:22 PDT
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 [:rstrong] (use needinfo to contact me) 2010-04-20 16:35:22 PDT
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) 2010-04-30 23:26:57 PDT
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] 2010-05-01 07:06:45 PDT
(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] 2010-05-11 11:03:44 PDT
http://hg.mozilla.org/mozilla-central/rev/bb4c7abafcf7
Comment 9 Henrik Skupin (:whimboo) 2010-05-18 06:42:41 PDT
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.
Comment 10 Dave Townsend [:mossop] 2010-05-20 08:01:16 PDT
*** Bug 553378 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.