RESOLVED FIXED 168115
REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields
https://bugs.webkit.org/show_bug.cgi?id=168115
Summary REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without s...
Carlos Garcia Campos
Reported 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()
Attachments
Patch (1.86 KB, patch)
2017-02-10 05:37 PST, Carlos Garcia Campos
buildbot: commit-queue-
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
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
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
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
Updated patch (8.14 KB, patch)
2017-02-11 07:03 PST, Carlos Garcia Campos
darin: review+
buildbot: commit-queue-
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
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
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
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
Patch for landing (8.51 KB, patch)
2017-02-14 05:46 PST, Carlos Garcia Campos
buildbot: commit-queue-
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
Patch for landing (9.00 KB, patch)
2017-02-15 00:12 PST, Carlos Garcia Campos
no flags
Carlos Garcia Campos
Comment 1 2017-02-10 05:37:31 PST
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Carlos Garcia Campos
Comment 6 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.
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Chris Dumez
Comment 11 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? :)
Carlos Garcia Campos
Comment 12 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.
Carlos Garcia Campos
Comment 13 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.
Carlos Garcia Campos
Comment 14 2017-02-11 07:03:16 PST
Created attachment 301252 [details] Updated patch
Michael Catanzaro
Comment 15 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?)
Chris Dumez
Comment 16 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.
Chris Dumez
Comment 17 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.
Build Bot
Comment 18 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
Build Bot
Comment 19 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
Build Bot
Comment 20 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
Build Bot
Comment 21 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
Build Bot
Comment 22 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
Build Bot
Comment 23 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
Build Bot
Comment 24 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
Build Bot
Comment 25 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
youenn fablet
Comment 26 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.
Michael Catanzaro
Comment 27 2017-02-11 08:43:34 PST
Also, bots are still failing with the latest patch....
Carlos Garcia Campos
Comment 28 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.
Darin Adler
Comment 29 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.
Darin Adler
Comment 30 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!
youenn fablet
Comment 31 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.
Carlos Garcia Campos
Comment 32 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.
Build Bot
Comment 33 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
Build Bot
Comment 34 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
Carlos Garcia Campos
Comment 35 2017-02-15 00:12:54 PST
Created attachment 301588 [details] Patch for landing
WebKit Commit Bot
Comment 36 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>
WebKit Commit Bot
Comment 37 2017-02-15 02:02:08 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.