1505850 - GetInheritedPrincipal shouldn't bother creating a content viewer if we don't have one

Status: RESOLVED WONTFIX

(Core :: DOM: Navigation, enhancement, P2)

Description

[:Gijs (he/him)]

Per discussion on IRC today, https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/docshell/base/nsDocShell.cpp#10182-10194 should probably just return nullptr even if we do pass aConsiderCurrentDocument. This will avoid creating an about:blank viewer e.g. in cases where we explicitly load a data: URI in a toplevel browsing context.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1505850 - GetInheritedPrincipal shouldn't bother creating an about:blank content viewer if one doesn't exist, r?nika

Comment 2

[:Gijs (he/him)]

:bz: thanks for the r+! I can't really clear the review and re-request it in phab, so using needinfo instead - it seems try is unhappy:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c6599f9536caaa3cfce1dcab7436602e488202d&selectedJob=210626906

TEST-UNEXPECTED-FAIL | docshell/test/chrome/test_principalInherit.xul | Test timed out.

https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/docshell/test/chrome/test_principalInherit.xul#58-80

looks like there's no load event for the JS uri loaded in the content frame anymore, or perhaps it's no longer visible to the test or something? Obviously I could just remove the test, but I'm worried that this indicates actual breakage...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, it indicated breakage in the specific scenario of this test: chrome code doing a javascript: load in a content docshell that doesn't already have a document in it.  This case does in fact break: we don't find a principal to inherit, give the javascript: a nullprincipal, then later we force creation of the about:blank, which gets a _different_ nullprincipal and the script doesn't get to run.

If we need to support this scenario, then we need  to keep creating the about:blank before we open the javascript: channel.   But I was assuming that we don't need to keep supporting it.

Comment 4

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)
> Well, it indicated breakage in the specific scenario of this test: chrome
> code doing a javascript: load in a content docshell that doesn't already
> have a document in it.  This case does in fact break: we don't find a
> principal to inherit, give the javascript: a nullprincipal, then later we
> force creation of the about:blank, which gets a _different_ nullprincipal
> and the script doesn't get to run.
> 
> If we need to support this scenario, then we need  to keep creating the
> about:blank before we open the javascript: channel.   But I was assuming
> that we don't need to keep supporting it.

Hm. So presumably loading the data URI would still work, just not the JS one?

The JS one we could solve on the chrome side by passing null principals as the triggering principal for cases like this (e.g. when opening bookmarklets in new tabs) -- the problem is that that doesn't work well because null principals can't cross process boundaries. We ran into this in bug 1484741, which I think still uses this approach.

This is a bit worrying, because I expect there's a bunch of places (bug 1484741 is fixed, but not e.g. bug 1502072, which made it through an entire nightly cycle before being found...) that would need fixes on the chrome JS side.

The alternative fix for some of the concerns here is to not send an onLocationChange notification for this type of initial about:blank load. I'll see if that is workable instead.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Hm. So presumably loading the data URI would still work, just not the JS one?

Yes.

> we could solve on the chrome side by passing null principals as the triggering principal

I don't think that would help.  The issue with the javascript: bit is that it will only run if the principal being used for the load matches the principal of the page it runs against.  If the page to run against is going to come from CreateAboutBlankContentViewer() and synthesize about:blank with a nullprincipal or similar, then having a different nullprincipal on the load does not work.

The bug 1502072 situation is an interesting one... if people want that to work, then we do need some way of ensuring that either we create a document before we pick a principal for the javascript: load or flagging javascript: loads to just run no matter whether the principals match.

Comment 6

[:Gijs (he/him)]

Based on the comments here, I don't think we want to move ahead with this, at least in its current form.