1200027 - 40.0.3 broke ability to install featured addons from the add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[[:philipp]]

Attachment: screenshot.png

a user on irc reported that it is no longer possible to install extensions featured directly in the addons manager since the latest chemspill update.

this is reproducible:
- open about:addons 
- click on one of the featured addons
- click on "add to firefox"
- you receive an error: services.addons.mozilla.org - Firefox prevented this site from asking you to install software on your computer

probably regressed by bug 1042699 which isn't publicly accessible

Comment 1

[Alice Wyman]

(In reply to philipp from comment #0)

> probably regressed by bug 1042699 which isn't publicly accessible
I can't access bug 1042699 but here's the security advisory that links to it:

https://www.mozilla.org/en-US/security/advisories/mfsa2015-95/ 
Mozilla Foundation Security Advisory 2015-95 Add-on notification bypass through data URLs

Comment 2

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

Need info'ing Dave as this seems a bit severe and is also impacting FF41.

Comment 3

[[:philipp]]

it's also no longer possible to install an addon by putting the path to its xpi (like https://addons.mozilla.org/firefox/downloads/latest/1865/addon-1865-latest.xpi ) into the location bar, but that's probably a minor issue in comparison...

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

That's also affecting add-on urls directly pasted into the location bar, e.g. https://download.mailvelope.com/releases/latest/mailvelope.firefox.xpi

Reason: Bug 1042699 switched to principal usage in XPIProvider.jsm's isInstallAllowed. Before aURI was null, now its replacement "uri" is a nullprincipal which doesn't pass the test |if (!uri)| which is required for running this.isDirectRequestWhitelisted()

Comment 5

[Robert Kaiser]

Jorge, is there something that AMO can do to make the featured add-ons work, or is this completely on the client side? In any case, you should know about this. :)

Comment 6

[Dave Townsend [:mossop]]

(In reply to Robert Kaiser (:[e-mail]) from comment #5)
> Jorge, is there something that AMO can do to make the featured add-ons work,
> or is this completely on the client side? In any case, you should know about
> this. :)

This is completely client side, we're running afoul of the inner-frame running in a different origin to the main page, something we intended to block when websites do it but not necessarily when we do it to ourselves.

(In reply to Aryx [:archaeopteryx][:aryx] from comment #4)
> That's also affecting add-on urls directly pasted into the location bar,
> e.g. https://download.mailvelope.com/releases/latest/mailvelope.firefox.xpi

This was an expected result of the change.

Comment 7

[Dave Townsend [:mossop]]

Attachment: patch

The messages to chrome to start add-on installs all go through the content frame message manager and we use that to determine which browser triggered the install and so which browser to use for security checks and which tab to notify the UI about.

The problem is that the AMO discovery pane runs in a <browser> inside the add-ons manager which is itself running inside a <browser>. In this case we should be doing the security checks against the inner browser and notifying UI about the outer browser so the doorhangers are parented correctly. The code the install paths use to send the messages all find the outer browser because the inner browser doesn't have a message manager. Bug 1200742 means we can't change that right now.

Instead this patch makes us skip the message manager in the case that we're already running in the main process. It walks up the frame tree from the installing window to find the privileged browser element that contains the website and then passes that directly to the add-ons manager. The add-ons manager can then walk further up to get the tab that that browser is contained in if necessary. This won't work for in-content UI that loads remotely but there is no danger of the add-ons manager loading remotely for now.

Comment 8

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

Tracked for 41 and 42.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8655589 [details] [diff] [review]
patch

Review of attachment 8655589 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8655589 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1042699
[User impact if declined]: Installing add-ons through the AMO discovery page in the add-ons manager doesn't work
[Describe test coverage new/current, TreeHerder]: On mozilla-central with automated tests and verified by QA
[Risks and why]: Fairly safe with good tests.
[String/UUID change made/needed]: None

Comment 13

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

Florin, would it be possible to verify this fix on Nightly so I can approve uplift to Beta41? Given that the patch is pretty big, I will feel more comfortable if it is verified by QE. Has it already been verified? Please let me know.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox Nightly on 43.0a1 20150908
Installing a featured and a recommend add-on, one restartless, the other one requiring a restart, both pass the installation as expected and also work after install. Also an add-on can be installed from file and works.

Comment 15

[Vasilica Tamas, QA [:vtamas]]

Attachment: gif21.gif

I was able to reproduce this issue on Firefox 41 Beta 8 under Windows 7 64-bit.

Tested on Firefox 43.0a1 (2015-09-08/09) and the add-ons are successfully installed when the straight flow is followed: left click on “Add to Firefox” green button.

But this issue is still reproducible for right click on “Add to Firefox” green button. When “Open link in New Tab”, “Open link in New Window” and “Open Link in New Private Window” options are selected, the following error message is displayed: “Firefox prevented this site from asking you to install software on your computer”.

I am attaching a video recorded on Windows 7 64-bit.

Tested under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.5.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #15)
> But this issue is still reproducible for right click on “Add to Firefox”
> green button. When “Open link in New Tab”, “Open link in New Window” and
> “Open Link in New Private Window” options are selected, the following error
> message is displayed: “Firefox prevented this site from asking you to
> install software on your computer”.

That's an expected side-effect of bug 1042699

Comment 17

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

Comment on attachment 8655589 [details] [diff] [review]
patch

Given that this patch was verified by two independent sources, it seems safe to uplift to Beta41 and Aurora42.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22c0d857e4c4

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/da0ee59da1ba

Comment 20

[Nigel Babu [:nigelb]]

This has caused consistent bustage on beta that looks like this - https://treeherder.mozilla.org/logviewer.html#?job_id=507409&repo=mozilla-beta

Comment 21

[Nigel Babu [:nigelb]]

Backed out from beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a3e17097503e

Comment 22

[Nigel Babu [:nigelb]]

Mossop, I had to back this out for busting bcX on beta.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch for beta, v2

Modified the backed out for beta by changing the first argument in Services.perms.remove to a string (got changed in Fx42+ by bug 1170200). This fixes the failing browser_discovery_install.js. (There are tests failing locally in browser_details.js with and without the patch.)

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8659144 [details] [diff] [review]
patch for beta, v2

Sorry this isn't correct, nsIPermissionManager used to accept the hostname not the url for the remove method.

Comment 25

[Dave Townsend [:mossop]]

Attachment: patch

This is the correct beta patch, it passed a try run overnight. Given that it is just a bustage fix I don't think we need review here.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Dave Townsend [:mossop] from comment #24)
> Comment on attachment 8659144 [details] [diff] [review]
> patch for beta, v2
> 
> Sorry this isn't correct, nsIPermissionManager used to accept the hostname
> not the url for the remove method.
It determines the host from the provided url. That's e.g. used at http://mxr.mozilla.org/mozilla-beta/source/browser/base/content/test/general/browser_remoteTroubleshoot.js#47

Comment 27

[Dave Townsend [:mossop]]

(In reply to Aryx [:archaeopteryx][:aryx] from comment #26)
> (In reply to Dave Townsend [:mossop] from comment #24)
> > Comment on attachment 8659144 [details] [diff] [review]
> > patch for beta, v2
> > 
> > Sorry this isn't correct, nsIPermissionManager used to accept the hostname
> > not the url for the remove method.
> It determines the host from the provided url. That's e.g. used at
> http://mxr.mozilla.org/mozilla-beta/source/browser/base/content/test/general/
> browser_remoteTroubleshoot.js#47

Huh, looks like the code can indeed handle full urls despite what the interface says: http://mxr.mozilla.org/mozilla-beta/source/netwerk/base/nsIPermissionManager.idl#108

Comment 28

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

Comment on attachment 8659299 [details] [diff] [review]
patch

This was approved earlier and re-nom'd due to bustage. Beta41+

Comment 29

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5fd58765314c

Comment 30

[Vasilica Tamas, QA [:vtamas]]

Verified fixed on Firefox 43.0a1 (2015-09-13), Firefox 42.0a2 (2015-09-13) and Firefox 41 Beta 9 under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.
 
Dave, should I file a new bug for the mentioned issue from Comment 15 or this is already tracked?

Comment 31

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #30)
> Verified fixed on Firefox 43.0a1 (2015-09-13), Firefox 42.0a2 (2015-09-13)
> and Firefox 41 Beta 9 under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS
> X 10.9.5.
>  
> Dave, should I file a new bug for the mentioned issue from Comment 15 or
> this is already tracked?

It's basically the same issue as bug 1201534

Comment 32

[Vasilica Tamas, QA [:vtamas]]

Encountered this issue on Firefox 38.3.0 ESR (20150914164146). It is reproducible across all platforms: Windows 7 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.10.4.
See screenshot: http://i.imgur.com/huFXjJZ.jpg

How we should proceed in this situation?

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 8655589 [details] [diff] [review]
patch

Right, we landed the broken bug on the ESR branch too so we'd need to land hte fix there too

Comment 34

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

Redirecting to Liz. Liz, we might need to take another fix in 38.3.0.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8655589 [details] [diff] [review]
patch

Approved for uplift to esr38 - we will have to do a build2.

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Kwierso or tomcat, can you do the uplift so I can start another ESR build tonight? Thanks.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/44df22d67fa5

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 8655589 [details] [diff] [review]
patch

Forgot that this patch doesn't work on beta so it's sure not to work on esr.

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 8659299 [details] [diff] [review]
patch

This one however should be good

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Speaking on IRC, I believe I uplifted the proper patch (from comment 29) to ESR38, so we should be good.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8659299 [details] [diff] [review]
patch

twiddling the flag on the correct patch, which kwierso already uplifted anyway

Comment 42

[Dave Townsend [:mossop]]

(In reply to Wes Kocher (:KWierso) from comment #40)
> Speaking on IRC, I believe I uplifted the proper patch (from comment 29) to
> ESR38, so we should be good.

Yes that's right

Comment 43

[Dave Townsend [:mossop]]

Attachment: ESR test fix

Except that ESR38 has a different server-locations file so tests are failing there. This changes the test to use origins that exist on ESR38 so they will pass again.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/94b9605b5b2c

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

I think since this is a test-only change, we don't need to rebuild esr 38.3.0. This test fix will end up shipping with 38.4.0 eventually.

Comment 46

[Vasilica Tamas, QA [:vtamas]]

This issue was manually verified as fixed on Firefox 38.0.3 build 2 (20150916094008) during our ESR Smoke Testing under Windows 8.1 32-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5. All add-ons are now successfully installed from Add-ons Manager.