Implement "create a potential-CORS request"

• Component: Page Loading
• Status: RESOLVED FIXED

[Rob Buis]

Implement "create a potential-CORS request" and remove deprecatedSetAsPotentiallyCrossOrigin.

[Rob Buis]

Created attachment 385860 [details]
Patch

[Rob Buis]

Created attachment 385863 [details]
Patch

[Rob Buis]

Created attachment 385871 [details]
Patch

[Rob Buis]

Created attachment 385874 [details]
Patch

[Rob Buis]

Created attachment 385879 [details]
Patch

[Rob Buis]

Created attachment 385886 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 385886 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Adjusts test to same-origin fallback behavior by using CORS.

These changes seem to suggest high likelihood of breaking behaviors websites could rely on. What evidence exists to demonstrate them being fine?

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/cloneNode-expected.txt:1
> +CONSOLE MESSAGE: Unsafe attempt to load URL javascript:"network error" from origin http://localhost:8800. Domains, protocols and ports must match.

Can you elaborate on why this is right, in particular?

[Rob Buis]

(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 385886 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385886&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Adjusts test to same-origin fallback behavior by using CORS.
> 
> These changes seem to suggest high likelihood of breaking behaviors websites
> could rely on. What evidence exists to demonstrate them being fine?

It is specified that way for track loading:
https://html.spec.whatwg.org/multipage/media.html#sourcing-out-of-band-text-tracks:create-a-potential-cors-request

Chrome and Firefox implement it (I should have added that to the Changelog, will fix). 

> > LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/cloneNode-expected.txt:1
> > +CONSOLE MESSAGE: Unsafe attempt to load URL javascript:"network error" from origin http://localhost:8800. Domains, protocols and ports must match.
> 
> Can you elaborate on why this is right, in particular?

That does look a bit odd, I will debug what is triggering those console messages.

[Rob Buis]

Created attachment 385947 [details]
Patch

[youenn fablet]

Comment on attachment 385947 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        The new behavior matches that of Chrome and Firefox.

Can you detail what behavioural changes are one there?

[Rob Buis]

Created attachment 386201 [details]
Patch

[Rob Buis]

Comment on attachment 385947 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        The new behavior matches that of Chrome and Firefox.
> 
> Can you detail what behavioural changes are one there?

I tried to add more details in all ChangeLog files. Note that if the same-origin fallback is controversial I can remove it from the patch.

[youenn fablet]

Comment on attachment 386201 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        not specified.

Could we split the patch in two parts: one which is just a refactoring and then the one about text track loading?

> Source/WebCore/loader/CrossOriginAccessControl.cpp:122
> +CachedResourceRequest createPotentialAccessControlRequest(ResourceRequest&& request, Document& document, const String& crossOriginAttribute, ResourceLoaderOptions&& options, bool sameOriginFlag)

Can we move the options as the second parameter, that will allow to group the moved parameters together.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:131
> +        auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) };

CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) };

> Source/WebCore/loader/CrossOriginAccessControl.cpp:142
>      auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) };

CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) };

> Source/WebCore/loader/TextTrackLoader.cpp:163
> +    auto cueRequest = createPotentialAccessControlRequest(WTFMove(resourceRequest), document, element.mediaElementCrossOriginAttribute(), WTFMove(options), true);

An enum would be slightly clearer.

[youenn fablet]

@EricCarlson, any concern about changing text track loading to same-origin by default, as done by Chrome and Firefox nowadays apparently?

[Rob Buis]

Created attachment 386673 [details]
Patch

[Rob Buis]

Comment on attachment 386201 [details]
Patch

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

I think I addressed your comments.

>> Source/WebCore/ChangeLog:13
>> +        not specified.
> 
> Could we split the patch in two parts: one which is just a refactoring and then the one about text track loading?

Doing that now, the new patch does not contain the text track loading changes.

>> Source/WebCore/loader/CrossOriginAccessControl.cpp:122
>> +CachedResourceRequest createPotentialAccessControlRequest(ResourceRequest&& request, Document& document, const String& crossOriginAttribute, ResourceLoaderOptions&& options, bool sameOriginFlag)
> 
> Can we move the options as the second parameter, that will allow to group the moved parameters together.

Done and adjusted the call sites.

>> Source/WebCore/loader/CrossOriginAccessControl.cpp:131
>> +        auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) };
> 
> CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) };

Done.

>> Source/WebCore/loader/CrossOriginAccessControl.cpp:142
>>      auto cachedRequest = CachedResourceRequest { WTFMove(request), WTFMove(options) };
> 
> CachedResourceRequest cachedRequest { WTFMove(request), WTFMove(options) };

Done.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 386673 [details]:

imported/w3c/web-platform-tests/IndexedDB/large-requests-abort.html bug 205724 (author: [e-mail])
imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 201481 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 386673 [details]:

imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-large.html bug 201849
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 386673 [details]
Patch

Clearing flags on attachment: 386673

Committed r254000: <https://trac.webkit.org/changeset/254000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58298224>

[Rob Buis]

Reopening as a reminder that Eric still needs to react (comment 14) :)

[Eric Carlson]

(In reply to youenn fablet from comment #14)
> @EricCarlson, any concern about changing text track loading to same-origin
> by default, as done by Chrome and Firefox nowadays apparently?

No, I think it is the right thing.

[Eric Carlson]

(In reply to Rob Buis from comment #22)
> Reopening as a reminder that Eric still needs to react (comment 14) :)

Closing bug.

Thanks for checking in Rob!

[Rob Buis]

Reopening to attach new patch.

[Rob Buis]

Created attachment 388140 [details]
Patch

[Rob Buis]

Created attachment 388141 [details]
Patch

[Rob Buis]

Created attachment 388214 [details]
Patch

[Rob Buis]

Created attachment 388215 [details]
Patch

[youenn fablet]

Comment on attachment 388215 [details]
Patch

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

> Source/WebCore/loader/CrossOriginAccessControl.cpp:140
> +    options.storedCredentialsPolicy = credentials == FetchOptions::Credentials::Include ? StoredCredentialsPolicy::Use : document.securityOrigin().canRequest(request.url()) ? StoredCredentialsPolicy::Use : StoredCredentialsPolicy::DoNotUse;

Can we write it with something like:
switch (credentials) {
case FetchOptions::Credentials::Omit:
   ...
case FetchOptions::Credentials::Omit:
   ...
case FetchOptions::Credentials::Include:
   ...
case FetchOptions::Credentials::SameOrigin:
   ...
}
This should be easier to use and we would also ensure consistency between the two values (for instance omit => DoNotUse).

[Rob Buis]

Created attachment 388225 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388225 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388225 [details]
Patch

Clearing flags on attachment: 388225

Committed r254821: <https://trac.webkit.org/changeset/254821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Rob Buis]

(In reply to youenn fablet from comment #30)
> Comment on attachment 388215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388215&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:140
> > +    options.storedCredentialsPolicy = credentials == FetchOptions::Credentials::Include ? StoredCredentialsPolicy::Use : document.securityOrigin().canRequest(request.url()) ? StoredCredentialsPolicy::Use : StoredCredentialsPolicy::DoNotUse;
> 
> Can we write it with something like:
> switch (credentials) {
> case FetchOptions::Credentials::Omit:
>    ...
> case FetchOptions::Credentials::Omit:
>    ...
> case FetchOptions::Credentials::Include:
>    ...
> case FetchOptions::Credentials::SameOrigin:
>    ...
> }
> This should be easier to use and we would also ensure consistency between
> the two values (for instance omit => DoNotUse).

Yes, this did make the code easier to read and reason about.