Closed Bug 1598520 Opened 2 years ago Closed 2 years ago

Use DocumentChannel for srcdoc loads

Categories

(Core :: Networking, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla74
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.

Whiteboard: [necko-triaged]
Assignee: nobody → matt.woodrow

Depends on D57587

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

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.

Flags: needinfo?(matt.woodrow) → needinfo?(achronop)

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

Flags: needinfo?(achronop)

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?

Flags: needinfo?(bzbarsky)

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.

Flags: needinfo?(bzbarsky)

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

Attachment #9116604 - Attachment is obsolete: true
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
See Also: → 1644825
You need to log in before you can comment on or make changes to this bug.