Parent service worker controller should be used for child iframe as per https://w3c.github.io/ServiceWorker/#control-and-use-window-client

[youenn fablet]

We implement an old version of the spec.

[youenn fablet]

Created attachment 390292 [details]
Patch

[youenn fablet]

Created attachment 390360 [details]
Patch

[youenn fablet]

Created attachment 390375 [details]
Patch

[Darin Adler]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 390412 [details]
Patch for landing

[Radar WebKit Bug Importer]

<rdar://problem/59363308>

[WebKit Commit Bot]

Comment on attachment 390412 [details]
Patch for landing

Clearing flags on attachment: 390412

Committed r256381: <https://trac.webkit.org/changeset/256381>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

*** Bug 207378 has been marked as a duplicate of this bug. ***