Closed Bug 1200027 Opened 4 years ago Closed 4 years ago

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

Categories

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

40 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox40 - wontfix
firefox41 + verified
firefox42 + verified
firefox43 --- verified
firefox-esr38 --- verified

People

(Reporter: philipp, Assigned: mossop)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

Attached image 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
Keywords: regression
(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
Need info'ing Dave as this seems a bit severe and is also impacting FF41.
Flags: needinfo?(dtownsend)
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()
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)
(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)
Attached patch patchSplinter Review
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)
Tracked for 41 and 42.
Comment on attachment 8655589 [details] [diff] [review]
patch

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

r=dveditz
Attachment #8655589 - Flags: review?(dveditz) → review+
https://hg.mozilla.org/mozilla-central/rev/6a8e0442d9ab
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: qe-verify+
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.
Attached image 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)
(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+
This has caused consistent bustage on beta that looks like this - https://treeherder.mozilla.org/logviewer.html#?job_id=507409&repo=mozilla-beta
Mossop, I had to back this out for busting bcX on beta.
Flags: needinfo?(dtownsend)
Attached patch patch for beta, v2 (obsolete) — Splinter Review
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)
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-
Attached patch patchSplinter Review
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
(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+
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
Flags: needinfo?(dtownsend)
(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?
Flags: needinfo?(rkothari)
Flags: needinfo?(dtownsend)
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)
Flags: needinfo?(cbook)
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+
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+
(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)
Attached patch ESR test fixSplinter Review
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.
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.