730925 - XHR.getAllResponseHeaders should use CRLF, not LF per spec

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We seem to separate headers with just LFs. (Both in nsHeaderVisitor::VisitHeader and nsXMLHttpRequest::GetAllResponseHeaders)

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Original TRY version of the patch didn't contain change to mochitest: content/base/test/test_XHR.html
https://tbpl.mozilla.org/?tree=Try&rev=67ce3b709cab

Controlled TRY test of current version with corrected regex:
https://tbpl.mozilla.org/?tree=Try&rev=72c2a8e36a4e

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 645225 [details] [diff] [review]
Patch (v1)

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

I guess this is fine, but I don't understand the regex at all :)

Can you verify we pass <http://w3c-test.org/webapps/XMLHttpRequest/tests/submissions/Opera/getallresponseheaders-cookies.htm> with this patch?

Comment 3

[Mark Capella [:capella]]

Regex is new to me also, I learn just enough to make required patches.

For the linked test, without patch we get:
assert_true: Headers should include a CRLF pair. expected true got false

with the patch it still fails, but now for a different reason / later test:
assert_true: Headers should include the custom header. expected true got false

Comment 4

[Mark Capella [:capella]]

Nice. With your changes, and this patch, the test page "custom header" error went away:
Pass	XMLHttpRequest: getAllResponseHeaders() excludes cookies

Comment 5

[Mark Capella [:capella]]

Comment on attachment 645225 [details] [diff] [review]
Patch (v1)

Review request from Module Owner...

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 645225 [details] [diff] [review]
Patch (v1)

r=jst assuming Jonas agrees with the spec here. Jonas, any reason not to take this change?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 645225 [details] [diff] [review]
Patch (v1)

As long as this is the behavior defined by spec

Comment 8

[Mark Capella [:capella]]

Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=a4e3236387dc

Comment 9

[Mark Capella [:capella]]

On to Inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e4330f6365d9

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e4330f6365d9

Comment 11

[Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)]

XHR doc updated - https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#getAllResponseHeaders%28%29