If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 41

Status

()

Toolkit
Add-ons Manager
--
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: mossop)

Tracking

({regression})

40 Branch
mozilla43
regression
Points:
---

Firefox Tracking Flags

(firefox40- wontfix, firefox41+ verified, firefox42+ verified, firefox43 verified, firefox-esr38 verified)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8654617 [details]
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
(Reporter)

Updated

2 years ago
Keywords: regression

Comment 1

2 years ago
(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

2 years ago
Need info'ing Dave as this seems a bit severe and is also impacting FF41.
Flags: needinfo?(dtownsend)
(Reporter)

Comment 3

2 years ago
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...
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

2 years ago
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. :)
Flags: needinfo?(jorge)
(Assignee)

Comment 6

2 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Assignee: nobody → dtownsend
Flags: needinfo?(jorge)
Flags: needinfo?(dtownsend)
(Assignee)

Updated

2 years ago
Blocks: 1042699
(Assignee)

Comment 7

2 years ago
Created attachment 8655589 [details] [diff] [review]
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.
Attachment #8655589 - Flags: review?(dveditz)

Comment 8

2 years ago
Tracked for 41 and 42.
status-firefox42: --- → affected
tracking-firefox41: ? → +
tracking-firefox42: --- → +
Comment on attachment 8655589 [details] [diff] [review]
patch

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

r=dveditz
Attachment #8655589 - Flags: review?(dveditz) → review+

Comment 10

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/6a8e0442d9ab
https://hg.mozilla.org/mozilla-central/rev/6a8e0442d9ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: qe-verify+
(Assignee)

Comment 12

2 years ago
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
Attachment #8655589 - Flags: approval-mozilla-beta?
Attachment #8655589 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(florin.mezei)
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.
Created attachment 8658711 [details]
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.
Flags: needinfo?(florin.mezei)
(Assignee)

Comment 16

2 years ago
(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 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.
Attachment #8655589 - Flags: approval-mozilla-beta?
Attachment #8655589 - Flags: approval-mozilla-beta+
Attachment #8655589 - Flags: approval-mozilla-aurora?
Attachment #8655589 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/22c0d857e4c4
status-firefox42: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/da0ee59da1ba
status-firefox41: affected → fixed
This has caused consistent bustage on beta that looks like this - https://treeherder.mozilla.org/logviewer.html#?job_id=507409&repo=mozilla-beta
Backed out from beta: https://hg.mozilla.org/releases/mozilla-beta/rev/a3e17097503e
status-firefox41: fixed → affected
Mossop, I had to back this out for busting bcX on beta.
Flags: needinfo?(dtownsend)
Created attachment 8659144 [details] [diff] [review]
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.)
Attachment #8659144 - Flags: review?(dtownsend)
(Assignee)

Comment 24

2 years ago
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.
Flags: needinfo?(dtownsend)
Attachment #8659144 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 25

2 years ago
Created attachment 8659299 [details] [diff] [review]
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.
Attachment #8659144 - Attachment is obsolete: true
Attachment #8659299 - Flags: approval-mozilla-beta?
(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
(Assignee)

Comment 27

2 years ago
(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 on attachment 8659299 [details] [diff] [review]
patch

This was approved earlier and re-nom'd due to bustage. Beta41+
Attachment #8659299 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/5fd58765314c
status-firefox41: affected → fixed
QA Contact: vasilica.mihasca
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?
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
status-firefox42: fixed → verified
status-firefox43: fixed → verified
Flags: needinfo?(dtownsend)
(Assignee)

Comment 31

2 years ago
(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
Flags: needinfo?(dtownsend)
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?
status-firefox-esr38: --- → affected
Flags: needinfo?(rkothari)
Flags: needinfo?(dtownsend)
(Assignee)

Comment 33

2 years ago
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
Flags: needinfo?(dtownsend)
Attachment #8655589 - Flags: approval-mozilla-esr38?
Redirecting to Liz. Liz, we might need to take another fix in 38.3.0.
Flags: needinfo?(rkothari) → needinfo?(lhenry)
Comment on attachment 8655589 [details] [diff] [review]
patch

Approved for uplift to esr38 - we will have to do a build2.
Flags: needinfo?(lhenry)
Attachment #8655589 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Kwierso or tomcat, can you do the uplift so I can start another ESR build tonight? Thanks.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
https://hg.mozilla.org/releases/mozilla-esr38/rev/44df22d67fa5
status-firefox-esr38: affected → fixed
Flags: needinfo?(wkocher)

Updated

2 years ago
Flags: needinfo?(cbook)
(Assignee)

Comment 38

2 years ago
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.
Attachment #8655589 - Flags: approval-mozilla-esr38+
(Assignee)

Comment 39

2 years ago
Comment on attachment 8659299 [details] [diff] [review]
patch

This one however should be good
Attachment #8659299 - Flags: approval-mozilla-esr38?
Speaking on IRC, I believe I uplifted the proper patch (from comment 29) to ESR38, so we should be good.
Flags: needinfo?(dtownsend)
Comment on attachment 8659299 [details] [diff] [review]
patch

twiddling the flag on the correct patch, which kwierso already uplifted anyway
Attachment #8659299 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
(Assignee)

Comment 42

2 years ago
(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
Flags: needinfo?(dtownsend)
(Assignee)

Comment 43

2 years ago
Created attachment 8661954 [details] [diff] [review]
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.
https://hg.mozilla.org/releases/mozilla-esr38/rev/94b9605b5b2c
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.
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.
status-firefox-esr38: fixed → verified
Flags: qe-verify+
status-firefox40: affected → wontfix
tracking-firefox40: ? → -
You need to log in before you can comment on or make changes to this bug.