Failure in XMLHttpRequest/getallresponseheaders-cl.htm

NEW
Assigned to

Status

()

Core
DOM
P3
normal
5 months ago
13 days ago

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 months ago
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"
(Assignee)

Updated

5 months ago
Assignee: nobody → shuang
(Assignee)

Comment 1

5 months ago
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.
(Assignee)

Comment 2

5 months ago
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.
(Assignee)

Comment 3

5 months ago
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
(Assignee)

Comment 4

5 months ago
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?
(Assignee)

Comment 5

5 months ago
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.
(Assignee)

Comment 6

5 months ago
Created attachment 8871674 [details] [diff] [review]
bug-1366945.patch
(Assignee)

Comment 7

5 months ago
Created attachment 8871675 [details] [diff] [review]
Bug 1366945 - Compare a header name that is a byte-case-insensitive match
(Assignee)

Updated

5 months ago
Attachment #8871675 - Attachment description: bug-1366945-p2.patch → Bug 1366945 - Compare a header name that is a byte-case-insensitive match
(Assignee)

Updated

5 months ago
Attachment #8871674 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #8871675 - Attachment is obsolete: true
(Assignee)

Comment 8

5 months ago
Created attachment 8871676 [details] [diff] [review]
Bug 1366945 - Patch 1: New header value will be merged with any existing values
(Assignee)

Comment 9

5 months ago
Created attachment 8871677 [details] [diff] [review]
Bug 1366945 - Patch 2: Compare a header name that is a byte-case-insensitive match
(Assignee)

Updated

5 months ago
Attachment #8871677 - Flags: review?(annevk)
(Assignee)

Comment 10

5 months ago
I will do more tests to avoid any regression.
(Assignee)

Updated

5 months ago
Attachment #8871676 - Attachment is obsolete: true
(Assignee)

Comment 11

5 months ago
Created attachment 8871686 [details] [diff] [review]
Bug 1366945 - Patch 1: New header value will be merged with any existing values for HeadersInit
(Assignee)

Updated

5 months ago
Attachment #8871686 - Attachment description: 提交摘要: Bug 1366945 - Patch 1: New header value will be merged with any existing values for HeadersInit → Bug 1366945 - Patch 1: New header value will be merged with any existing values for HeadersInit
(Assignee)

Comment 12

5 months ago
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

4 months ago
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.
Attachment #8871677 - Flags: review?(annevk) → review-
Priority: -- → P3

Updated

26 days ago
Blocks: 726433

Comment 14

13 days ago
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.
You need to log in before you can comment on or make changes to this bug.