Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support severities for blocklist entries

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1b2
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(9 attachments, 16 obsolete attachments)

110.45 KB, image/png
Details
68.62 KB, image/png
Details
87.37 KB, image/png
Details
86.20 KB, image/png
Details
48.30 KB, image/png
Details
43.12 KB, image/png
Details
126.37 KB, patch
rstrong
: review+
jst
: review+
Details | Diff | Splinter Review
934 bytes, application/xml
Details
28.01 KB, image/png
Details
(Assignee)

Description

9 years ago
The add-on blocklist service needs to support the concept of severities for blocks and allow for customising what level is a hard block and what is a warning.
(Assignee)

Updated

9 years ago
Blocks: 391714

Updated

9 years ago
Flags: blocking1.9.1?
(Assignee)

Updated

9 years ago
Blocks: 422900
(Assignee)

Comment 1

9 years ago
First round of this patch is on its way and will cover notification when the blocklist is updated for plugins and add-ons and notification during installation of xpis.

Plugin detection and add-ons detected during startup will not have any notification in the first patch, instead those that are blocked above the threshold will be app disabled, the others that are still listed in the blocklist will be disabled but can be re-enabled later by the user.

Screenshots for UI review to follow...
(Assignee)

Comment 2

9 years ago
Created attachment 344300 [details]
Blocklist updated, new items have been hard blocked
(Assignee)

Comment 3

9 years ago
Created attachment 344301 [details]
Blocklist updated, new items have been soft blocked
(Assignee)

Comment 4

9 years ago
Created attachment 344302 [details]
Blocklist updated, some new hard blocks, some new soft blocks
(Assignee)

Comment 5

9 years ago
Created attachment 344303 [details]
User attempts to install an xpi that is soft blocked
(Assignee)

Comment 6

9 years ago
Created attachment 344304 [details]
User attempts to install an xpi that is hard blocked
(Assignee)

Comment 7

9 years ago
I need a skim over the words used in the example screenshots, my wording is probably not right, trying to clarify the difference between the soft and hard block to the user is not easy.

Comment 8

9 years ago
a few testcases to be created here once this is finalized.
Flags: in-litmus?
Comment on attachment 344303 [details]
User attempts to install an xpi that is soft blocked

Shouldn't the default button be cancel since we are recommending not to install soft blocked add-ons?
These look pretty good.  I had a few suggestions, which mainly attempt to be more consistent with our language and description to the user of what's going on.

1. New items have been hard blocked

In this screenshot, I think it's a bit unclear whether the the user is disabling the add-ons by restarting, or whether they have already been disabled.  The texts says they have "now been disabled," and the text asks the user to "disable and restart."

Mossop, have these hard-block addons been disabled at the point the user sees this message, or are they disabled at the next restart?  If the former, the message should be more informative - telling the user what has happened and clicking OK.  If the latter, then even if the user dismisses this message the add-ons will still be disabled at the next restart - that's more of a "later" than "cancel," which suggests the user is canceling the block itself.  I'd change the title of this window based on which one is the case as well.

2. New items have been soft-blocked

The language we're using to describe soft-block addons here is inconsistent.  We haven't verified them as dangerous, so my though is that we should be steering more towards they "may be a security risk" than they are "dangerous add-ons."  I would change the text to "It is highly recommended that you restart with these add-ons disabled for your protection."  Would change title to: "Warning: Add-ons may be instable or insecure."

3. Some soft-block, some hard-block

As with #2 above, I'd change text to:  "The add-ons that have been verified as security threats have been blocked.  It is highly recommended that you restart with the other add-ons disabled for your protection."  Would change title to "Warning: Add-ons may be instable or insecure."

4. Soft-block install

Would change title to: "Warning: Add-on may be insecure or unstable"

5. Hard-block install

Would change title to: Warning: Add-on cannot be installed."
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> These look pretty good.  I had a few suggestions, which mainly attempt to be
> more consistent with our language and description to the user of what's going
> on.
> 
> 1. New items have been hard blocked
> 
> In this screenshot, I think it's a bit unclear whether the the user is
> disabling the add-ons by restarting, or whether they have already been
> disabled.  The texts says they have "now been disabled," and the text asks the
> user to "disable and restart."
> 
> Mossop, have these hard-block addons been disabled at the point the user sees
> this message, or are they disabled at the next restart?  If the former, the
> message should be more informative - telling the user what has happened and
> clicking OK.  If the latter, then even if the user dismisses this message the
> add-ons will still be disabled at the next restart - that's more of a "later"
> than "cancel," which suggests the user is canceling the block itself.  I'd
> change the title of this window based on which one is the case as well.

You must restart for them to be properly unloaded and so the user be safe. Once this dialog appears they have been marked to never load again but they remain in memory till the next time Firefox is closed.

> 2. New items have been soft-blocked
> 
> The language we're using to describe soft-block addons here is inconsistent. 
> We haven't verified them as dangerous, so my though is that we should be
> steering more towards they "may be a security risk" than they are "dangerous
> add-ons."  I would change the text to "It is highly recommended that you
> restart with these add-ons disabled for your protection."  Would change title
> to: "Warning: Add-ons may be instable or insecure."

I'm a little wary of just describing things as potential security risks. We use the blocklist to cover both security and stability issues, there isn't one word to describe that I think. I also don't think we will be adding things to the blocklist unless we have verified them as a problem. Another problem is that the user can change what level the block becomes soft or hard, meaning the specific issue with an add-on could be different.

> 3. Some soft-block, some hard-block
> 
> As with #2 above, I'd change text to:  "The add-ons that have been verified as
> security threats have been blocked.  It is highly recommended that you restart
> with the other add-ons disabled for your protection."  Would change title to
> "Warning: Add-ons may be instable or insecure."
> 
> 4. Soft-block install
> 
> Would change title to: "Warning: Add-on may be insecure or unstable"
> 
> 5. Hard-block install
> 
> Would change title to: Warning: Add-on cannot be installed."

These titles feel very inconsistent with what we use elsewhere in the product to me.
(Assignee)

Comment 12

9 years ago
Created attachment 344469 [details] [diff] [review]
patch rev 1

Still need to do further revisions on the strings but that shouldn't stop us getting the code reviewed and it's not the smallest patch in the world...

There are a couple of interface changes to note. nsIBlocklistService has additive changes and shouldn't break any existing js users. nsIExtensionManager has a breaking change, however the method removed should never be getting called by anyone. If that is a real problem though we can just leave it in and make it do nothing.

There will probably need to be a change to the blocklist url, that will come when the server side is updated.

I've created the new blocklist.xul UI for the notification, list.xul was getting too overloaded for it to make sense anymore.

nsIBlocklistService.idl has been moved to the gecko tier to make it visible to the plugin host (final location still to be determined). This allows the plugin host to check for blocklisting when it detects new plugins as opposed to the blocklist having to go through the entire list whenever it changes.

Updated the test for bug 455906 to also cover notification for plugins, and added a test for the new notifications including blocking and warning.
Attachment #344469 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 13

9 years ago
Comment on attachment 344469 [details] [diff] [review]
patch rev 1

jst, can you review the changes to nsPluginHostImpl.cpp. Also bsmedberg said you might have a better idea of where to put nsIBlocklistService.idl?
Attachment #344469 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Thanks for the helpful clarifications, Mossop.

1. New Items have been hard blocked

The language regarding whether or not the add-ons have been blocked is still a bit confusing.  How about for the text under the list:

"The add-ons that have been verified as security risks have been blocked, but a restart is required to remove them completely."

Button: 

"Remove Add-ons and Restart Minefield"

2. New items have been soft-blocked.

Mossop said that soft-blocks are for verified problems, not just potential risks, so I agree the language should be stronger. Only potential problem with the current form is that "the dangerous add-ons" seems to imply a subset of those listed in the box.  And, we've already said that they may be unstable or insecure (above the box)  So, how about for text under the list:

"For your protection, it is highly recommended that you restart with these add-ons disabled."

3. Some hard, some soft

To be consistent with above:

"The add-ons that have been verified as security risks have been blocked.  For your protection, it is highly recommended that you restart with the other add-ons disabled."

4. - 5. Soft & hard block install

After looking through Firefox's other error messages, the language in these in their current form looks good!
(Assignee)

Comment 15

9 years ago
(In reply to comment #14)
> Button: 
> 
> "Remove Add-ons and Restart Minefield"

This isn't really appropriate since we aren't removing the add-ons we are simply blocking them from loading. They will still appear in the add-ons manager and the user can later update to a new stable version.

Fixing up the patch for the other bits now.
(Assignee)

Comment 16

9 years ago
I'm also going to add a warning in the add-ons manager for items that have been warned about: "This add-on may expose you to security risks."
Re Comment 15: 

Fair enough.  How about:

"The add-ons that have been verified as security risks have been blocked, but a
restart is required to disable them completely."

Button: 

"Disable Add-ons and Restart Minefield"

Re Comment 16: Sounds good
(Assignee)

Comment 18

9 years ago
Created attachment 345067 [details]
Blocklist updated, new items have been hard blocked
Attachment #344300 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
Created attachment 345068 [details]
Blocklist updated, new items have been soft blocked
Attachment #344301 - Attachment is obsolete: true
(Assignee)

Comment 20

9 years ago
Created attachment 345070 [details]
Blocklist updated, some new hard blocks, some new soft blocks
Attachment #344302 - Attachment is obsolete: true
(Assignee)

Comment 21

9 years ago
Created attachment 345071 [details]
Warning in the add-ons manager

Updated screenshots for the previous views above, these should be good to go now I assume. This is the warning in the add-ons manager that hopefully will notify people that they shouldn't be using certain add-ons.
(Assignee)

Comment 22

9 years ago
Created attachment 345072 [details]
Add-ons manager warning when unselected

Slightly better view. The warning displays even when the item is unselected to indicate to the user that they are running with something that may be dangerous
(Assignee)

Comment 23

9 years ago
Created attachment 345074 [details] [diff] [review]
patch rev 2

Updated patch with the new strings and additional warning.

Rob if you could look at the string usage so I can land them before the freeze on Thursday that would be great.
Attachment #344469 - Attachment is obsolete: true
Attachment #345074 - Flags: review?(robert.bugzilla)
Attachment #344469 - Flags: review?(robert.bugzilla)
Attachment #344469 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
Attachment #345074 - Flags: review?(jst)
I should be able to get to it by tomorrow evening.
Comment on attachment 345072 [details]
Add-ons manager warning when unselected

One possible way to draw attention besides the already overloaded exclamation point (which is really to draw attention to there being additional info) is to use a different background / text color for add-ons in this state... just a thought.

Also, did you intentionally make the text right align as in the screenshot?
(Assignee)

Comment 26

9 years ago
(In reply to comment #25)
> (From update of attachment 345072 [details])
> One possible way to draw attention besides the already overloaded exclamation
> point (which is really to draw attention to there being additional info) is to
> use a different background / text color for add-ons in this state... just a
> thought.

I'll chat with Boriss about this since we have all sorts of different ways we want to draw attention now and she has already been thinking about some of them. I think for now this will probably do but if something good comes up I'll add it in.

> Also, did you intentionally make the text right align as in the screenshot?

That is how those statuses have always displayed on mac
These all look good, let's push them out.  

Re Comment 26 - I do think there's better ways to highlight add-ons in this list, but since this is already standard perhaps we should deal with this in another bug or overhaul it completely when we fix the add-on manager.
(In reply to comment #26)
>...
> > Also, did you intentionally make the text right align as in the screenshot?
> 
> That is how those statuses have always displayed on mac
I'm surprised there isn't a bug on this... they should all be left aligned and there should be an exclamation point image to the left (red for issues such as blocklist, incompatible, etc. and green for info such as update available).
(Assignee)

Comment 29

9 years ago
(In reply to comment #28)
> (In reply to comment #26)
> >...
> > > Also, did you intentionally make the text right align as in the screenshot?
> > 
> > That is how those statuses have always displayed on mac
> I'm surprised there isn't a bug on this... they should all be left aligned and
> there should be an exclamation point image to the left (red for issues such as
> blocklist, incompatible, etc. and green for info such as update available).

Sorry I can add the exclamation point to the warning items, I hadn't noticed it wasn't there. I'll try to figure out where the right align came from
(Assignee)

Comment 30

9 years ago
Created attachment 345120 [details] [diff] [review]
patch rev 3

Adds the red notification for the warning state.

The right alignment on mac came from bug 397723, specifically http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/toolkit/themes/pinstripe/mozapps/extensions&command=DIFF_FRAMESET&file=extensions.css&rev2=1.33&rev1=1.32

I'll catch up with Boriss later and see what she thinks about that, I'll file it separately if it warrants a change.
Attachment #345074 - Attachment is obsolete: true
Attachment #345120 - Flags: review?(robert.bugzilla)
Attachment #345120 - Flags: review?(jst)
Attachment #345074 - Flags: review?(robert.bugzilla)
Attachment #345074 - Flags: review?(jst)
Comment on attachment 345120 [details] [diff] [review]
patch rev 3

A lot more has changed than I expected so I am adding my string change recommendations before I've finished the review

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/blocklist.dtd
>@@ -0,0 +1,10 @@
>+<!ENTITY blocklist.title             "Add-ons may be causing problems">
>+<!ENTITY blocklist.style             "width: 45em; height: 30em">
>+<!ENTITY blocklist.summary           "&brandShortName; has determined that the following add-ons may be unstable or insecure:">
For consistency with the other new strings:
&brandShortName; has determined that the following add-ons are known to cause stability or security problems.

>+<!ENTITY blocklist.warning           "For your protection, it is highly recommended that you restart with these add-ons disabled.">
"with these add-ons disabled" is a tad awkward.
Perhaps:
For your protection, it is highly recommended that you restart to disable these add-ons.
along with a note that unchecking disable is not recommended.

>+<!ENTITY blocklist.blocked           "The add-ons that have been verified as security risks have been blocked, but a restart is required to disable them completely.">
If the above is changed perhaps:
For your protection, it is highly recommended that you restart to disable these add-ons.

>+<!ENTITY blocklist.blockedandwarn    "The add-ons that have been verified as security risks have been blocked. For your protection, it is highly recommended that you restart with the other add-ons disabled.">
"with the other add-ons disabled" is awkward.

>+<!ENTITY blocklist.moreinfo          "More information">
>+
>+<!ENTITY blocklist.accept.label      "Disable Add-ons and Restart &brandShortName;">
This is a tad wordy for a button especially since it is already stated immediately above that restarting is required to disable the blocked add-ons. How about just "Restart &brandShortName;"?

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>--- a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd
>@@ -107,16 +107,17 @@
> <!ENTITY includeUpdate.label              "Include this update">
> <!ENTITY includeUpdate.accesskey          "n">
> <!ENTITY includeUpdate.tooltip            "Include this Add-on when installing the updates">
> 
> <!-- Status Messsages -->
> <!ENTITY insecureUpdate.label             "Does not provide secure updates.">
> <!ENTITY needsDependencies.label          "Requires additional items.">
> <!ENTITY blocklisted.label                "Disabled for your protection.">
>+<!ENTITY blocklistWarning.label           "This add-on may expose you to security risks.">
"expose you" seems wrong though it may be appropriate since using you may indicate to the user the importance of this. "This add-on" seems somewhat superfluous in that all other messages within the add-on don't use "This add-on" since it is obvious in order to save space. I'm a tad concerned about how elsewhere stability and security problems is called out and that here it only states "security risks" since if it is important to call out both elsewhere then it should be just as important to call out both here.
I'm not happy with it but it is a start:
Known to cause security or stability problems.

Can you provide a screenshots of the case where the add-on is disabled and selected / unselected to the blocklisted.label is also shown?
Comment on attachment 345120 [details] [diff] [review]
patch rev 3

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>--- a/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties
>@@ -54,21 +54,20 @@ incompatibleExtension=Disabled - not com
> incompatibleExtension=Disabled - not compatible with %S %S
> incompatibleAddonMsg=Not compatible with %S %S
> insecureUpdateMessage="%S" will not be installed because it does not provide secure updates
> 
> invalidGUIDMessage="%S" could not be installed because of an error in its Install Manifest ("%S" is not a valid GUID). Please contact the author of this item about the problem.
> invalidVersionMessage="%S" could not be installed because of an error in its Install Manifest ("%S" is not a valid Version String). Please contact the author of this item about the problem.
> incompatiblePlatformMessage="%S" could not be installed because it is not compatible with your %S build type (%S). Please contact the author of this item about the problem.
> 
>-blocklistedInstallTitle=This extension is not secure
>-blocklistedInstallMsg=The extension %S is known to be dangerous, and can't be installed.
>-blocklistNotifyTitle2=Add-ons may be causing problems
>-blocklistNotifyMsg2=%S has determined that the following add-ons may be unstable or insecure.
>-blocklistRestartMsg2=You should restart %S so that these add-ons can be disabled.
>+blocklistedInstallTitle2=This add-on is not secure
Since these are being changed I suggest consistency between the two titles though it may be a good thing to use something more descriptive than dangerous in these strings
This add-on is dangerous to use

>+blocklistedInstallMsg2=The add-on %S causes stability or security problems and can't be installed.
>+warningInstallTitle=This add-on may be dangerous to use
>+warningInstallMsg=The add-on %S is known to cause stability or security problems and should not be installed. Do you wish to continue with the installation?
(Assignee)

Comment 33

9 years ago
Created attachment 345275 [details]
selected disabled add-on with warning

Currently it still displays the warning message even when disabled. Perhaps a better option there might be to hide the warning message but display the warning dialog when they try to enable the same as when they install? Or maybe only the warning on selection when disabled.
(Assignee)

Comment 34

9 years ago
Created attachment 345276 [details]
unselected disabled add-on with warning
Comment on attachment 345120 [details] [diff] [review]
patch rev 3

>diff --git a/toolkit/themes/winstripe/mozapps/extensions/blocklist.css b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css
>@@ -0,0 +1,16 @@
>+richlistitem {
>+  padding-top: 6px;
>+  padding-bottom: 6px;
>+  -moz-padding-start: 7px;
>+  -moz-padding-end: 7px;
>+  border-bottom: 1px solid #C0C0C0;
>+}
>+
>+.addon-name-version {
>+  font-size: 110%;
>+}
nit: is this really better than going with the default size? I'll leave it up to you to decide.

>+.blockedLabel {
>+  color: red;
>+  font-style: italic;
>+}
Hardcoding the color will cause problems with custom themes (in this case if my list background color is red then the text won't be displayed
Also, this appears in blocklist.css and the class blockedLabel is only used in extensions.xml which does not include blocklist.css.

>diff --git a/toolkit/themes/winstripe/mozapps/extensions/extensions.css b/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>@@ -152,16 +152,17 @@ richlistitem[selected="true"]:not([opTyp
>   display: none;
> }
> 
> richlistitem[availableUpdateURL][updateable="true"] .updateBadge,
> richlistitem[availableUpdateURL][updateable="true"] .updateAvailableBox,
> richlistitem[compatible="false"] .notifyBadge,
> richlistitem[providesUpdatesSecurely="false"] .notifyBadge,
> richlistitem[blocklisted="true"] .notifyBadge,
>+richlistitem[warning="true"] .notifyBadge,
> richlistitem[satisfiesDependencies="false"] .notifyBadge {
>   display: -moz-box;
> }
> 
> /* Selected Add-on buttons
>    See content/extensions.css to hide / display buttons */
> .selectedButtons {
>   margin-top: 4px;
>@@ -184,17 +185,20 @@ richlistitem[satisfiesDependencies="fals
> .uninstallButton, .cancelUninstallButton {
>   -moz-margin-start: 5px;
> }
> 
> /* Selected Add-on status messages and images */
> richlistitem[compatible="true"] .incompatibleBox,
> richlistitem[providesUpdatesSecurely="true"] .insecureUpdateBox,
> richlistitem[satisfiesDependencies="true"] .needsDependenciesBox,
>-richlistitem[blocklisted="false"] .blocklistedBox,
>+richlistitem:not([blocklisted="true"]):not([warning="true"]) .blocklistedBox,
>+richlistitem[warning="false"]:not([selected="true"]) .blocklistedBox,
>+richlistitem[blocklisted="false"] .blocklistedLabel,
>+richlistitem[warning="false"] .warningLabel,
What do you think about going with blocklistedhard and blocklistedsoft instead of blocklisted and warning? My concern is that warning is too generic and can be applied to other things.

> richlistitem[opType="needs-uninstall"] .blocklistedBox,
> richlistitem[opType="needs-uninstall"] .incompatibleBox,
> richlistitem[opType="needs-uninstall"] .needsDependenciesBox,
> richlistitem[opType="needs-uninstall"] .blocklistedBox {
>   display: none;
> }
> 
> richlistitem[loading="true"] .updateBadge {

>diff --git a/toolkit/mozapps/extensions/public/nsIBlocklistService.idl b/xpcom/system/nsIBlocklistService.idl
>rename from toolkit/mozapps/extensions/public/nsIBlocklistService.idl
>rename to xpcom/system/nsIBlocklistService.idl
>--- a/toolkit/mozapps/extensions/public/nsIBlocklistService.idl
>+++ b/xpcom/system/nsIBlocklistService.idl
>@@ -34,19 +34,29 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> 
> #include "nsISupports.idl"
> 
>-[scriptable, uuid(0c3fe697-d50d-4f42-b747-0c5855cfc60e)]
>+interface nsIPluginTag;
>+
>+[scriptable, uuid(8439f9c0-da03-4260-8b21-dc635eed28fb)]
> interface nsIBlocklistService : nsISupports
> {
>+  // Indicates that the item does not appear in the blocklist.
>+  const unsigned long STATE_UNBLOCKED = 0;
Should be NOT_BLOCKED or something similar since UNBLOCKED implies that it was unblocked when it may have never been blocked.

>+  // Indicates that the item is in the blocklist but the problem is not severe
>+  // enough to warant forcibly blocking.
>+  const unsigned long STATE_WARN      = 1;
>+  // Indicates that the item should be blocked and never used.
>+  const unsigned long STATE_BLOCKED   = 2;
>+
>   /**
>    * Determine if an item is blocklisted
>    * @param   id
>    *          The GUID of the item.
>    * @param   version
>    *          The item's version.
>    * @param   appVersion
>    *          The version of the application we are checking in the blocklist.
>@@ -55,10 +65,49 @@ interface nsIBlocklistService : nsISuppo
>    * @param   toolkitVersion
>    *          The version of the toolkit we are checking in the blocklist.
>    *          If this parameter is null, the version of the running toolkit
>    *          is used.
>    * @returns true if the item is compatible with this version of the
>    *          application or this version of the toolkit, false, otherwise.
>    */
>   boolean isAddonBlocklisted(in AString id, in AString version,
>-                             in AString appVersion, in AString toolkitVersion);
>+                             [optional] in AString appVersion,
>+                             [optional] in AString toolkitVersion);
>+
>+  /**
>+   * Determine the blocklist state of an add-on
>+   * @param   id
>+   *          The GUID of the item.
nit: Since we changed to our ID format these are technically no longer GUIDs... just change it to ID

>+   * @param   version
>+   *          The item's version.
>+   * @param   appVersion
>+   *          The version of the application we are checking in the blocklist.
>+   *          If this parameter is null, the version of the running application
>+   *          is used.
>+   * @param   toolkitVersion
>+   *          The version of the toolkit we are checking in the blocklist.
>+   *          If this parameter is null, the version of the running toolkit
>+   *          is used.
>+   * @returns The STATE constant..
nit: extra trailing period

>+   */
>+  unsigned long getAddonBlocklistState(in AString id, in AString version,
>+                                       [optional] in AString appVersion,
>+                                       [optional] in AString toolkitVersion);
>+
>+  /**
>+   * Determine the blocklist state of a plugin
>+   * @param   plugin
>+   *          The plugin to get the state for
>+   * @param   appVersion
>+   *          The version of the application we are checking in the blocklist.
>+   *          If this parameter is null, the version of the running application
>+   *          is used.
>+   * @param   toolkitVersion
>+   *          The version of the toolkit we are checking in the blocklist.
>+   *          If this parameter is null, the version of the running toolkit
>+   *          is used.
>+   * @returns The STATE constant..
nit: extra trailing period
(In reply to comment #35)
> (From update of attachment 345120 [details] [diff] [review])
> >diff --git a/toolkit/themes/winstripe/mozapps/extensions/blocklist.css b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/themes/winstripe/mozapps/extensions/blocklist.css
> >@@ -0,0 +1,16 @@
>...
> >+.blockedLabel {
> >+  color: red;
> >+  font-style: italic;
> >+}
> Hardcoding the color will cause problems with custom themes (in this case if my
> list background color is red then the text won't be displayed
> Also, this appears in blocklist.css and the class blockedLabel is only used in
> extensions.xml which does not include blocklist.css.
I see now that it is getting the blocklist.css from blocklist.xul which though somewhat confusing is ok. The comment regarding color still stands
do we really want to have an IDL that knows about plugins, app names, toolkit names in xpcom/system? wouldn't /toolkit be more appropriate?
(Assignee)

Comment 38

9 years ago
(In reply to comment #37)
> do we really want to have an IDL that knows about plugins, app names, toolkit
> names in xpcom/system? wouldn't /toolkit be more appropriate?

The plugin host cannot use it if it is in toolkit. As I've stated I'm open to better suggestions for where to put it but it must be in the main build tier.
Comment on attachment 345120 [details] [diff] [review]
patch rev 3

+<!ENTITY blocklist.accept.accesskey  "r">
Shouldn't this be R?

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>@@ -75,16 +75,17 @@ pref("extensions.getAddons.recommended.b
> pref("extensions.getAddons.recommended.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/recommended");
> pref("extensions.getAddons.recommended.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/list/featured/all/10/%OS%/%VERSION%");
> pref("extensions.getAddons.search.browseURL", "https://%LOCALE%.add-ons.mozilla.com/%LOCALE%/%APP%/search?q=%TERMS%");
> pref("extensions.getAddons.search.url", "https://services.addons.mozilla.org/%LOCALE%/%APP%/api/%API_VERSION%/search/%TERMS%/all/10/%OS%/%VERSION%");
> 
> // Blocklist preferences
> pref("extensions.blocklist.enabled", true);
> pref("extensions.blocklist.interval", 86400);
>+pref("extensions.blocklist.severity", 2);
A note describing what this pref does would be a good thing.
Also, severity doesn't describe the purpose very well and elsewhere the term level is used.

>diff --git a/toolkit/mozapps/extensions/content/blocklist.js b/toolkit/mozapps/extensions/content/blocklist.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/content/blocklist.js
>@@ -0,0 +1,81 @@
>...
nit: warnAddon seems a bit generic as noted elsewhere.

>diff --git a/toolkit/mozapps/extensions/content/extensions.js b/toolkit/mozapps/extensions/content/extensions.js
>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -293,16 +293,17 @@ function showView(aView) {
>   // Using disabled to represent add-on state in regards to the EM causes evil
>   // focus behavior when used as an element attribute when the element isn't
>   // really disabled.
>   var bindingList = [ [ ["aboutURL", "?aboutURL"],
>                         ["addonID", "?addonID"],
>                         ["availableUpdateURL", "?availableUpdateURL"],
>                         ["availableUpdateVersion", "?availableUpdateVersion"],
>                         ["blocklisted", "?blocklisted"],
>+                        ["warning", "?warning"],
nit: warning seems a bit generic as noted elsewhere... not going to note any additional parts where warning is used.


>diff --git a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>--- a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>+++ b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl
>...
>@@ -467,23 +467,23 @@ interface nsIExtensionManager : nsISuppo
>   void getDependentItemListForID(in AString id,
>                                  in boolean includeDisabled,
>                                  out unsigned long itemCount,
>                                  [retval, array, size_is(itemCount)] out nsIUpdateItem items);
> 
>   /**
>    * Checks for changes to the blocklist using the local blocklist file,
>    * application disables / enables items that have been added / removed from
>-   * the blocklist, and if there are additions to the blocklist this will
>-   * inform the user by displaying a list of the items added.
>+   * the blocklist.
>    *
>-   * XXXrstrong - this method is not terribly useful and was added so we can
>-   * trigger this check from the additional timer used by blocklisting.
>+   * @returns An array of nsIUpdateItems that are blocklisted or the user should
>+   *          be warned about but are currently enabled.
>    */
>-  void checkForBlocklistChanges();
>+  void getNewlyBlocklistedItems(out unsigned long itemCount,
>+                                [retval, array, size_is(itemCount)] out nsIUpdateItem items);
Would it be worthwhile to change this so it has an includeDisabled param? If so, how about naming it getBlocklistedItems and updating the description accordingly?
If not, I think getNewBlocklistedItems is a better name though I'm fine with it as is.

>diff --git a/toolkit/mozapps/extensions/src/nsBlocklistService.js b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/src/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/src/nsBlocklistService.js
>@@ -48,25 +48,34 @@ Components.utils.import("resource://gre/
>...
>+const URI_BLOCKLIST_DIALOG            = "chrome://mozapps/content/extensions/blocklist.xul"
>+const DEFAULT_SEVERITY                = 3;
>+const DEFAULT_BLOCK_LEVEL             = 2;
>+const MAX_SEVERITY                    = 3;
The mixing of the terms severity and level makes this somewhat confusing

>+const STATE_UNBLOCKED                 = Ci.nsIBlocklistService.STATE_UNBLOCKED;
>+const STATE_WARN                      = Ci.nsIBlocklistService.STATE_WARN;
>+const STATE_BLOCKED                   = Ci.nsIBlocklistService.STATE_BLOCKED;
nit: I'd prefer not defining these and just going with Ci.nsIBlocklistService.STATE_etc where needed for consistency if nothing else.

>+  /**
>+   * Private version of getAddonBlocklistState that allows the caller to pass in
>+   * the blocklist entries.
the blocklist entries to compare against.

>+   *
>+   * @param   id
>+   *          The id of the xpi item to get the blocklist state for.
Are you really getting an "xpi item" here and below?

>+   * @param   version
>+   *          The version of the xpi item to get the blocklist state for.
>+   * @param   addonEntries
>+   *          The blocklist entries to compare against.
The addon blocklist entries to compare against.

>+   * @param   appVersion
>+   *          The application version to compare to, will use the current
>+   *          version if null.
>+   * @param   toolkitVersion
>+   *          The toolkit version to compare to, will use the current version if
>+   *          null.
>+   * @param   The blocklist state for the item, one of the STATE constants
@returns

>+   *          defined in nsIBlocklistService.
nit: as defined in nsIBlocklistService.idl.

>+  /**
>+   * Private version of getPluginBlocklistState that allows the caller to pass in
>+   * the blocklist entries.
>+   *
>+   * @param   plugin
>+   *          The nsIPluginTag to get the blocklist state for.
>+   * @param   pluginEntries
>+   *          The blocklist entries to compare against.
>+   * @param   appVersion
>+   *          The application version to compare to, will use the current
>+   *          version if null.
>+   * @param   toolkitVersion
>+   *          The toolkit version to compare to, will use the current version if
>+   *          null.
>+   * @param   The blocklist state for the item, one of the STATE constants
@returns

>+   *          defined in nsIBlocklistService.
nit: as defined in nsIBlocklistService.idl.

> 
>-  _checkPluginsList: function() {
>...
>+    var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+             getService(Ci.nsIWindowWatcher);
>+    ww.openWindow(null, URI_BLOCKLIST_DIALOG, "",
>+                  "chrome,centerscreen,dialog,modal,titlebar", args);
>+
>+    for (let i = 0; i < addonList.length; i++) {
>+      if (!addonList[i].disable)
>+        continue;
>+
>+      if (addonList[i].item instanceof Ci.nsIUpdateItem)
>+        em.disableItem(addonList[i].item.id);
>+      else if (addonList[i].item instanceof Ci.nsIPluginTag)
>+        addonList[i].item.disabled = true;
Not sure if it is worth it but perhaps add a log statement if either of these two fail?
(In reply to comment #39)
>...
> >+   * @param   version
> >+   *          The version of the xpi item to get the blocklist state for.
> >+   * @param   addonEntries
> >+   *          The blocklist entries to compare against.
> The addon blocklist entries to compare against.
Note: if these are just addonEntries then the new comment should be used. If they are blocklist entries perhaps change the param name to blocklistEntries here and for pluginEntries?
Comment on attachment 345120 [details] [diff] [review]
patch rev 3

Suggestion: perhaps just use the theme's extensions.css instead of adding blocklist.css since you are already loading extensions.css via extensions.xml?

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>@@ -147,16 +147,17 @@ const INSTALLERROR_INVALID_GUID         
> const INSTALLERROR_INVALID_GUID          = -2;
> const INSTALLERROR_INCOMPATIBLE_VERSION  = -3;
> const INSTALLERROR_PHONED_HOME           = -4;
> const INSTALLERROR_INCOMPATIBLE_PLATFORM = -5;
> const INSTALLERROR_BLOCKLISTED           = -6;
> const INSTALLERROR_INSECURE_UPDATE       = -7;
> const INSTALLERROR_INVALID_MANIFEST      = -8;
> const INSTALLERROR_RESTRICTED            = -9;
>+const INSTALLERROR_SOFTBLOCKED           = -10;
You use SOFTBLOCKED here and warning elsewhere. Can this be made consistent with whatever is chosen for warning?

>@@ -764,43 +765,40 @@ function showMessage(titleKey, titlePara
>   else
>     message = extensionStrings.GetStringFromName(messageKey);
>   var ps = Cc["@mozilla.org/embedcomp/prompt-service;1"].
>            getService(Ci.nsIPromptService);
>   ps.alert(null, title, message);
> }
> 
> /**
>- * Shows a dialog for blocklisted items.
>- * @param   items
>- *          An array of nsIUpdateItems.
>- * @param   fromInstall
>- *          Whether this is called from an install or from the blocklist
>- *          background check.
>- */
>-function showBlocklistMessage(items, fromInstall) {
>-  var win = null;
>+ * Shows a dialog for blocklisted items. For warning items returns true if the
Shows a dialog for a blocklisted item during installation.

>+ * item should still be installed
>+ * @param   item
>+ *          The nsIUpdateItem to warn about
The nsIUpdateItem to that is blocklisted.

>+  getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) {
Another thing that concerns me is that typically getETC methods don't perform additional actions such as enable / disable items as this does.
Attachment #345120 - Flags: review?(robert.bugzilla) → review-
(In reply to comment #41)
>...
> >+ * item should still be installed
> >+ * @param   item
> >+ *          The nsIUpdateItem to warn about
> The nsIUpdateItem to that is blocklisted.
The nsIUpdateItem that is blocklisted.
(Assignee)

Comment 43

9 years ago
Had a long discussion with Boriss about the strings and hopefully we've reached a consensus though there are a few changes. Going to answer all the comments here then attach the updated patch shortly.

(In reply to comment #35)
> (From update of attachment 345120 [details] [diff] [review])
> >+.addon-name-version {
> >+  font-size: 110%;
> >+}
> nit: is this really better than going with the default size? I'll leave it up
> to you to decide.

I was going for matching the add-ons manager UI. On OSX we increase the font size. Elsewhere we bold the name. I've updated to match.

> >+.blockedLabel {
> >+  color: red;
> >+  font-style: italic;
> >+}
> Hardcoding the color will cause problems with custom themes (in this case if my
> list background color is red then the text won't be displayed

Yeah this is annoying, gone with bold italics which stands out ok though not great.

> > /* Selected Add-on status messages and images */
> > richlistitem[compatible="true"] .incompatibleBox,
> > richlistitem[providesUpdatesSecurely="true"] .insecureUpdateBox,
> > richlistitem[satisfiesDependencies="true"] .needsDependenciesBox,
> >-richlistitem[blocklisted="false"] .blocklistedBox,
> >+richlistitem:not([blocklisted="true"]):not([warning="true"]) .blocklistedBox,
> >+richlistitem[warning="false"]:not([selected="true"]) .blocklistedBox,
> >+richlistitem[blocklisted="false"] .blocklistedLabel,
> >+richlistitem[warning="false"] .warningLabel,
> What do you think about going with blocklistedhard and blocklistedsoft instead
> of blocklisted and warning? My concern is that warning is too generic and can
> be applied to other things.

Gone with blocklisted and blocklistedsoft as per IRC.

> >-[scriptable, uuid(0c3fe697-d50d-4f42-b747-0c5855cfc60e)]
> >+interface nsIPluginTag;
> >+
> >+[scriptable, uuid(8439f9c0-da03-4260-8b21-dc635eed28fb)]
> > interface nsIBlocklistService : nsISupports
> > {
> >+  // Indicates that the item does not appear in the blocklist.
> >+  const unsigned long STATE_UNBLOCKED = 0;
> Should be NOT_BLOCKED or something similar since UNBLOCKED implies that it was
> unblocked when it may have never been blocked.

Works better yes

(In reply to comment #39)
"?availableUpdateVersion"],
> >                         ["blocklisted", "?blocklisted"],
> >+                        ["warning", "?warning"],
> nit: warning seems a bit generic as noted elsewhere... not going to note any
> additional parts where warning is used.

I've updated everywhere to use the soft term as appropriate.

> >-  void checkForBlocklistChanges();
> >+  void getNewlyBlocklistedItems(out unsigned long itemCount,
> >+                                [retval, array, size_is(itemCount)] out nsIUpdateItem items);
> Would it be worthwhile to change this so it has an includeDisabled param? If
> so, how about naming it getBlocklistedItems and updating the description
> accordingly?
> If not, I think getNewBlocklistedItems is a better name though I'm fine with it
> as is.

I'm not sure it is worth it. Really I'd prefer to get rid of this method, it only exists because the blocklist service can't get what it needs without learning RDF which I don't want to do. I've renamed it and added a note to the comment that people shouldn't be using it

> >+   *
> >+   * @param   id
> >+   *          The id of the xpi item to get the blocklist state for.
> Are you really getting an "xpi item" here and below?

Yeah I was trying to differentiate between EM items and plugins, probably not all that worth it.

> >+      if (addonList[i].item instanceof Ci.nsIUpdateItem)
> >+        em.disableItem(addonList[i].item.id);
> >+      else if (addonList[i].item instanceof Ci.nsIPluginTag)
> >+        addonList[i].item.disabled = true;
> Not sure if it is worth it but perhaps add a log statement if either of these
> two fail?

Hopefully we should never hit it, only if someone were to do something weird with the window I think, but there is no harm in adding it.

(In reply to comment #41)
> (From update of attachment 345120 [details] [diff] [review])
> Suggestion: perhaps just use the theme's extensions.css instead of adding
> blocklist.css since you are already loading extensions.css via extensions.xml?

I'll have to see what that does. I'm a bit wary that we might start making changes to extensions.css not realising they impact the blocklist dialog too. As far as I understand it the extension.css inclusion in extensions.xml would only affect the bindings. I might be wrong there though.

> >+  getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) {
> Another thing that concerns me is that typically getETC methods don't perform
> additional actions such as enable / disable items as this does.

Yeah that isn't ideal. Not many non-get methods return things though. Maybe |updateAndReturnBlocklistedItems| though it's a bit long?
Sounds good.

(In reply to comment #43)
>...
> > >+  getNewlyBlocklistedItems: function EM_getNewlyBlocklistedItems(itemCount) {
> > Another thing that concerns me is that typically getETC methods don't perform
> > additional actions such as enable / disable items as this does.
> 
> Yeah that isn't ideal. Not many non-get methods return things though. Maybe
> |updateAndReturnBlocklistedItems| though it's a bit long?
I'm fine with updateAndGetNewBlocklistedItems or just getNewBlocklistItems with a prominent note in the idl that states specifically what happens to the new blocklisted items when calling this method.
(Assignee)

Comment 45

9 years ago
Created attachment 345401 [details]
selected disabled add-on with warning

Updating screenshots
Attachment #345071 - Attachment is obsolete: true
Attachment #345072 - Attachment is obsolete: true
Attachment #345275 - Attachment is obsolete: true
Attachment #345276 - Attachment is obsolete: true
(Assignee)

Comment 46

9 years ago
Created attachment 345402 [details]
Blocklist updated, new items have been soft blocked
Attachment #344303 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #345402 - Attachment description: User attempts to install an xpi that is soft blocked → Blocklist updated, new items have been soft blocked
(Assignee)

Comment 47

9 years ago
Created attachment 345403 [details]
Blocklist updated, new items have been hard blocked
Attachment #345067 - Attachment is obsolete: true
(Assignee)

Comment 48

9 years ago
Created attachment 345404 [details]
Blocklist updated, some new hard blocks, some new soft blocks
Attachment #345068 - Attachment is obsolete: true
Attachment #345070 - Attachment is obsolete: true
(Assignee)

Comment 49

9 years ago
Created attachment 345405 [details]
User attempts to install an xpi that is soft blocked
(Assignee)

Comment 50

9 years ago
Created attachment 345406 [details]
User attempts to install an xpi that is hard blocked
Attachment #344304 - Attachment is obsolete: true
Comment on attachment 345403 [details]
Blocklist updated, new items have been hard blocked

This says "remove them completely" but you just disable them... true? Whatever the case I'm only going to make comments on strings if they appear incorrect or are awkwardly worded but I won't minus based on the strings. Also, if a new patch is submitted I'll do the review tonight.
(Assignee)

Comment 52

9 years ago
Comment on attachment 345403 [details]
Blocklist updated, new items have been hard blocked

Ignore the ".:" in the text at the top of this, I've corrected it already.
(Assignee)

Comment 53

9 years ago
Created attachment 345415 [details] [diff] [review]
patch rev 4

(In reply to comment #51)
> (From update of attachment 345403 [details])
> This says "remove them completely" but you just disable them... true?

Yeah that isn't very good, replaces "remove" with "disable".
Attachment #345120 - Attachment is obsolete: true
Attachment #345415 - Flags: review?(robert.bugzilla)
Attachment #345415 - Flags: review?(jst)
Attachment #345120 - Flags: review?(jst)
Comment on attachment 345415 [details] [diff] [review]
patch rev 4

It looks good Dave.
Attachment #345415 - Flags: review?(robert.bugzilla) → review+

Updated

9 years ago
Attachment #345415 - Flags: review?(jst) → review+

Updated

9 years ago
Attachment #345415 - Flags: superreview+
(Assignee)

Comment 55

9 years ago
Pushed: http://hg.mozilla.org/mozilla-central/rev/bbccbf2c7456
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Comment 56

9 years ago
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 57

9 years ago
Re-landed: http://hg.mozilla.org/mozilla-central/rev/8291b82dd323

Had to remove my use of Cc and Ci from extensions.js which are only defined on OSX due to the mac browser overlay weirdness.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Some notes from a localizer's perspective.

softBlocklisted.label is the only string with the form "security or stability", all the other strings have "stability or security". Maybe it could make sense to have uniformity across all strings.
Dave, do you have any tests that you can attach here that i can verify by hand?  Thanks
(Assignee)

Comment 60

9 years ago
Created attachment 346066 [details]
test blocklist

(In reply to comment #59)
> Dave, do you have any tests that you can attach here that i can verify by hand?
>  Thanks

Sorry forgot to add this. This is the basic test I was developing with. It includes extensions:

DOM Inspector at severity 3
Storage Explorer at severity 3
Nightly Tester Tools at severity 2
Fire Gestures at severity 1

Also plugins:

Anything with "DivX" in the name at severity 3
Anything with "Silverlight" in the name at severity 3
Anything with "QuickTime" in the name at severity 2
Anything with "Shockwave" in the name at severity 1

By default anything at a severity lower than 2 will only be warned about, equal to or above will be blocked, but this is controlled with the extensions.blocklist.level preference. You can just set extensions.blocklist.url to the url of this attachment and update the blocklist in the normal way. When it is updated, you should be able to adjust extensions.blocklist.level and extensions.blocklist.enabled and it should automatically update the state of items.

One issue I am aware of is that if the add-ons manager is opened when the blocklist updates it often won't display the restart message as necessary, reopening the add-ons manager would solve that. This is an old issue though.
(Assignee)

Comment 61

9 years ago
I should add that if needs be I can put together a blocklist the blocks any particular items you like for testing purposes, just let me know what you need.
(In reply to comment #61)
> I should add that if needs be I can put together a blocklist the blocks any
> particular items you like for testing purposes, just let me know what you need.

Thanks Dave.  question: does blocklisting severity levels affect the versions of the plugins?  or as long as its a quicktime plugin severity 2 (using your example), all versions of quicktime will be affected?
(Assignee)

Comment 63

9 years ago
(In reply to comment #62)
> (In reply to comment #61)
> > I should add that if needs be I can put together a blocklist the blocks any
> > particular items you like for testing purposes, just let me know what you need.
> 
> Thanks Dave.  question: does blocklisting severity levels affect the versions
> of the plugins?  or as long as its a quicktime plugin severity 2 (using your
> example), all versions of quicktime will be affected?

The simple test blocklist I have there will block any of those add-ons regardless of the version. We can block different versions of a single add-on at different severities though if we choose.

Updated

9 years ago
Blocks: 463687
Created attachment 346965 [details]
Incompatible Extension Screenshot

I'm Unsure how to trigger the hard block dialog from comment #50.

I have set the blocklist pointed to the testcase in comment #60, and restarted.   i then installed Fire Gestures (sev level 1) and Dom Inspector (sev level 3) from addons manager.

1) Fire Gestures gave me the soft block installation error as expected.   Check.
2) But Dom Inspector gave me the "incompatible extension" with Fx3.1b1 error.   Is that right?  Shouldn't I be expecting to see the Hard Block Error dialog instead?

(FWIW, if the blocklist is not enabled, Dom Inspector installs just fine without an error)   Screenshot of error attached.
Created attachment 346966 [details]
Correct incompatible error

Sorry, wrong attachment in prior comment.  This is the right screenshot.
Comment on attachment 346965 [details]
Incompatible Extension Screenshot

invalid
Attachment #346965 - Attachment is obsolete: true
(Assignee)

Comment 67

9 years ago
(In reply to comment #64)
> Created an attachment (id=346965) [details]
> Incompatible Extension Screenshot
> 
> I'm Unsure how to trigger the hard block dialog from comment #50.
> 
> I have set the blocklist pointed to the testcase in comment #60, and restarted.
>   i then installed Fire Gestures (sev level 1) and Dom Inspector (sev level 3)
> from addons manager.
> 
> 1) Fire Gestures gave me the soft block installation error as expected.  
> Check.
> 2) But Dom Inspector gave me the "incompatible extension" with Fx3.1b1 error.  
> Is that right?  Shouldn't I be expecting to see the Hard Block Error dialog
> instead?
> 
> (FWIW, if the blocklist is not enabled, Dom Inspector installs just fine
> without an error)   Screenshot of error attached.

Filed this as bug 463819.
Marking this bug verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre
Status: RESOLVED → VERIFIED
copy and paste fail.   I meant Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081110 Minefield/3.1b2pre.
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1

Updated

9 years ago
Keywords: verified1.9.1
Keywords: fixed1.9.1
tests covered in-testsuite.
Flags: in-litmus? → in-litmus-
(Assignee)

Updated

8 years ago
Duplicate of this bug: 502304
Depends on: 522944

Updated

8 years ago
Blocks: 523133
You need to log in before you can comment on or make changes to this bug.