Bug 168115 – REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

When the header already exists the values is appended after a ',' in both cases, but for common headers ", " is used while for uncommon headers ',' is used. This makes test imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm to fail because of the space difference:

-PASS XMLHttpRequest: getResponseHeader() case-insensitive matching 
+FAIL XMLHttpRequest: getResponseHeader() case-insensitive matching assert_equals: expected "1, 2" but got "1,2"

This doesn't affect ports using CF, because CF already joins the headers, so they don't use HTTPHeaderMap::add(), but HTTPHeaderMap::set()

[Carlos Garcia Campos]

Created attachment 301153 [details]
Patch

[Build Bot]

Comment on attachment 301153 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html
http/tests/xmlhttprequest/check-combining-headers.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm
http/tests/xmlhttprequest/web-apps/005.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

[Build Bot]

Created attachment 301154 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 301153 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html
http/tests/xmlhttprequest/check-combining-headers.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm
http/tests/xmlhttprequest/web-apps/005.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm

[Build Bot]

Created attachment 301155 [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

[Carlos Garcia Campos]

Maybe it's imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm the one wrong, and it should be ',' instead of ", ". For example, imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html was updated in r205743 removing the space.

[Build Bot]

Comment on attachment 301153 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html
http/tests/xmlhttprequest/check-combining-headers.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm
http/tests/xmlhttprequest/web-apps/005.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

[Build Bot]

Created attachment 301156 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Build Bot]

Comment on attachment 301153 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html
http/tests/xmlhttprequest/check-combining-headers.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm
http/tests/xmlhttprequest/web-apps/005.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

[Build Bot]

Created attachment 301157 [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.6

[Chris Dumez]

Comment on attachment 301153 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use

And yet some tests are failing on Mac? :)

[Carlos Garcia Campos]

(In reply to comment #11)
> Comment on attachment 301153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301153&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use
> 
> And yet some tests are failing on Mac? :)

I meant for that particular test, that is not failing on mac with the patch applied. I don't understand why the other tests can pass, though.

[Carlos Garcia Campos]

Ok, looking at the tests in detail now I understand better what is going on

http/tests/xmlhttprequest/check-combining-headers.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

These have FAIL lines in the expectations, like:

FAIL XMLHttpRequest: setRequestHeader() - combining headers (Authorization) assert_equals: Combined header value should be t1,t2 expected "t1,t2" but got "t1, t2"

Whatever patch we write for this bug should make those pass.

The other ones failing expect ',' not ", ".

And finally the one I was trying to fix expects ", " but it should really expect ','. According to the HTTP spec, the space is optional, but fetch spec says it shouldn't be any trailing/leaving whitespace in values, see https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it was changed in w3c web platform tests, but I think they forgot about imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm. I guess it's not a good idea to change imported tests, so we can simply add a failure expectation for this test.

So, let's be consistent and use ',' everywhere. I'll submit a new patch.

[Carlos Garcia Campos]

Created attachment 301252 [details]
Updated patch

[Michael Catanzaro]

(In reply to comment #13)
> And finally the one I was trying to fix expects ", " but it should really
> expect ','. According to the HTTP spec, the space is optional, but fetch
> spec says it shouldn't be any trailing/leaving whitespace in values, see
> https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it
> was changed in w3c web platform tests, but I think they forgot about
> imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-
> insensitive.htm. I guess it's not a good idea to change imported tests, so
> we can simply add a failure expectation for this test.

(CC Youenn. How hard would it be to upstream this fix?)

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #13)
> > And finally the one I was trying to fix expects ", " but it should really
> > expect ','. According to the HTTP spec, the space is optional, but fetch
> > spec says it shouldn't be any trailing/leaving whitespace in values, see
> > https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it
> > was changed in w3c web platform tests, but I think they forgot about
> > imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-
> > insensitive.htm. I guess it's not a good idea to change imported tests, so
> > we can simply add a failure expectation for this test.
> 
> (CC Youenn. How hard would it be to upstream this fix?)

It is just a Pull Request on Github. It is not hard to fix a WPT.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > And finally the one I was trying to fix expects ", " but it should really
> > > expect ','. According to the HTTP spec, the space is optional, but fetch
> > > spec says it shouldn't be any trailing/leaving whitespace in values, see
> > > https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it
> > > was changed in w3c web platform tests, but I think they forgot about
> > > imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-
> > > insensitive.htm. I guess it's not a good idea to change imported tests, so
> > > we can simply add a failure expectation for this test.
> > 
> > (CC Youenn. How hard would it be to upstream this fix?)
> 
> It is just a Pull Request on Github. It is not hard to fix a WPT.

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

If it's too much trouble, send me a patch and I can help marking the PR on GitHub.

[Build Bot]

Comment on attachment 301252 [details]
Updated patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm

[Build Bot]

Created attachment 301253 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 301252 [details]
Updated patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm

[Build Bot]

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

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

[Build Bot]

Comment on attachment 301252 [details]
Updated patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm

[Build Bot]

Created attachment 301258 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[Build Bot]

Comment on attachment 301252 [details]
Updated patch

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm

[Build Bot]

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

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

[youenn fablet]

Fetch spec may change back to ", ".
See https://github.com/whatwg/xhr/issues/108 for some discussions about it.

Note that for some well-known headers, CFNetwork is converting "," in ", " on the fly.
This may be an issue if a user specifically wants no space in such header values.

[Michael Catanzaro]

Also, bots are still failing with the latest patch....

[Carlos Garcia Campos]

(In reply to comment #27)
> Also, bots are still failing with the latest patch....

Yes, precisely because CF generates ", ". If we decide to use ',' then CF based ports will need to add platform specific results for these tests.

[Darin Adler]

Comment on attachment 301252 [details]
Updated patch

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

Please fix the failing test on the Cocoa platforms before landing. I suspect that the comments about this code only affecting ports that don’t use CF was incorrect, given that a test result changed!

> LayoutTests/imported/w3c/ChangeLog:3
> +        HTTPHeaderMap add methods are not consistent to each other

Either "are not consistent" or "are not consistent with each other" would be OK. But "are not consistent to each other" is not correct English grammar.

This is C++, so these are "functions", not "methods".

But, also, why be so vague about what the inconsistency is? The comment should mention the space after the comma instead of leaving a mystery about what the difference is. I would use this title:

    REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields

> Source/WebCore/ChangeLog:14
> +        When the header already exists the values is appended after a ',' in both cases, but for common headers ", " is
> +        used while for uncommon headers ',' is used. This makes test
> +        imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm to fail because of the
> +        space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use
> +        HTTPHeaderMap::add(), but HTTPHeaderMap::set(). According to the HTTP spec, the space is optional, but fetch
> +        spec says it shouldn't be any trailing/leading whitespace in values. So let's use ',' everywhere and update the
> +        tests results to expect that.

If you do a bit more research on the history of this function, you will see that this inconsistency was introduced by <https://trac.webkit.org/changeset/206014>, a change that removed the space to match the Fetch specification. So I think the explanation is simpler than what you write here: We simply need to finish the job that was only partly done in that change.

[Darin Adler]

(In reply to comment #28)
> > Also, bots are still failing with the latest patch....
> 
> Yes, precisely because CF generates ", ". If we decide to use ',' then CF
> based ports will need to add platform specific results for these tests.

That is an interesting problem. Please talk with Youenn and other people from Apple about how we can resolve this!

[youenn fablet]

We want WebKit to be consistent here.
So the question we want to decide on is whether we want to use "," or ", ".
The fetch spec is mandating "," atm but may change if there is not enough support for it.
According IETF, they should be semantically equivalent...

XHR implementation is using ", " but XHR spec is using whatever fetch spec prescribes.
Other browsers afaik, are still using ", ".

For the Mac port, WebKit should be able to send headers without CFNetwork changing them on the fly.

[Carlos Garcia Campos]

Created attachment 301490 [details]
Patch for landing

Let's do what the current tests expect for now then.

[Build Bot]

Comment on attachment 301490 [details]
Patch for landing

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm

[Build Bot]

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

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

[Carlos Garcia Campos]

Created attachment 301588 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 301588 [details]
Patch for landing

Clearing flags on attachment: 301588

Committed r212355: <http://trac.webkit.org/changeset/212355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.