1177130 - When there isn't a valid XPI inside a multipackage XPI we display the message "addon it is not compatible with Firefox"

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We should be saying the add-on is corrupt (in the case of a bad signature) or unsigned (in the case of a missing signature) but for some reason we just call the add-on incompatible. This isn't the end of the world, the add-on isn't being installed but needs investigation.

This affects Firefox 40 but as it's not a big problem we may choose to not fix it there depending on the complexity.

Comment 1

[Dave Townsend [:mossop]]

This isn't specific to signing. If there is no valid XPI inside a multipackage XPI then we end up in a broken internal state. Probably been like this for some time.

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

While this is a valid bug, it seems to have a low end-user impact. I prefer adding a tracking flag for this in FF41 and fixing it in Aurora rather than FF40 which is in Beta phase.

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed downloads.

Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed downloads.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #3)
> Created attachment 8637510 [details]
> MozReview Request: Bug 1177130: Multipackage XPIs with no valid XPIs should
> appear as failed downloads.
> 
> Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed
> downloads.

This isn't a great introduction to the add-ons manager code as it deals with some rarely used cases so here are some notes to help get you up to speed:

* AddonInstall is the class that works to download XPIs and install them. It's basically a state machine going through the process of download, verify, install, sending notifications as it does so.
* After download and verify the AddonInstall can be queried for information about the extension based on the read install.rdf manifest. We use that to display the install UI.
* There is a special type of XPI, a multipackage XPI which contains an install.rdf and then a bunch of other XPIs, in this case the included XPIs get installed rather than the root XPI.
* In this case when AddonInstall detects a multipackage XPI it finds the first valid XPI inside and uses that as its extension to be installed and then creates a linkedInstalls property which is an array of AddonInstalls for the other XPIs.
* In some cases the code wasn't correctly validating the signed state of the first valid XPI. That is fixed by calling AddonInstall.loadManifest which does that check rather than just loading the manifest.
* When no valid add-ons were in the XPI things were breaking because we weren't sending notifications for broken files.

Ask if you have any other questions.

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8637510 [details]
MozReview Request: Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed downloads.

https://reviewboard.mozilla.org/r/13855/#review12573

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5433
(Diff revision 1)
>        catch (e) {
> -        logger.warn(this.file.leafName + " cannot be installed from multi-package " +
> -             "XPI", e);
> +        // _createLinkedInstalls will log errors when it tries to process this
> +        // file

sorry, the comment isn't clear to me - why isn't it useful to log this exception?

Comment 6

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/13855/#review12573

> sorry, the comment isn't clear to me - why isn't it useful to log this exception?

now I get it - the `_createLinkedInstalls` function will log the errors. Could there be any other (unexpected) exceptions that would be useful to log here though?

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8637510 [details]
MozReview Request: Bug 1177130: Multipackage XPIs with no valid XPIs should appear as failed downloads.

Rob gave an r+ on this over IRC.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6f47e7896ce5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f47e7896ce5

Comment 10

[Vasilica Tamas, QA [:vtamas]]

Tried to verify this issue on Firefox 42.0a1 (2015-07-30) under Windows 7 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.10.4 using a signed add-on package from addons-dev.allizom.org:
https://addons-dev.allizom.org/en-US/firefox/addon/test31072015mdl099/

I also set xpinstall.signatures.dev-root to true, but the add-on could not be installed because it appears to be corrupt.

The add-ons from package are successfully installed separately.

Dave, any thoughts about this behaviour?

Comment 11

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #10)
> Tried to verify this issue on Firefox 42.0a1 (2015-07-30) under Windows 7
> 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.10.4 using a signed add-on
> package from addons-dev.allizom.org:
> https://addons-dev.allizom.org/en-US/firefox/addon/test31072015mdl099/
> 
> I also set xpinstall.signatures.dev-root to true, but the add-on could not
> be installed because it appears to be corrupt.
> 
> The add-ons from package are successfully installed separately.
> 
> Dave, any thoughts about this behaviour?

I get different results with that test add-on. For the individual add-ons inside Firefox says they aren't verified. Looking at their signatures that is what I would expect as they don't seem to be signed with the correct ID. Installing the multi-package XPI installs warning that both of the add-ons are unsigned which seems correct too.

Can you try to reproduce with extensions.logging.enabled set to true and show me what appears in the browser console?

Comment 12

[Dave Townsend [:mossop]]

Note that multi-package signing on AMO isn't yet working: bug 1172696

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Robert, given that this patch has been in Nightly for a week, do you want to request an uplift to Aurora? Thanks.

Comment 14

[Dave Townsend [:mossop]]

Given that we're so close to merge and this is a reasonable chunk of code changing I don't think we should rush this through the trains. Multipackage XPIs are relatively rare so the user impact here is low.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

That might mean this will be a won't fix for FF41. Given the low end-user impact, I think that's ok.

Comment 16

[Vasilica Tamas, QA [:vtamas]]

(In reply to Dave Townsend [:mossop] from comment #11)

> Can you try to reproduce with extensions.logging.enabled set to true and
> show me what appears in the browser console?

These logs are thrown in browser console when I try to install https://addons-dev.allizom.org/en-US/firefox/addon/test31072015mdl099/

1439897352309	addons.xpi	DEBUG	Download started for https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary to file C:\Users\VASILI~1.MIH\AppData\Local\Temp\tmp-3vd.xpi
1439897352318	addons.xpi	DEBUG	Download of https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary completed.
1439897352336	addons.xpi	DEBUG	removeTemporaryFile: https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary removing temp file C:\Users\VASILI~1.MIH\AppData\Local\Temp\tmp-3vd.xpi
Use of nsIFile in content process is deprecated. Content.js:23:0
1439897352367	addons.xpi	DEBUG	removeTemporaryFile: https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary does not own temp file
1439897352385	addons.xpi	WARN	Download of https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary failed: Multi-package XPI does not contain any valid packages to install
1439897352390	addons.xpi	DEBUG	downloadFailed: removing temp file for https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary
1439897352390	addons.xpi	DEBUG	removeTemporaryFile: https://addons-dev.allizom.org/firefox/downloads/latest/490860/addon-490860-latest.xpi?src=dp-btn-primary does not own temp file

I have tested again with several multipack add-ons that were created as is specified in Bug 1172696 and all packages were successfully installed on Firefox 43.0a1 (2015-08-17) and Firefox 42.0a2 (2015-08-17) using Windows 8.1 32-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit.

I have noticed that installing the old multipack https://addons-dev.allizom.org/en-US/firefox/addon/test31072015mdl099/ the installation process remains stuck on confirmation install doorhanger and the error doorhanger is not displayed.
- See screenshot: http://i.imgur.com/4KGGcha.jpg
- This issue is reproducible on Firefox 43.0a1 (2015-08-17/18), Firefox 42.0a2 (2015-08-17/18) and Firefox 41 Beta 2 using Windows 8.1 32-bit.
- This is not reproducible on Mac OS X 10.10.4 and Ubuntu 14.04 32-bit.

Should I file a new bug for this issue?

Comment 17

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #16)
> I have noticed that installing the old multipack
> https://addons-dev.allizom.org/en-US/firefox/addon/test31072015mdl099/ the
> installation process remains stuck on confirmation install doorhanger and
> the error doorhanger is not displayed.
> - See screenshot: http://i.imgur.com/4KGGcha.jpg
> - This issue is reproducible on Firefox 43.0a1 (2015-08-17/18), Firefox
> 42.0a2 (2015-08-17/18) and Firefox 41 Beta 2 using Windows 8.1 32-bit.
> - This is not reproducible on Mac OS X 10.10.4 and Ubuntu 14.04 32-bit.
> 
> Should I file a new bug for this issue?

Yes please, that's not what I would expect.

Comment 18

[Vasilica Tamas, QA [:vtamas]]

(In reply to Dave Townsend [:mossop] from comment #17)

> Yes please, that's not what I would expect.

Filed Bug 1196174.

Based on the above mentioned, I am marking this bug as Verified on Firefox 43.0a1 and Firefox 42.0a2, since the other issue is tracked separately.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Untracked for FF41 and setting status to "won't fix". Please see comment 14 and 15 for more info.