1366945 - Failure in xhr/getallresponseheaders.htm

Status: RESOLVED DUPLICATE of bug 1491010

(Core :: DOM: Core & HTML, defect, P3)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Casing of known headers 2
assert_regexp_match: expected object "/THIS-is-A-test: 1, 2/" but got "Host: web-platform.test:8000\r\nUser-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0\r\nAccept: */*\r\nAccept-Language: en-US,en;q=0.5\r\nAccept-Encoding: gzip, deflate\r\nReferer: http://web-platform.test:8000/XMLHttpRequest/getallresponseheaders-cl.htm\r\nthis-is-a-test: 2\r\norigin: http://web-platform.test:8000\r\nConnection: keep-alive\r\n"

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Well, fetch api's result is "this-is-a-test: 2".
This looks good to me.
I guess this test case assertion is wrong.

To set
a name/value (name/value) pair in a header list (list), run these steps:
    If list contains name, then set the value of the first such header to value and remove the others.
    Otherwise, append a new header whose name is name and value is value to list.

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Oh I'm wrong. fetch constructor should call append() not set(). But somehow gecko only append() "this-is-a-test: 2". But at least the test case should compare case-insensitive name.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I'm a bit surprise it's because SetRequestHeader doesn't merge the value of "this-is-a-test", but replace the value.

aChannel->SetRequestHeader(headers[i].mName, headers[i].mValue,
                           false /* merge */);


http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/dom/fetch/FetchDriver.cpp#916-917

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Based on 
http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#168


     * @param aMerge
     *        If true, the new header value will be merged with any existing
     *        values for the specified header.

I guess we should use true, no?

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I think we should append.

To append
a name/value (name/value) pair to a header list (list), run these steps:

    If list contains name, then set name to the first such header’s name.

    This reuses the casing of the name of the header already in the header list, if any. If there are multiple matched headers their names will all be identical.

    Append a new header whose name is name and value is value to list.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1366945.patch

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1366945 - Compare a header name that is a byte-case-insensitive match

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1366945 - Patch 1: New header value will be merged with any existing values

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1366945 - Patch 2: Compare a header name that is a byte-case-insensitive match

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I will do more tests to avoid any regression.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1366945 - Patch 1: New header value will be merged with any existing values for HeadersInit

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Found some test failures:
fetch/api/basic/accept-header.any.html | Request through fetch should have 'accept' header with value 'custom/*'
assert_equals: Request has accept header with value 'custom/*' expected "custom/*" but got "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8, custom/*" 

/fetch/api/basic/accept-header.any.html | Request through fetch should have 'accept-language' header with value 'bzh'
assert_equals: Request has accept header with value 'bzh' expected "bzh" but got "en-US,en;q=0.5, bzh" 


fetch/api/basic/accept-header.any.worker.html | Request through fetch should have 'accept' header with value 'custom/*'
assert_equals: Request has accept header with value 'custom/*' expected "custom/*" but got "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8, custom/*" 


/fetch/api/basic/accept-header.any.worker.html | Request through fetch should have 'accept-language' header with value 'bzh'
assert_equals: Request has accept header with value 'bzh' expected "bzh" but got "en-US,en;q=0.5, bzh" 

service-workers/service-worker/fetch-header-visibility.https.html | Visibility of defaulted headers during interception
assert_unreached: unexpected rejection: custom accept FAIL - expected hmm got "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8, hmm" Reached unreachable code

Comment 13

[Anne (:annevk)]

Comment on attachment 8871677 [details] [diff] [review]
Bug 1366945 - Patch 2: Compare a header name that is a byte-case-insensitive match

Review of attachment 8871677 [details] [diff] [review]:
-----------------------------------------------------------------

This looks wrong to me. The case of request headers is to be preserved. Only response headers get lowercased.

Comment 14

[Anne (:annevk)]

I think the main problem with XMLHttpRequest/getallresponseheaders-cl.htm by the way is that we end up overwriting an existing header rather than appending to its value. See also https://github.com/whatwg/fetch/pull/484#issuecomment-334759826.

Comment 15

[Thomas Wisniewski [:twisniewski]]

Anne, is this still an issue? The test doesn't seem to exist in the same form, and aside from the www-authenticate stuff, I don't see any related tests failing.

Comment 16

[Anne (:annevk)]

The test was merged into another one per https://github.com/web-platform-tests/wpt/commit/b32391a796893529fa425c45b9e8b96d58d68440 and xhr/getallresponseheaders.htm does have a number of failures. We might track that separately though?

Comment 17

[Thomas Wisniewski [:twisniewski]]

Ah, ok.

I think those failures have to do with us not using sort-and-combine on the header names for getAllResponseHeaders (not byte-lowercasing the header names, and not combining with a comma, but rather with a newline).

I suppose we might as well just re-use this bug for fixing that.

Comment 18

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1366945 - fix our sort-and-combine behavior for xhr.getallresponseheaders to match the spec; r?baku

fix our sort-and-combine behavior for XHR.getAllResponseHeaders to match the spec

Comment 19

[Thomas Wisniewski [:twisniewski]]

Actually, we were already passing the tests to do with lowercasing, it was just that we weren't doing the sort-and-combine step properly.

This patch addresses that, and also makes sure we pass the content-length/type headers in sorted, lowercase order whenever we manually add those headers for non-HTTP channels.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0220dfa671561a7811fe2396ec9895ba9028251f

Comment 20

[Anne (:annevk)]

Note that the remaining issue in the 4th test with WWW-Authenticate likely can't be fixed here and is tracked by bug 1491010. (I'm somewhat surprised you didn't seem to run into it.)

Comment 21

[Thomas Wisniewski [:twisniewski]]

Comment on attachment 9013433 [details]
Bug 1366945 - fix our sort-and-combine behavior for xhr.getallresponseheaders to match the spec; r?baku

Ah, ok. I had not caught all of the detail there, and thought it was a related bug only about the Fetch side of things.

I'm not sure if that means we ought to close this as a dupe (since I figure it would be better to fix both Fetch and XHR at the same time).

Comment 22

[Anne (:annevk)]

Unless I'm missing something this wouldn't be a duplicate, since WWW-Authenticate parsing in a special manner is a subset of the failures.

Comment 23

[Thomas Wisniewski [:twisniewski]]

Well, the www-authenticate case is the only one of the four tests in that WPT file that's currently failing on nightly builds, ever since bug 1398718 was fixed.

Comment 24

[Anne (:annevk)]

Ah, my Nightly wasn't fresh enough. I agree that it would be better to have a centralized fix for this.