463819 - Blocklisted add-ons with a compatibility update say they are incompatible rather than blocklisted

Status: VERIFIED FIXED

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

Description

[Dave Townsend [:mossop]]

If you attempt to install a blocklisted add-on that is also incompatible with the current version of the application we performs a compatibility update check. Any compatibility update is ignored because the add-on is blocklisted, so we then tell the user the add-on is incompatible. This is incorrect, it would be compatible if we ignored the blocklist.

We should just fail with the blocklisted error ignoring any compatibility info.

Comment 1

[Dave Townsend [:mossop]]

This is a little less straightforward than at first glance. In general if an add-on is incompatible we should say that. If it is compatible and blocklisted then we should say that. The problem is that doing that will involve doing a remote update check for add-ons that appear incompatible even when we know that eventually we are going to fail to install anyway.

We pretty much either have to give the blocklist error a higher priority than incompatibility errors, or have to do a remote update check for blocklisted add-ons. Anything else gets inconsistent I think.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

We also want to support the scenario where the compatibility update moves the add-on to a version which is no longer blocklisted, don't we?

Feels like what we should be doing when we do an add-on install is:

 - check for compatibility updates to be applied
 - if they exist ..
 --- determine what the add on version would be with that applied
 --- check that against the blocklist
 --- if the add on is still blocklisted ..
 ------- show the "Add on is blocklisted" error
 --- otherwise...
 ------- install the add-on & its compatibility update
 - otherwise...
 --- check against the blocklist
 --- (as per normal)

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This patch allows the compatibility check to proceed for blocklisted items. It alters _isValidUpdate to just ignore the blocklist when it is a compatibility check. A change is also necessary to allow the IncompatibleObserver to see the final status of the installation to determine whether it must still apply the compatibility update to the datasource.

The test includes nine add-ons. three groups of three. The first group are not in the blocklist, second are soft blocked and third hard blocked. In each group there is an add-on that is compatible according to it's install.rdf, an add-on that requires a compatibility update and an add-on that is incompatible even after the compatibility update. The test just attempts to install all 9 in sequence to see whether the correct status is thrown for each.

Comment 4

[Dave Townsend [:mossop]]

Just to clarify the behaviour expected from this patch:

If an add-on is incompatible with the application (even after a remote compatibility update), then the incompatible error is displayed.
The blocklist error is only displayed if the add-on is compatible with the application.

Comment 5

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

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

It looks like bug 286034 introduced INSTALLERROR_PHONED_HOME but never used it. Perhaps just replace it with INSTALLERROR_PHONING_HOME?

Comment 6

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

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

Looks good with comment #5 fixed.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Updated patch with the last fix

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/070e364189c8

It looks like tests were included (yay!) so setting in-testsuite+.  Please correct as needed.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Could this have caused ts regression on vista?

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Could this have caused ts regression on vista?

I don't believe so, none of the changes are in the startup path.

Comment 11

[Tony Chung [:tchung]]

hey dave, anyway i can test this?  do you have a test xpi that i can use to simulate this behavior?   Thanks

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> hey dave, anyway i can test this?  do you have a test xpi that i can use to
> simulate this behavior?   Thanks

For a manual testcase I would need to write a custom blocklist and testers would have to modify the preferences for that and get it to update then try to install a custom add-on. This is in the automated testsuite so I think we are probably covered, but if you think it is worthwhile me providing that then I can do it.

Comment 13

[Tony Chung [:tchung]]

per comment 3, we're covered well with tests.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre