InstallTrigger callbacks are being called with some kind of chrome context

VERIFIED FIXED in mozilla2.0b5

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: mossop, Unassigned)

Tracking

({regression})

Trunk
mozilla2.0b5
regression
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [AddonsRewrite][sg:moderate])

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Created attachment 452178 [details] [diff] [review]
testcase

Not sure how serious this is at this point, would appreciated some extra eyes on this.

The InstallTrigger object available to webpages allows content to pass a callback function that will be called when the XPI add-on they request has been installed. Currently InstallTrigger is a C++ object implementing amIInstallTrigger so the callback passed by content gets wrapped into a amIInstallCallback.

It seems that in certain situations when chrome JS calls this callback something about the chrome scope leaks into the callback function. The specific case that I have discovered is that if the callback attempts to set "document.location" to a relative URL the platform interprets it as relative to a chrome URL instead of the document URL. The attached testcase demonstrates this case. The attempt to redirect fails and the following shows up in the error console:

Error: [Exception... "'[JavaScript Error: "Access to 'chrome://browser/content/browser.xul#foo' from script denied" {file: "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/triggerredirect.html" line: 12}]' when calling method: [amIInstallCallback::onInstallEnded]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///Users/dave/mozilla/build/minefield/dist/Minefield.app/Contents/MacOS/components/addonManager.js :: callCallback :: line 100"  data: yes]
Source File: file:///Users/dave/mozilla/build/minefield/dist/Minefield.app/Contents/MacOS/components/addonManager.js
Line: 103

I suspect that chrome://browser/content/browser.xul is coming from the fact that code in browser.xul's scope is the first JS part of the stack in the call to that callback. What I don't know is how serious this is or how to solve it.

The problem is reproducable in a plain trunk build of Firefox by going to this page: https://addons.mozilla.org/en-US/firefox/addon/122/, clicking on "Add to Firefox" and then clicking Cancel in the install prompt.
(Reporter)

Comment 1

8 years ago
This is a regression from the new add-ons manager
Keywords: regression
Whiteboard: [AddonsRewrite]
blocking2.0: --- → ?
Do we want to take the opportunity in Firefox 4 to kill the callback function? If the functionality is really really wanted we could fire events at the page and let people write listeners for it. That seems more "webby" to me.

Updated

8 years ago
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected

Updated

8 years ago
Whiteboard: [AddonsRewrite] → [AddonsRewrite][sg:moderate]
(Reporter)

Comment 3

8 years ago
(In reply to comment #2)
> Do we want to take the opportunity in Firefox 4 to kill the callback function?
> If the functionality is really really wanted we could fire events at the page
> and let people write listeners for it. That seems more "webby" to me.

If someone contributed a patch to do that then I think we would take it, otherwise I don't think we have the time left to do it. I'd rather roll that out very early in the cycle of a future release to give people as much time to update as possible.
(Reporter)

Comment 4

8 years ago
Looks like bug 550936 fixes this.
Depends on: 550936
(Reporter)

Comment 5

8 years ago
Comment on attachment 452178 [details] [diff] [review]
testcase

Once bug 550936 is fixed I'd like to land this testcase to make sure we don't regress. The code change also adds nicer error console reporting when the callback function in the webpage throws an exception.
Attachment #452178 - Flags: review?(robert.bugzilla)
(Reporter)

Updated

8 years ago
blocking2.0: ? → betaN+
Attachment #452178 - Flags: review?(robert.bugzilla) → review+

Comment 6

8 years ago
poke.
(Reporter)

Comment 7

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/91ec3dadef64
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
(Reporter)

Updated

8 years ago
Flags: in-testsuite+
Flags: in-litmus-
Dave, is there a way to manually verify the fix or do we have to trust the automated test?
(Reporter)

Comment 9

8 years ago
Testing the fix would require a page with a callback that attempts to redirect to a relate url. The one in the test case could be adjusted for a manual test, I'm not sure there is a lot to be gained from that though.
Ok, so our automated test should be enough then. Since QA cannot test it in a manual way I will mark it verified fixed based on the passing automated test.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.