Bug 168115 - REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields
Summary: REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2017-02-10 05:35 PST by Carlos Garcia Campos
Modified: 2017-03-07 11:52 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2017-02-10 05:37 PST, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.31 MB, application/zip)
2017-02-10 06:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (862.61 KB, application/zip)
2017-02-10 06:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.52 MB, application/zip)
2017-02-10 06:50 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (767.65 KB, application/zip)
2017-02-10 06:53 PST, Build Bot
no flags Details
Updated patch (8.14 KB, patch)
2017-02-11 07:03 PST, Carlos Garcia Campos
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (740.39 KB, application/zip)
2017-02-11 08:04 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (865.74 KB, application/zip)
2017-02-11 08:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.49 MB, application/zip)
2017-02-11 08:17 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (689.42 KB, application/zip)
2017-02-11 08:18 PST, Build Bot
no flags Details
Patch for landing (8.51 KB, patch)
2017-02-14 05:46 PST, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (794.23 KB, application/zip)
2017-02-14 11:59 PST, Build Bot
no flags Details
Patch for landing (9.00 KB, patch)
2017-02-15 00:12 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-02-10 05:35:34 PST
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()
Comment 1 Carlos Garcia Campos 2017-02-10 05:37:31 PST
Created attachment 301153 [details]
Patch
Comment 2 Build Bot 2017-02-10 06:21:13 PST
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
Comment 3 Build Bot 2017-02-10 06:21:16 PST
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
Comment 4 Build Bot 2017-02-10 06:43:45 PST
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
Comment 5 Build Bot 2017-02-10 06:43:48 PST
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
Comment 6 Carlos Garcia Campos 2017-02-10 06:46:27 PST
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.
Comment 7 Build Bot 2017-02-10 06:50:42 PST
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
Comment 8 Build Bot 2017-02-10 06:50:47 PST
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
Comment 9 Build Bot 2017-02-10 06:53:05 PST
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
Comment 10 Build Bot 2017-02-10 06:53:09 PST
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
Comment 11 Chris Dumez 2017-02-10 07:55:05 PST
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? :)
Comment 12 Carlos Garcia Campos 2017-02-10 08:34:32 PST
(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.
Comment 13 Carlos Garcia Campos 2017-02-11 06:57:39 PST
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.
Comment 14 Carlos Garcia Campos 2017-02-11 07:03:16 PST
Created attachment 301252 [details]
Updated patch
Comment 15 Michael Catanzaro 2017-02-11 07:40:42 PST
(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?)
Comment 16 Chris Dumez 2017-02-11 07:42:15 PST
(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.
Comment 17 Chris Dumez 2017-02-11 07:43:30 PST
(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.
Comment 18 Build Bot 2017-02-11 08:04:25 PST
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
Comment 19 Build Bot 2017-02-11 08:04:28 PST
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
Comment 20 Build Bot 2017-02-11 08:08:09 PST
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
Comment 21 Build Bot 2017-02-11 08:08:13 PST
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
Comment 22 Build Bot 2017-02-11 08:17:38 PST
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
Comment 23 Build Bot 2017-02-11 08:17:43 PST
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
Comment 24 Build Bot 2017-02-11 08:18:07 PST
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
Comment 25 Build Bot 2017-02-11 08:18:11 PST
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
Comment 26 youenn fablet 2017-02-11 08:23:17 PST
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.
Comment 27 Michael Catanzaro 2017-02-11 08:43:34 PST
Also, bots are still failing with the latest patch....
Comment 28 Carlos Garcia Campos 2017-02-11 08:55:27 PST
(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.
Comment 29 Darin Adler 2017-02-12 00:37:26 PST
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.
Comment 30 Darin Adler 2017-02-12 00:38:14 PST
(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!
Comment 31 youenn fablet 2017-02-13 15:46:04 PST
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.
Comment 32 Carlos Garcia Campos 2017-02-14 05:46:54 PST
Created attachment 301490 [details]
Patch for landing

Let's do what the current tests expect for now then.
Comment 33 Build Bot 2017-02-14 11:59:13 PST
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
Comment 34 Build Bot 2017-02-14 11:59:19 PST
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
Comment 35 Carlos Garcia Campos 2017-02-15 00:12:54 PST
Created attachment 301588 [details]
Patch for landing
Comment 36 WebKit Commit Bot 2017-02-15 02:02:01 PST
Comment on attachment 301588 [details]
Patch for landing

Clearing flags on attachment: 301588

Committed r212355: <http://trac.webkit.org/changeset/212355>
Comment 37 WebKit Commit Bot 2017-02-15 02:02:08 PST
All reviewed patches have been landed.  Closing bug.