Bug 175264 – [Beacon] Add support for CORS-preflighting for WK2 / NETWORK_SESSION

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Add support for CORS-preflighting when sendBeacon() is called with a payload that has a non-safelisted MIME type.

[Chris Dumez]

<rdar://problem/33547793>

[Chris Dumez]

Created attachment 317456 [details]
WIP Patch

[Chris Dumez]

Created attachment 317457 [details]
WIP Patch

[Build Bot]

Attachment 317457 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/PingLoad.cpp:41:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317477 [details]
WIP Patch

[Build Bot]

Attachment 317477 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:104:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/NetworkConnectionToWebProcess.cpp".  [list/order] [5]
ERROR: Source/WebKit/NetworkProcess/PingLoad.cpp:146:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317478 [details]
WIP Patch

[Build Bot]

Attachment 317478 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317484 [details]
WIP Patch

[Build Bot]

Attachment 317484 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317499 [details]
WIP Patch

[Build Bot]

Attachment 317499 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317501 [details]
WIP Patch

[Build Bot]

Attachment 317501 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317539 [details]
WIP Patch

[Build Bot]

Attachment 317539 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 5 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 317539 [details]
WIP Patch

Attachment 317539 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4274844

New failing tests:
http/wpt/beacon/headers/cors-preflight-success.html
http/wpt/beacon/headers/cors-preflight-failure.html

[Build Bot]

Created attachment 317544 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 317580 [details]
WIP Patch

[Build Bot]

Attachment 317580 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 317580 [details]
WIP Patch

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

> LayoutTests/imported/w3c/web-platform-tests/beacon/resources/beacon-preflight.py:24
> +def main(request, response):

I could not figure out how to keep this script in http/wpt, I was getting 404 errors. @Youenn: please let me know if you know how I can do this.

[youenn fablet]

Comment on attachment 317580 [details]
WIP Patch

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

> LayoutTests/http/wpt/beacon/headers/cors-preflight-failure.html:13
> +var RESOURCES_DIR = "/beacon/resources/";

If you want to have beacon-preflight.py in LayoutTests/http/wpt/beacon/resources, you need to set RESOURCES_DIR to an absolute path like /WebKit/beacon/resources or a relative one like ../resources/
The latter would ease the migration to web-platform-tests.

[Chris Dumez]

Created attachment 317595 [details]
Patch

[Chris Dumez]

(In reply to youenn fablet from comment #22)
> Comment on attachment 317580 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317580&action=review
> 
> > LayoutTests/http/wpt/beacon/headers/cors-preflight-failure.html:13
> > +var RESOURCES_DIR = "/beacon/resources/";
> 
> If you want to have beacon-preflight.py in
> LayoutTests/http/wpt/beacon/resources, you need to set RESOURCES_DIR to an
> absolute path like /WebKit/beacon/resources or a relative one like
> ../resources/
> The latter would ease the migration to web-platform-tests.

I have to use an absolute URL because this is testing CORS. I need a cross-origin URL.

[Build Bot]

Attachment 317595 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 317607 [details]
Patch

[Build Bot]

Attachment 317607 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 317607 [details]
Patch

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

> Source/WebCore/loader/LoaderStrategy.h:65
> +    virtual void createPingHandle(NetworkingContext*, ResourceRequest&, Ref<SecurityOrigin>&& sourceOrigin, const FetchOptions&) = 0;

This ResourceRequest+origin+fetch options API is looking good to me, since this is what a fetch request is.

> Source/WebCore/loader/PingLoader.h:59
> +    static void startPingLoad(Frame&, ResourceRequest&, SecurityOrigin& sourceOrigin, ShouldFollowRedirects);

I guess the styler might not like sourceOrigin parameter name.

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:71
> +    loadParameters.request = createAccessControlPreflightRequest(m_parameters.originalRequest, m_parameters.sourceOrigin, m_parameters.originalRequest.httpReferrer());

We had issues in the past with preflight requests using credentials or using the session that has credentials.
Code here looks good to me with that respect.
It would be great though if we could add a test to ensure that since this is a new code path where we could do mistake.

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:74
> +        m_task = NetworkDataTask::create(*networkSession, *this, loadParameters);

It seems strange that we are not moving loadParameters.
It contains a request which is probably stored somewhere in NetworkDataTask and we would gain if we could move it instead of copying it.

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:83
> +    completionHandler(ResourceRequest { });

Can we write completionHandler({ })?

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:106
> +void NetworkCORSPreflightChecker::didCompleteWithError(const WebCore::ResourceError& error, const WebCore::NetworkLoadMetrics&)

This code is really similar to CrossOriginPreflightChecker::validatePreflightResponse.
I guess it is fine although some additional code sharing would be good too.

> Source/WebKit/NetworkProcess/PingLoad.cpp:43
> +    : m_parameters(parameters)

We should take a NetworkResourceLoadParameters&& here.
Probably came for past code, which probably comes from the fact that IPC is calling with const& parameters.
Maybe in the future IPC APIs could use &&, which might make the change to NetworkResourceLoadParameters&& useful.

> Source/WebKit/NetworkProcess/PingLoad.cpp:78
> +    // FIXME: Do a CORS preflight if necessary.

FIXME: Ensure max redirection count is lower than 20.

[Chris Dumez]

Comment on attachment 317607 [details]
Patch

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

>> Source/WebCore/loader/PingLoader.h:59
>> +    static void startPingLoad(Frame&, ResourceRequest&, SecurityOrigin& sourceOrigin, ShouldFollowRedirects);
> 
> I guess the styler might not like sourceOrigin parameter name.

Style script does not complain because of the 'source' prefix. I think the parameter name is useful here it clarifies that it is the SecurityOrigin of the source (e.g. not the destination).

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:71
>> +    loadParameters.request = createAccessControlPreflightRequest(m_parameters.originalRequest, m_parameters.sourceOrigin, m_parameters.originalRequest.httpReferrer());
> 
> We had issues in the past with preflight requests using credentials or using the session that has credentials.
> Code here looks good to me with that respect.
> It would be great though if we could add a test to ensure that since this is a new code path where we could do mistake.

Will clarify with your offline how to test this.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:74
>> +        m_task = NetworkDataTask::create(*networkSession, *this, loadParameters);
> 
> It seems strange that we are not moving loadParameters.
> It contains a request which is probably stored somewhere in NetworkDataTask and we would gain if we could move it instead of copying it.

OK.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:83
>> +    completionHandler(ResourceRequest { });
> 
> Can we write completionHandler({ })?

Yes, OK.

>> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:106
>> +void NetworkCORSPreflightChecker::didCompleteWithError(const WebCore::ResourceError& error, const WebCore::NetworkLoadMetrics&)
> 
> This code is really similar to CrossOriginPreflightChecker::validatePreflightResponse.
> I guess it is fine although some additional code sharing would be good too.

Ok, Will see if I can share a little bit more code here.

>> Source/WebKit/NetworkProcess/PingLoad.cpp:43
>> +    : m_parameters(parameters)
> 
> We should take a NetworkResourceLoadParameters&& here.
> Probably came for past code, which probably comes from the fact that IPC is calling with const& parameters.
> Maybe in the future IPC APIs could use &&, which might make the change to NetworkResourceLoadParameters&& useful.

Actually, the IPC code seems to correctly move. I'll fix in this patch.

>> Source/WebKit/NetworkProcess/PingLoad.cpp:78
>> +    // FIXME: Do a CORS preflight if necessary.
> 
> FIXME: Ensure max redirection count is lower than 20.

Will add FIXME and fix in a follow up.

[youenn fablet]

Patch looks good overall.
We could foresee a far future where https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch would more or less be implemented in network process.

As a side note, we do no see preflights in the web inspector.
I guess we would need to retrofit that information to the web process if we want to do that.

[Chris Dumez]

Created attachment 317645 [details]
Patch

[Chris Dumez]

Still working on the additional testing but this new iteration should address other review comments.

[Build Bot]

Attachment 317645 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 317645 [details]
Patch

Attachment 317645 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4280918

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html
http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html
http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html
http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html
http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[Build Bot]

Created attachment 317655 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317645 [details]
Patch

Attachment 317645 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4280980

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html
http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html
http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html
http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html
http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[Build Bot]

Created attachment 317659 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 317645 [details]
Patch

Attachment 317645 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4280964

New failing tests:
http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html
http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html
http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html
http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html
http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html
http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html
http/tests/xmlhttprequest/access-control-and-redirects-async.html

[Build Bot]

Created attachment 317660 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 317668 [details]
Patch

[Chris Dumez]

Created attachment 317670 [details]
Patch

[Chris Dumez]

Comment on attachment 317670 [details]
Patch

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

> LayoutTests/http/wpt/beacon/cors/cors-preflight-cookie.html:16
> +  testRunner.setAlwaysAcceptCookies(true);

Added cookie test as suggested. Sadly, it looks like I need rely on this testRunner API or the cookie does not get set cross origin.

[Build Bot]

Attachment 317670 [details] did not pass style-queue:


ERROR: Source/WebKit/CMakeLists.txt:103:  Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp".  [list/order] [5]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

What about an iframe?

[Chris Dumez]

(In reply to youenn fablet from comment #44)
> What about an iframe?

What do you mean? My test is using an iframe.

[WebKit Commit Bot]

Comment on attachment 317670 [details]
Patch

Clearing flags on attachment: 317670

Committed r220442: <http://trac.webkit.org/changeset/220442>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.