1426979 - dynamically added iframes are not controlled by service workers properly

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating the crash in bug 1425975 comment 58 I noticed some strange behavior:

1. Open a site with a service worker like https://fetch-event-echo.glitch.me
2. Verify its controlled
3. Use the web console to add an iframe with no src attribute
4. Notice the frame is not controlled
5. Set the frame's source to the same as the top level windows
6. Notice that the final loaded window is not controlled either

We should be inheriting the controller in (4) and using the load interception to control in (6).

None of this works in firefox 57 either.  We should not do (4) in earlier version of firefox, but we should do (6) in earlier versions.

I'll investigate this when I return from holiday PTO.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Also, statically defined <iframe> elements give a null controller, but assertions suggest ClientSource::SetController() is being called.  Setting a src attribute to a controlled URL results in a controlled frame as well.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make about-blank-replacement.https.html verify that controller property is set. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ignore about:blank channels when determining if a window ClientSource is controlled or not. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make about-blank-replacement.https.html verify that controller property is set. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I need to fix bug 1425975 before I can do a try build here.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0beb462e6d198eb5a72d37ac1f7bf85299c2314

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940300 [details] [diff] [review]
P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth

So the root problem here was the code I added to cover this case:

1. about:blank inherits controller from parent
2. frame loads a real site that is not controlled
3. we must clear the controller to avoid insanity

What I did not realize was that about:blank and about:srcdoc documents actually have an nsIChannel and loadinfo.  These loadinfos will never have a controller so we always trigger the code that clears the controller.

This patch fixes the problem by ignoring the loadinfo for about:blank and about:srcdoc windows.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8940307 [details] [diff] [review]
P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

This expands the about-blank-replacements.https.html test to:

1. Verify that the controller property is set correctly.
2. Check simple static about:blank frame that never loads a src.
3. Check dynamic about:blank frame without a src.
4. Check about:srcdoc frame.

Technically some of these are not about "replacement", but they fit well in this tests infrastructure bits.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8940300 [details] [diff] [review]
P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth

Review of attachment 8940300 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindowInner.cpp
@@ +1750,5 @@
>    bool newClientSource = false;
>  
> +  // Get the load info for the document if we performed a load.  Be careful
> +  // not to look at about:blank or about:srcdoc loads as they won't contain
> +  // the state we are looking for here.

This comment is enigmatic, suggest fusing the useful comment from comment 11 into the comment.  Something like:

// Get the load info for the document if we performed a load.  Be careful
// not to look at about:blank or about:srcdoc loads as although they have
// a channel and loadinfo, their loadinfo will never be controlled, so if
// we process them, we will erroneously clear the controller.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8940307 [details] [diff] [review]
P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

Review of attachment 8940307 [details] [diff] [review]:
-----------------------------------------------------------------

Great coverage and comment documentation!

::: testing/web-platform/tests/service-workers/service-worker/about-blank-replacement.https.html
@@ +158,5 @@
> +  await doAsyncTest(t, 'resources/about-blank-replacement-blank-nested-frame.html');
> +}, 'Simple about:blank is controlled and is exposed to clients.matchAll().');
> +
> +promise_test(async function(t) {
> +  // Execute a test where the nested frame is an iframe without a src

nit: s/iframe without a src attribute/iframe using a non-empty srcdoc containing only a tag pair so its textContent is still empty/.
also: s/The simple nested about:blank should/The nested iframe should/

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1991807b39
P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c611046a0c
P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1e1991807b39
https://hg.mozilla.org/mozilla-central/rev/a4c611046a0c