So, I was playing with bootstrapped extensions, and I've hit what is called problem 6.5 in this blog post: http://adblockplus.org/blog/how-many-hacks-does-it-take-to-make-your-extension-install-without-a-restart To workaround the "chrome" protocol problem (not available to register my resources) I've registered my own system principal protocol, the problem is that using this protocol in optionsURL or aboutURL doesn't open a dialog, rather opens a browser window with xul inside it. The problem is that about:addons is a specially privileged page, when extensions.js invokes openDialog it does with special privileges, and it ends up using the uriToLoadIsChrome check in http://hg.mozilla.org/mozilla-central/annotate/0680c776e806/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l527 (most likely ending up here http://hg.mozilla.org/mozilla-central/annotate/0680c776e806/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l1594) A really simple workaround to this is to change in extensions.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#894) from openDialog to: var browserWin = Services.wm.getMostRecentWindow("navigator:browser"); browserWin.openDialog(optionsURL, "", features); This way it's the browser window that opens the dialog, and all security context is in place. But I suppose fixing properly openDialog could be better, unfortunately touching this security code looks scary.

Discussed on IRC: Can't rely on having a navigator:browser window open in toolkit applications, but we can walk up the docshell tree to get the parent window. This should work in cases where the addons manager is in-content, and in its own window (in which case, its already the root parent window). The alternative is to make windowwatcher do what we want, but that's a long rabbithole with its own set of subtle side-effects.

Not taking the bug for now since I dunno if this solution is fine for you, I've noticed that we use it in other situations (like in mobile about:home). I don't know enough about security implications of touching the windowwatcher to investigate the other approach.

Can we just use nsIWindowWatcher.openWindow or does that have the same problem?

hm, well openWindow wants to know the parent, then it would need chromeWindow again since it would end up again in CalculateChromeFlags. I suspect anything passing through windowwatcher would show the same bug since all calls are (correctly) going to the same code path.

Passing dialog parameters through nsIWindowWatcher.openWindow is really ugly, navigating the docshell hierarchy is simple in comparison. And it needs the correct parent window anyway, otherwise you won't be able to open a proper modal dialog.

Ah I had forgotten about the new arguments for the about dialog. Why won't the dialog be properly modal if it is parented to the content window? If we parent it to the chrome window then it will display in a different place on OSX, not a big deal I guess.

(In reply to comment #6) > Why won't the dialog be properly modal if it is parented to the content window? I think (but correct me if I'm wrong) he meant that parenting to the content window would hit this bug again?

Comment on attachment 527241 [details] [diff] [review] addon manager workaround v1.0 Not terribly keen on it but there doesn't seem to be a better way. Can we get a test?

hm, I suppose I can make a browser-chrome test, it could take some time though.

The hardcoded chrome protocol check that's causing this is this one: http://hg.mozilla.org/mozilla-central/annotate/357593f3abbd/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l527. CalculateChromeFlags() later removes "chrome" and "modal" flags if the target uses a non-chrome protocol and the parent isn't nsIChromeWindow. And using nsIWindowWatcher.openWindow() won't help because it takes the same code path. Maybe this can be fixed relatively easily after all. Changing the hardcoded check for the "chrome" scheme is hard - this is a security check and URI_IS_LOCAL_RESOURCE isn't restrictive enough. Unfortunately, chrome privileges are associated with the channel, not with the URI, so detecting them at this stage is impossible. But a better logic to calculate aHasChromeParent parameter should be possible, e.g. checking whether the parent uses the system principal rather than whether it implements nsIChromeWindow. I would suggest using nsContentUtils::IsCallerTrustedForWrite() like GlobalWindow::OpenDialog but that won't work for C++ callers.

nsContentUtils::IsChromeDoc() is apparently the function to be used here. Something along the lines of: - nsCOMPtr<nsIDOMChromeWindow> chromeParent(do_QueryInterface(aParent)); + PRBool hasChromeParent = PR_TRUE; + if (aParent) { + nsCOMPtr<nsIDOMDocument> domdoc; + aParent->GetDocument(getter_AddRefs(domdoc)); + nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument)); + hasChromeParent = doc && nsContentUtils::IsChromeDoc(parentDoc); + } // Make sure we call CalculateChromeFlags() *before* we push the // callee context onto the context stack so that // CalculateChromeFlags() sees the actual caller when doing its // security checks. chromeFlags = CalculateChromeFlags(features.get(), featuresSpecified, aDialog, uriToLoadIsChrome, - !aParent || chromeParent); + hasChromeParent);

> + nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument)); > + hasChromeParent = doc && nsContentUtils::IsChromeDoc(parentDoc); Should have been "domdoc" rather than "mDocument" and "doc" rather than "parentDoc" of course. I obviously cannot write code without having the compiler check my variables :)

cool, will try this solution as well, in both cases there is probably the need for a test, luckily the test should be independent from the fix. If we fix ww, most likely we'll want an additional review from jst.

This is the test only, clearly now it fails. I'll make a patch based on Wladimir findings, then we can choose the approach, the test will stay valid for both.

This is the patch suggested from Wladimir, it works fine, I've set him as patch author since he practically wrote it :) I think this approach is globally better since fixes a wrong check at the source. Setting the review request to jst that reviewed other patches in the ww (alternatively sicking could review this I think).

Comment on attachment 527895 [details] [diff] [review] test v1.0 Review of attachment 527895 [details] [diff] [review]: ::: toolkit/mozapps/extensions/test/browser/browser_openDialog.js @@ +13,5 @@ + Ci.nsIProtocolHandler.URI_NORELATIVE | + Ci.nsIProtocolHandler.URI_NOAUTH, + + newURI: function CCP_newURI(aSpec, aOriginCharset, aBaseUri) + { Can you put opening braces on the same line as the function declaration throughout please. @@ +77,5 @@ +} +XPCOMUtils.defineLazyGetter(CustomChromeProtocol.factory, "registrar", function() +{ + return Components.manager.QueryInterface(Ci.nsIComponentRegistrar); +}); Doesn't seem much point in this being a lazy getter, it will always get used. @@ +140,5 @@ + } + break; + case "domwindowopened": + let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget); + win.addEventListener("load", function () As I said in the other bug, we've hit problems where we must wait for focus on the new window, I think we should do it here too.

Comment on attachment 527902 [details] [diff] [review] windowwatcher patch v1.0 Review of attachment 527902 [details] [diff] [review]: Looks good, r=jst, but I'd like bz to glance over this as well.

Comment on attachment 527902 [details] [diff] [review] windowwatcher patch v1.0 r=me, I guess. I _really_ wish we'd just fix the pile of broken that is loading about:addons in a content docshell. If we did that, this issue would never have arisen. :( Do we have anyone working on that, or are we just hoping it will happen somehow?

Note, in particular, that I can't offer any guarantee that this change is not adding a new security vulnerability...

(In reply to comment #18) > Comment on attachment 527902 [details] [diff] [review] > windowwatcher patch v1.0 > > r=me, I guess. I _really_ wish we'd just fix the pile of broken that is > loading about:addons in a content docshell. If we did that, this issue would > never have arisen. :( > > Do we have anyone working on that, or are we just hoping it will happen > somehow? I've filed bug 652736 but I don't know of anyone working on it, nor do I really know how to go about it, can docshell types be changed dynamically or anything?

uups, misformatted the addons array in the previous version.

Using Places as a staging area, plan to merge to central soon (today or tomorrow) if everything goes well. http://hg.mozilla.org/projects/places/rev/e48ba6fbc57c http://hg.mozilla.org/projects/places/rev/ad9ca556b46c

pushed a follow-up for the test. It was failing on Mac and Linux because on these platforms there is instantApply enabled, thus the dialog is not modal. http://hg.mozilla.org/projects/places/rev/bb1886a87fea

http://hg.mozilla.org/mozilla-central/rev/e48ba6fbc57c http://hg.mozilla.org/mozilla-central/rev/ad9ca556b46c http://hg.mozilla.org/mozilla-central/rev/bb1886a87fea

Marco, can you verify this fix with your example jetpack or let me know where I can find it to verify it myself?

https://addons.mozilla.org/firefox/addon/places-maintenance/ You can try the difference between FF5.0 and FF6.0

Thanks. Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110512 Firefox/6.0a1. Marco, what's the coverage of the automated test? Do we also need a manual one?

The test opens about:addons, clicks on the options button, and checks the chrome flags on the opened dialog. I feel like it's enough.

oh and that is done for both a classic chrome: and a fake-chrome: schemes.

Ok, declining a Litmus then. Thanks.