I'm cleaning up mobile.js, and I noticed this: pref("extensions.blocklist.url", "https://addons.mozilla.org/blocklist/2/%APP_ID%/%APP_VERSION%/%PRODUCT%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/"); v2 is the old blocklisting URL. We're at v3 now. v3 has things like soft blocking and other stuff, iirc.

Since v1.0 doesn't actually use the blocklisting service, this shouldn't block that release, but it should be fixed for the future version.

Nevermind... was thinking of the malware site blocklist, not this one.

(In reply to comment #0) > v2 is the old blocklisting URL. We're at v3 now. v3 has things like soft > blocking and other stuff, iirc. We don't have UI for softblocking. Is there any other difference? Do both versions contain the same data (apart from the soft blocks)?

Someone needs to test that blocklisting is working right now. We're certainly pinging for the updates, I think, but there may be UI hooks it depends on?

Morgamic: need a web-dev answer to comment 3 about the content differences between the v2 and v3 blocklisting URL

(In reply to comment #5) > Morgamic: need a web-dev answer to comment 3 about the content differences > between the v2 and v3 blocklisting URL http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/webroot/services/blocklist.php?view=markup Looks like the differences are that v3: * Supports specifying specific applications * Supports specifying version ranges (and only for specific products and their version ranges) Mossop, is that correct?

v3 adds support for targetApplication information for plugins. Fennec should update to use it. You'll be getting softblockling and stuff regardless of using v2 or v3 urls.

(In reply to comment #4) > Someone needs to test that blocklisting is working right now. We're certainly > pinging for the updates, I think, but there may be UI hooks it depends on? Looks like there is: * http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#68 * http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/list.js#80

Mozilla-central changes. Adds an nsIBlockedAddonPrompt called when addons are blocked due to a newly downloaded blocklist.

Implemented the nsIBlockedAddonPrompt interface. I made this show a notification popup for now, although I'm guessing that the preferred UI is debatable. This also checks for addoninstalls that are blocked due to a blocklist, and shows an alert indicating why they were rejected. There's probably a million little side cases here. Posting these for initial feedback.

Comment on attachment 469358 [details] [diff] [review] Mozilla Central changes Some drive by feedback: * I was initially thinking nsIBlocklistPrompt, but as long as Mossop is happy, we are happy * Why not put nsIBlockedAddonPrompt in the nsIBlocklistService.idl file? * You remove some blank lines that should probably stay.

Comment on attachment 469359 [details] [diff] [review] Mobile-browser changes >diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js >- this._showAlert(true); >+ this._showIntallCompleteAlert(true); showIntallCompleteAlert -> showInstallCompleteAlert >+ _showAlert: function xpidm_showAlert(aMessage) { > if (ExtensionsView.visible) > return; >- > let strings = Elements.browserBundle; >- let message = aSucceeded ? strings.getString("alertAddonsInstalled") : >- strings.getString("alertAddonsFail"); >- > let observer = { Don't kill the nice blank lines I need to look more closely at how we handle various blocklist situations.

Moved the interface. This should actually work.

Comment on attachment 469501 [details] [diff] [review] Mozilla Central changes Let's get Mossop to review. We also need to find a way to test this out for Fennec.

Comment on attachment 469502 [details] [diff] [review] Mobile-browser changes >diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js >+ onDownloadCancelled: function(aInstall, aAddon) { >+ if(aInstall.addon.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) { >+ this._showBlockedAlert(); >+ } nits: * Space between "if" and "(" * No {} needed around one liner >- this._showAlert(true); >+ this._showIntallCompleteAlert(true); Typo: showInstallCompleteAlert >diff --git a/components/BlockedAddonPrompt.js b/components/BlockedAddonPrompt.js >+BlockedAddonPrompt.prototype = { >+ prompt: function(aAddons, aCount) { >+ let win = Services.wm.getMostRecentWindow("navigator:browser"); >+ win.ExtensionsView.showRestart("blocked"); >+ if (win.ExtensionsView.visible) >+ return; Should we only show the ExtensionsView restart _if_ the ExtensionView is visible? Why show the ExtensionView restart _and_ the main notification area restart? If we really should then ok, but I'm just checking. nit: add a blank line after the "return;" >+ notifyBox.appendNotification(bundle.GetStringFromName("notificationRestart.blocked"), >+ "blocked-add-on", >+ "chrome://browser/skin/images/alert-addons-30.png", >+ "PRIORITY_CRITICAL_HIGH", >+ buttons); I think we ignore any image passed to appendNotification. You can just pass "" I have not worked much with blocking add-ons, so we need to find some good test cases (look in Firefox and talk to Mossop) so we can test the various situation where blocklisting can happen. r+, but let's fix the nits and figure out the restart in ExtensionView behavior. Oh and find ways to test! We can't land this before we have tested various situations.

Comment on attachment 469501 [details] [diff] [review] Mozilla Central changes This part of the blocklist service really needs to be updated to use the new add-ons manager API more fully but it didn't seem worthwhile previously. I'm guessing you guys don't have the time to do it right now either so I suppose we'll muddle on as-is and do it post-Firefox 4. The prompting API proposed here doesn't seem right to me. It looks like you're doing your work asynchronously yet as soon as you return the service goes ahead and disables the soft-blocked items. I think instead if the nsIBlockedAddonPrompt service exists then it should handle the disabling of soft-blocked items itself, so just call into it and do nothing afterwards. This would be cleaner if the code was using the new API's but should just work as-is anyway. As for naming I think "@mozilla.org/addons/blocklist-prompt;1" and "nsIBlocklistPrompt" is probably a better choice.

Addressed I think. The download canceled code is now stolen from Firefox: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#718 and handles a few other situations. Added support to showing blocked and softblocked in the add-ons manager. This probably needs some UX-review. There are two cases: 1.) User installs addon, blocklist is downloaded later the blocks (hard or soft blocking) the add-on In this case, this shows a notification bar with a restart button added. If you dismiss that alert, and later visit the add-ons manager (without having restarted), there is a notification bar telling you that something has been blocklisted and you should restart (this is probably overkill) 2.) User tries to install an addon that we've already blocklisted If the addon is hard-blocked we refuse to install and show a slide-in notification If the addon is soft-blocked we just allow the install like always, but the addon will have a badge in the add-ons manager. For testing this, I have been using some stuff on my dropbox: https://dl.dropbox.com/u/72157/Testing/extensions/installExtensions.html Two extensions, and a custom blocklist. The first one ("Hello World") is hard blocked in the list. The second is soft-blocked. You have to change some prefs to point the blocklist to a local file. Testing the second case above is easy. Testing the first, you have to delete your blocklist. Flipping the extensions.blocklist.enabled pref should trigger that, but it never does for me. So I just set the timer to be short and wait a bit. It's a pain. Related Firefox bugs: bug 455906, bug 391714

Name change. And we can handle our own soft blocking.

Comment on attachment 470825 [details] [diff] [review] Mobile browse > let updateable = (addon.permissions & AddonManager.PERM_CAN_UPDATE) > 0; > let uninstallable = (addon.permissions & AddonManager.PERM_CAN_UNINSTALL) > 0; Add the | let blockedString =""; | section of code here, not below. And rename "blockedString" -> "blocked" Oh, and make sure a blank line is above and below the section :) > listitem.setAttribute("updateable", updateable); > listitem.setAttribute("isReadonly", !uninstallable); >+ let blockedString = ""; >+ switch(addon.blocklistState) { >+ case Ci.nsIBlocklistService.STATE_BLOCKED: >+ blockedString = strings.getString("addon.item.blocked") >+ break; >+ case Ci.nsIBlocklistService.STATE_SOFTBLOCKED: >+ blockedString = strings.getString("addon.item.softBlocked"); >+ break; >+ case Ci.nsIBlocklistService.STATE_OUTDATED: >+ blockedString = srings.getString("addon.item.outdated"); >+ break; >+ } >+ listitem.setAttribute("blockedStatus", blockedString); Just the | listitem.setAttribute("blockedStatus", blocked); | should be here. No blank lines needed :) >+ onDownloadCancelled: function(aInstall, aAddon) { >+ let strings = Elements.browserBundle; >+ let brandBundle = document.getElementById("bundle_brand"); >+ let brandShortName = brandBundle.getString("brandShortName"); >+ let host = (aInstall.originatingURI instanceof Ci.nsIStandardURL) && >+ aInstall.originatingURI.host; No need to wrap >+ if (!host) >+ host = (aInstall.sourceURI instanceof Ci.nsIStandardURL) && >+ aInstall.sourceURI.host; No need to wrap >diff --git a/components/BlocklistPrompt.js b/components/BlocklistPrompt.js "If you dismiss that alert, and later visit the add-ons manager (without having restarted), there is a notification bar telling you that something has been blocklisted and you should restart (this is probably overkill)" I agree, it's probably overkill. Does that happen here? >+ prompt: function(aAddons, aCount) { >+ let win = Services.wm.getMostRecentWindow("navigator:browser"); >+ win.ExtensionsView.showRestart("blocked"); >+ if (win.ExtensionsView.visible) >+ return; Is the overkill part here? Maybe we should just do the | win.ExtensionsView.showRestart("blocked"); | _if_ the extensions view is visible. That way we only show the user one notification. If they are in the Add-on Manager they get the add-on notification. If they are in the browser, they get the notificationBox one. >+ // Disable softblocked items automatically >+ for (let i = 0; i < aAddons.length; i++) { >+ if (aAddons[i].item instanceof Ci.nsIPluginTag) >+ addonList[i].item.disabled = true; >+ else >+ aAddons[i].item.userDisabled = true; >+ } In this patch, we don't disable softblocked items if the extension view is open, because we return early. Is that a problem? or do we disbale them elsewhere? >diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties > alertAddonsFail=Installation failed Move the "alertAddonsBlocked" line here, with the other add-on alert strings >+alertAddonsBlocked=Unsafe add-ons installed >+addon.item.blocked=Blocked >+addon.item.softBlocked=Known to cause security or stability issues >+addon.item.outdated=Out of date Instead of "addon.item." let's use "addonBlocked." and move up to a new group just below the "addonUpdate." section. >+addonError-1=The add-on could not be downloaded because of a connection failure on #2. >+addonError-2=The add-on from #2 could not be installed because it does not match the add-on #3 expected. >+addonError-3=The add-on downloaded from #2 could not be installed because it appears to be corrupt. >+addonError-4=#1 could not be installed because #3 cannot modify the needed file. We need a localization note here too >+# LOCALIZATION NOTE (addonLocalError-1, addonLocalError-2, addonLocalError-3, addonLocalError-4, addonErrorIncompatible, addonErrorBlocklisted): >+# #1 is the add-on name, #3 is the application name, #4 is the application version >+addonLocalError-1=This add-on could not be installed because of a filesystem error. >+addonLocalError-2=This add-on could not be installed because it does not match the add-on #3 expected. >+addonLocalError-3=This add-on could not be installed because it appears to be corrupt. >+addonLocalError-4=#1 could not be installed because #3 cannot modify the needed file. >+addonErrorIncompatible=#1 could not be installed because it is not compatible with #3 #4. >+addonErrorBlocklisted=#1 could not be installed because it has a high risk of causing stability or security problems. Let's move these two error blocks above the download manager section, keeping the addon manager strings together. Overall, this patch looks good to me. Just some nits to clean up. Also, the "overkill" and "auto softblock disable questions.

Nits addressed and tested again. Thanks for catching the Extensions view open case.

Comment on attachment 471176 [details] [diff] [review] Mobile-browser changes looks good

Comment on attachment 470829 [details] [diff] [review] Mozilla Central changes Want to see the updated comments but otherwise this is fine >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 >@@ -857,32 +857,40 @@ Blocklist.prototype = { > disable: false, > blocked: state == Ci.nsIBlocklistService.STATE_BLOCKED, > item: plugins[i] > }); > } > } > plugins[i].blocklisted = state == Ci.nsIBlocklistService.STATE_BLOCKED; > } >- Please don't remove this empty line > if (addonList.length == 0) > return; > >+ if ("@mozilla.org/addons/blocklist-prompt;1" in Cc) { >+ try { >+ let blockedPrompter = Cc["@mozilla.org/addons/blocklist-prompt;1"] >+ .getService(Ci.nsIBlocklistPrompt); >+ blockedPrompter.prompt(addonList); >+ } catch (e) { >+ LOG(e); >+ } >+ return; >+ } Add an empty line after this > var args = { > restart: false, > list: addonList > }; > // This lets the dialog get the raw js object > args.wrappedJSObject = args; > > var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. > getService(Ci.nsIWindowWatcher); > ww.openWindow(null, URI_BLOCKLIST_DIALOG, "", > "chrome,centerscreen,dialog,modal,titlebar", args); >- Please don't remove this empty line >diff --git a/xpcom/system/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl >+/** >+ * nsIBlockedAddonPrompt is used, if available, by the default implementation of >+ * nsIBlocklistService to display a confirmation UI to the user before blocking >+ * extensions/plugins. Correct the naming in the comment >+[scriptable, uuid(36f97f40-b0c9-11df-94e2-0800200c9a66)] >+interface nsIBlocklistPrompt : nsISupports >+{ >+ /** >+ * Prompt the user about newly blocked addons. The prompt is then resposible >+ * for soft-blocking any addons that need to be afterwards >+ * >+ * @param aAddons >+ * An array of addons and plugins that are blocked Could you add a little to the comment to describe the nature of the objects being passed here. >+ * @param aCount >+ * The number of addons >+ */ >+ void prompt([array, size_is(aCount)] in nsIVariant aAddons, >+ [optional] in PRUint32 aCount); Line up the arguments

Whitespace returns! Sorry about that...

pushed m-c: http://hg.mozilla.org/mozilla-central/rev/d81c3c24a14b pushed m-b: http://hg.mozilla.org/mobile-browser/rev/f482950a917d

Assigning to self to create a litmus testcase.

(In reply to comment #26) > Assigning to self to create a litmus testcase. The assignee field is only for the developer who wrote the patch.

Verified fix on 0921 android and n900 nightly Added litmus test: https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=12939