[Fetch API] Headers should be combined with ',' and not ', '

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

This is also the case for XHR

[youenn fablet]

Created attachment 288261 [details]
Patch

[Build Bot]

Comment on attachment 288261 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

[Build Bot]

Created attachment 288265 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 288261 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

[Build Bot]

Created attachment 288266 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

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

[youenn fablet]

Created attachment 288267 [details]
Patch

[Alex Christensen]

Comment on attachment 288267 [details]
Patch

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

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:100
> -            result.iterator->value = result.iterator->value + ", " + value;
> +            result.iterator->value = result.iterator->value + "," + value;

Somebody once told me makeString is faster than operator+ because this does unnecessary allocations and copies.

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10
> -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) 
> +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got ""

Should these tests be changed?

[youenn fablet]

>LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10
> > -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) 
> > +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got ""
> 
> Should these tests be changed?

I tried making the error messages more descriptive, but they would be flaky (see previous patch).
The tests do not need to be fixed. It is the lower HTTP library that would need to be fixed as they insert spaces for some headers: ct, pragma...
That should be done elsewhere

[youenn fablet]

Created attachment 288278 [details]
Using makeString

[youenn fablet]

Thanks for the comments

> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100
> > -            result.iterator->value = result.iterator->value + ", " + value;
> > +            result.iterator->value = result.iterator->value + "," + value;
> 
> Somebody once told me makeString is faster than operator+ because this does
> unnecessary allocations and copies.

Done.

> > LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10
> > -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) 
> > +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got ""
> 
> Should these tests be changed?

As said above, we should be able to pass these tests, but that should be left to another bug. It might be the case that other ports might pass this test directly.

[Alex Christensen]

Comment on attachment 288278 [details]
Using makeString

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

> LayoutTests/imported/w3c/ChangeLog:10
> +        This is due to the fact that the underlying HTTP code is probably reprocessing those headers and readding ', ' in lieu of ','.

But it wasn't before?  These tests passed before this change.  I'm not convinced.  I don't think we should regress this test.

[youenn fablet]

> > LayoutTests/imported/w3c/ChangeLog:10
> > +        This is due to the fact that the underlying HTTP code is probably reprocessing those headers and readding ', ' in lieu of ','.
> 
> But it wasn't before?  These tests passed before this change.  I'm not
> convinced.  I don't think we should regress this test.

Is is true that these tests were passing before the changes.
It is also true that these tests were not conforming with the spec.
The fact that some of these tests were passing is a proof that WebKit was not compliant with the spec.

Now that this patch makes the test conforming and updates part of the implementation, we see that some tests are passing (which is good), while some still do not work.
They do not work for two reasons:
- headers are not set according user input (previous failing tests).
- headers are reserialised at a lower level with a '' between values (new failing tests, Content-Type e.g.). This should be fixed at CFNetwork level. Plan is to file a radar once WebKit layer gets fixed.

[youenn fablet]

Created attachment 288402 [details]
Adding new test to keep same level of test coverage

[Alex Christensen]

If web platform tests are not conforming with the spec, web platform tests ought to be changed.

[youenn fablet]

(In reply to comment #14)
> If web platform tests are not conforming with the spec, web platform tests
> ought to be changed.

headers-combined was already changed in wpt repo.
I will upload the other changed WPT tests in this patch once it gets landed in WebKit.

[youenn fablet]

Or I can prepare the PR in parallel, I'll do it when possible

[youenn fablet]

https://github.com/w3c/web-platform-tests/pull/3689

[youenn fablet]

(In reply to comment #14)
> If web platform tests are not conforming with the spec, web platform tests
> ought to be changed.

PR was merged today.
The WPT tests are now changed according to this patch in W3C repo.

[Sam Weinig]

Comment on attachment 288402 [details]
Adding new test to keep same level of test coverage

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

> Source/WebCore/loader/CrossOriginAccessControl.cpp:134
> +                headerBuffer.appendLiteral(",");

This can use headerBuffer.append(',')

[youenn fablet]

Created attachment 289038 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289038 [details]
Patch for landing

Rejecting attachment 289038 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 289038, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
n "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-combine-expected.txt: needs update
LayoutTests/imported/w3cqweb-platform-tests/fetch/api/headers/headers-combine-expected.txt: needs update
update-index --refresh: command returned error: 1


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

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

[youenn fablet]

Created attachment 289047 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289047 [details]
Patch for landing

Clearing flags on attachment: 289047

Committed r206014: <http://trac.webkit.org/changeset/206014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 289047 [details]
Patch for landing

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

> Source/WebCore/platform/network/HTTPHeaderMap.cpp:100
> +            result.iterator->value = makeString(result.iterator->value, ",", value);

I think it would be slightly more efficient if you used ',' instead of "," here, skipping a call to the strlen function. Unless makeString does not know how to handle single characters.

[youenn fablet]

(In reply to comment #25)
> Comment on attachment 289047 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289047&action=review
> 
> > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100
> > +            result.iterator->value = makeString(result.iterator->value, ",", value);
> 
> I think it would be slightly more efficient if you used ',' instead of ","
> here, skipping a call to the strlen function. Unless makeString does not
> know how to handle single characters.

Filed bug 162140 for that purpose