Closed
Bug 1505850
Opened 6 years ago
Closed 6 years ago
GetInheritedPrincipal shouldn't bother creating a content viewer if we don't have one
Categories
(Core :: DOM: Navigation, enhancement, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox65 | --- | affected |
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 obsolete file)
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
: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...
Flags: needinfo?(bzbarsky)
Comment 3•6 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 4•6 years ago
|
||
(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•6 years ago
|
||
> 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.
Assignee | ||
Comment 6•6 years ago
|
||
Based on the comments here, I don't think we want to move ahead with this, at least in its current form.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Updated•6 years ago
|
Attachment #9023707 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•