572966 - InstallTrigger callbacks are being called with some kind of chrome context

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 1

[Dave Townsend [:mossop]]

This is a regression from the new add-ons manager

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Dave Townsend [:mossop]]

(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.

Comment 4

[Dave Townsend [:mossop]]

Looks like bug 550936 fixes this.

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Doug Turner (:dougt)]

poke.

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/91ec3dadef64

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, is there a way to manually verify the fix or do we have to trust the automated test?

Comment 9

[Dave Townsend [:mossop]]

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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.