Bug 207506 - Parent service worker controller should be used for child iframe as per https://w3c.github.io/ServiceWorker/#control-and-use-window-client
Summary: Parent service worker controller should be used for child iframe as per https...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-10 13:59 PST by youenn fablet
Modified: 2020-02-11 15:32 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.47 KB, patch)
2020-02-10 14:01 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.47 KB, patch)
2020-02-11 06:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.26 KB, patch)
2020-02-11 09:32 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (8.36 KB, patch)
2020-02-11 13:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-02-10 13:59:15 PST
We implement an old version of the spec.
Comment 1 youenn fablet 2020-02-10 14:01:10 PST
Created attachment 390292 [details]
Patch
Comment 2 youenn fablet 2020-02-11 06:34:18 PST
Created attachment 390360 [details]
Patch
Comment 3 youenn fablet 2020-02-11 09:32:14 PST
Created attachment 390375 [details]
Patch
Comment 4 Darin Adler 2020-02-11 12:47:22 PST
Comment on attachment 390375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390375&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1060
> +// https://w3c.github.io/ServiceWorker/#control-and-use-window-client

The terminology used in the specification is so different from the terminology we use here. It made hard for me to understand. For example, the specification doesn’t talk about a "controller" at all.

> Source/WebCore/loader/DocumentLoader.cpp:1061
> +static inline bool isInheritingControllerFromParent(const Document& document, const Document& parent)

Name should probably be:

    shouldApplyActiveServiceWorkerFromParent

> Source/WebCore/loader/DocumentLoader.cpp:1067
> +    if (document.url().protocolIsInHTTPFamily())
> +        return false;
> +    if (document.securityOrigin().isUnique())
> +        return false;
> +    return parent.securityOrigin().canAccess(document.securityOrigin());

I think this might be more readable as !a && !b && c rather than with cascading if statements.

> Source/WebCore/loader/DocumentLoader.cpp:1098
> +            } else if (auto* parent = m_frame->document()->parentDocument()) {

m_frame->document()-> is repeated over 10 times in this function! There’s got to be some refactoring we can do to cut down on that distracting repetition safely (some day).

> LayoutTests/http/tests/workers/service/serviceworkerclients-claim.https-expected.txt:3
> +CONSOLE MESSAGE: Origin null is not allowed by Access-Control-Allow-Origin.
> +CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/pinkelephant due to access control checks.
> +CONSOLE MESSAGE: line 1: Unhandled Promise Rejection: TypeError: Origin null is not allowed by Access-Control-Allow-Origin.

I don’t understand these changes to the results. Nothing in the change log makes it clear either.
Comment 5 youenn fablet 2020-02-11 13:15:50 PST
(In reply to Darin Adler from comment #4)
> Comment on attachment 390375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390375&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1060
> > +// https://w3c.github.io/ServiceWorker/#control-and-use-window-client
> 
> The terminology used in the specification is so different from the
> terminology we use here. It made hard for me to understand. For example, the
> specification doesn’t talk about a "controller" at all.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1061
> > +static inline bool isInheritingControllerFromParent(const Document& document, const Document& parent)
> 
> Name should probably be:
> 
>     shouldApplyActiveServiceWorkerFromParent

OK, will take shouldUseActiveServiceWorkerFromParent

> > Source/WebCore/loader/DocumentLoader.cpp:1067
> > +    if (document.url().protocolIsInHTTPFamily())
> > +        return false;
> > +    if (document.securityOrigin().isUnique())
> > +        return false;
> > +    return parent.securityOrigin().canAccess(document.securityOrigin());
> 
> I think this might be more readable as !a && !b && c rather than with
> cascading if statements.

OK

> > Source/WebCore/loader/DocumentLoader.cpp:1098
> > +            } else if (auto* parent = m_frame->document()->parentDocument()) {
> 
> m_frame->document()-> is repeated over 10 times in this function! There’s
> got to be some refactoring we can do to cut down on that distracting
> repetition safely (some day).

Will do in a follow up.
 
> > LayoutTests/http/tests/workers/service/serviceworkerclients-claim.https-expected.txt:3
> > +CONSOLE MESSAGE: Origin null is not allowed by Access-Control-Allow-Origin.
> > +CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/pinkelephant due to access control checks.
> > +CONSOLE MESSAGE: line 1: Unhandled Promise Rejection: TypeError: Origin null is not allowed by Access-Control-Allow-Origin.
> 
> I don’t understand these changes to the results. Nothing in the change log
> makes it clear either.

The iframe is doing a fetch. Since it is no longer controlled, it goes to the network where it fails due to CORS. Will beef up the ChangeLog.
Comment 6 youenn fablet 2020-02-11 13:22:36 PST
Created attachment 390412 [details]
Patch for landing
Comment 7 Radar WebKit Bug Importer 2020-02-11 14:16:18 PST
<rdar://problem/59363308>
Comment 8 WebKit Commit Bot 2020-02-11 15:32:15 PST
Comment on attachment 390412 [details]
Patch for landing

Clearing flags on attachment: 390412

Committed r256381: <https://trac.webkit.org/changeset/256381>
Comment 9 WebKit Commit Bot 2020-02-11 15:32:17 PST
All reviewed patches have been landed.  Closing bug.