Bug 160028 – CrossOrigin preflight checker should compute the right Access-Control-Request-Headers value

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

As per https://fetch.spec.whatwg.org/#cors-preflight-fetch, computation of this value should be done before Origin header is set.
It should also remove safe request headers from the list and sort the unsafe headers.

[youenn fablet]

Created attachment 284692 [details]
Patch

[Build Bot]

Comment on attachment 284692 [details]
Patch

Attachment 284692 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1761974

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.html

[Build Bot]

Created attachment 284696 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 284702 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284702 [details]
Patch

Rejecting attachment 284702 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 284702, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1762526

[Alex Christensen]

Comment on attachment 284702 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:121
> +            preflightSuccess(ResourceRequest(request));

Is it really impossible to make request a ResourceRequest&&?  This copy shouldn't be necessary.

> Source/WebCore/platform/network/HTTPParsers.cpp:770
> +        return value == "application/x-www-form-urlencoded" || value == "multipart/form-data" || value == "text/plain";

These should probably be case-insensitive comparisons and tested as such. tExT/pLaIn should be safe, right?

[youenn fablet]

Comment on attachment 284702 [details]
Patch

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:121
>> +            preflightSuccess(ResourceRequest(request));
> 
> Is it really impossible to make request a ResourceRequest&&?  This copy shouldn't be necessary.

request is const and preflightSuccess will add the origin in the headers, so we need to make a copy.
Or we should make DocumentThreadableLoader constructor take a ResourceRequest&&.

>> Source/WebCore/platform/network/HTTPParsers.cpp:770
>> +        return value == "application/x-www-form-urlencoded" || value == "multipart/form-data" || value == "text/plain";
> 
> These should probably be case-insensitive comparisons and tested as such. tExT/pLaIn should be safe, right?

I need to fix that obviously, thanks!

[youenn fablet]

Created attachment 284761 [details]
Fixing case comparison and beefing up tests

[youenn fablet]

Created attachment 284764 [details]
Fixing case comparison and beefing up tests

[youenn fablet]

I added rebasing of some worker tests (currently skipped).
As can be seen, referrer is set for workers before preflighting, in WorkerThreadableLoader probably.
Like we did for Origin, setting referrer should be done at a lower loader code level.

[Alex Christensen]

> Or we should make DocumentThreadableLoader constructor take a
> ResourceRequest&&.
Yes.  Let's do that.  That's better than making a copy.

[Alex Christensen]

Comment on attachment 284764 [details]
Fixing case comparison and beefing up tests

r=me other than that.

[youenn fablet]

(In reply to comment #11)
> > Or we should make DocumentThreadableLoader constructor take a
> > ResourceRequest&&.
> Yes.  Let's do that.  That's better than making a copy.

OK, I'll prepare patches to optimize ResourceResponse and ResourceRequest transfer around DocumentThreadableLoader.

As for ResourceRequest, note that DocumentThreadableLoader is already creating a request copy when creating its CachedResourceRequest.
We could probably optimize this already without changing its clients.
That should go back to the previous number of ResourceRequest copies.

[WebKit Commit Bot]

Comment on attachment 284764 [details]
Fixing case comparison and beefing up tests

Clearing flags on attachment: 284764

Committed r203899: <http://trac.webkit.org/changeset/203899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.