Use DocumentChannel for srcdoc loads
Categories
(Core :: Networking, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox74 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(4 files, 1 obsolete file)
DocumentChannel currently is never used for srcdoc= loads.
DocumentChannelChild::RecvRedirectToRealChannel/ContentChild::RecvCrossProcessRedirect both only use NS_NewChannelInternal to create the final channel.
It looks like this doesn't work for srcdoc, so we need to add support for creating a channel using an input stream, like nsDocShell::CreateChannelForLoadState does.
|
||
Updated•5 years ago
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 1•4 years ago
|
||
|
|
Assignee | |
Comment 2•4 years ago
|
||
Depends on D57586
|
|
Assignee | |
Comment 3•4 years ago
|
||
Depends on D57587
|
|
Assignee | |
Comment 4•4 years ago
|
||
Depends on D57588
|
|
Assignee | |
Comment 5•4 years ago
|
||
Depends on D57589
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4a5d10759bf3 Don't require nsIChildChannel for process switching, as we don't need this for DocumentChannel either. r=mayhemer,kmag https://hg.mozilla.org/integration/autoland/rev/10ad2e4d27c3 Allow DocumentChannel for srcdoc= loads. r=kmag https://hg.mozilla.org/integration/autoland/rev/73c37ad27d18 Fix missing flags write. r=kmag https://hg.mozilla.org/integration/autoland/rev/8e3b650d9313 Add more logging to DocumentChannel. r=mayhemer,jya https://hg.mozilla.org/integration/autoland/rev/b4f71abf75fe Remove crashtest that depended on specific load timing with srcdoc that wont work if we route through DocumentChannel. r=emilio
|
||
Comment 7•4 years ago
|
||
Backed out for Lint failure.
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=281989548&repo=autoland
Backout: https://hg.mozilla.org/integration/autoland/rev/5960274234be7d62122c7cd74440d06049ec31b7
|
|
||
Comment 8•4 years ago
|
||
Pushed by rmaries@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ddf722557c33 Don't require nsIChildChannel for process switching, as we don't need this for DocumentChannel either. r=mayhemer,kmag https://hg.mozilla.org/integration/autoland/rev/59cff6014447 Allow DocumentChannel for srcdoc= loads. r=kmag https://hg.mozilla.org/integration/autoland/rev/45287a2ec476 Fix missing flags write. r=kmag https://hg.mozilla.org/integration/autoland/rev/c769e733f588 Add more logging to DocumentChannel. r=mayhemer,jya https://hg.mozilla.org/integration/autoland/rev/583dac2feebc Remove crashtest that depended on specific load timing with srcdoc that wont work if we route through DocumentChannel. r=emilio
|
|
||
Comment 10•4 years ago
•
|
||
Backed out for perma fails on test_enumerateDevices_navigation.html.
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281996003&repo=autoland&lineNumber=16156
Later edit.
Also caused: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281995996&repo=autoland&lineNumber=15994
Backout: https://hg.mozilla.org/integration/autoland/rev/c8ce8b91465a7192b056102481d633fef850fac6
|
|
Assignee | |
Comment 11•4 years ago
|
||
Hmm, looks like test_enumerateDevices_navigation.html and test_getUserMedia_trackEnded.html both fail.
These tests appear to be relying on srcdoc mutations loading immediately.
Both tests are expecting the srcdoc to be loaded when the test starts, which with my changes doesn't always happen. Setting the srcdoc attribute from the test code, and then waiting on the load event fixes those issues.
test_enumerateDevices_navigation.html is also expecting setting srcdoc="" to navigate the document away reliably before enumerateDevices() completes. This also isn't guaranteed, and these patches make it reliably not-true.
I'm not sure how to test this really, without a way to slow down enumerateDevices and ensure that it doesn't complete until later.
Maybe a test mode that blocks it internally somehow, and we wait for the srcdoc to fire a load event (for the new blank doc), and then unblock the enumerateDevices promise, and ensure that it remains blocked for the reason we're testing? Not sure if it's worth the effort.
|
||
Comment 12•4 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
Setting the srcdoc attribute from the test code, and then waiting on the load event fixes those issues.
Sounds good, thanks!
Maybe a test mode that blocks it internally somehow, and we wait for the srcdoc to fire a load event (for the new blank doc), and then unblock the enumerateDevices promise, and ensure that it remains blocked for the reason we're testing? Not sure if it's worth the effort.
Yeah too much work for a corner case. Please, reverse the check for now in order to make sure that it's something that hardly ever or never happens from a mochitest. In addition to that open a bug, against me, to try excerising that code from gtests. I could simulate a delay there by using a moc cubeb backend.
Please let me know if you need help with any of those. Regarding reviews I am happy to take, keep in your mind I'll be on PTO for 2 weeks after today. I'll be available till late though.
|
|
Assignee | |
Comment 13•4 years ago
|
||
The other issue here is that the second and third subtests of html/browsers/origin/relaxing-the-same-origin-restriction/document_domain_setter_srcdoc.html fail.
It looks like the second one of these sets document.domain within a srcdoc= iframe, and then checks that it can still access its parent document's content.
From what I can tell, the URI that we use for document.document is the same, so the only actual change is that we lose the port (:8000) from the principal.
We then fail a bunch of IsPlatformObjectSameOrigin checks.
With DocumentChannel enabled, the only difference between the two principals is that the objectPrincipal has the document.domain override removing the port.
With DocumentChannel disabled, they are literally the same instance of the same ContentPrincipal, so both have the override and the check passes.
My assumption (that I haven't fully checked) is that because we're doing an IPDL roundtrip for the LoadInfo when creating the DocumentChannel, we end up serializing the Principal, and thus the final one on the channel (and then the Document) isn't the same instance as we start with (from the load context).
What's not clear to me is why we're relying on that, and if that's a bug or not. Boris, does that sound expected to you?
|
|
||
Comment 14•4 years ago
|
||
With DocumentChannel disabled, they are literally the same instance of the same ContentPrincipal
Yes, that is the right behavior, and an explicit spec requirement. See https://html.spec.whatwg.org/multipage/browsers.html#determining-the-origin step 5. The origin of the srcdoc document is literally the same object as the origin of the document containing the iframe.
|
|
Assignee | |
Comment 15•4 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)
With DocumentChannel disabled, they are literally the same instance of the same ContentPrincipal
Yes, that is the right behavior, and an explicit spec requirement. See https://html.spec.whatwg.org/multipage/browsers.html#determining-the-origin step 5. The origin of the srcdoc document is literally the same object as the origin of the document containing the iframe.
Fascinating, thanks for that!
Given that, I think trying to serialize via the parent process for non-sandboxed srcdoc loads probably isn't going to work (without weird hacks to find the old principal instance when we get back to the old process).
I'll switch to just using DocumentChannel for sandboxed ones, since that's the only time that we might want to load them in a different process.
|
||
Updated•4 years ago
|
|
||
Comment 16•4 years ago
|
||
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3f05a0cebcea Don't require nsIChildChannel for process switching, as we don't need this for DocumentChannel either. r=mayhemer,kmag https://hg.mozilla.org/integration/autoland/rev/4c86fb804a50 Allow DocumentChannel for srcdoc= loads. r=kmag https://hg.mozilla.org/integration/autoland/rev/5ea7cfbd339a Fix missing flags write. r=kmag https://hg.mozilla.org/integration/autoland/rev/35c4ea0dd402 Add more logging to DocumentChannel. r=mayhemer,jya
|
||
Comment 17•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/3f05a0cebcea
https://hg.mozilla.org/mozilla-central/rev/4c86fb804a50
https://hg.mozilla.org/mozilla-central/rev/5ea7cfbd339a
https://hg.mozilla.org/mozilla-central/rev/35c4ea0dd402
Description
•