WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2017-02-10 05:37:31 PST
Created
attachment 301153
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug