Closed Bug 1426979 Opened 6 years ago Closed 6 years ago

dynamically added iframes are not controlled by service workers properly

Categories

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

Product:
Core
Component:
DOM: Service Workers
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(2 files, 8 obsolete files)

P1 Ignore about:blank and about:srcdoc channels when determining if a window ClientSource is controlled or not. r=asuth
2.00 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P2 Make about-blank-replacement.https.html check simple about:blank and about:srcdoc cases. r=asuth
15.90 KB, patch
Details | Diff | Splinter Review 
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.
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.
I need to fix bug 1425975 before I can do a try build here.
Priority: -- → P1
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.
Attachment #8940300 - Flags: review?(bugmail)
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.
Attachment #8940307 - Flags: review?(bugmail)
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.
Attachment #8940300 - Flags: review?(bugmail) → review+
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/
Attachment #8940307 - Flags: review?(bugmail) → review+
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1e1991807b39
https://hg.mozilla.org/mozilla-central/rev/a4c611046a0c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: