819051 - XMLHttpRequest.setRequestHeader() overwrites instead of combines values for the same header.

Status: RESOLVED FIXED

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

Description

[jeff.vanvoorst]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Steps to reproduce:

I am using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:17.0) Gecko/20100101 Firefox/17.0 (FireFox 17.0.1).

Open javascript console, and set a non-standard HTTP request header:

xhr = new XMLHttpRequest();
xhr.open("POST", url);
xhr.setRequestHeader("X-appended-to-this", "True");
xhr.setRequestHeader("X-appended-to-this", "False");

.... Do rest of xhr


Actual results:

Open the firebug tool to inspect the XHR request.  The header "X-appended-to-this" was sent with the value "False".  Of course, the server also received "X-appended-to-this" with the value "False".


Expected results:

I expected (based on http://www.w3.org/TR/XMLHttpRequest/#request-method) that the header "X-appended-to-this" would be sent with the value "True; False".

I don't know if there is a good reason for not following the W3C draft.

Comment 1

[Masatoshi Kimura [:emk]]

Same for the XHR Living Standard.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I don't know if there is a good reason for not following the W3C draft.

Probably that it wasn't implemented that way initially back before there was ever a draft.

Jonas, just flipping the last arg of nsIHttpChannel::SetRequestHeader would do the right thing here, right?  (Though note the value would be "True, False".)

Comment 3

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

Yup, sounds about right. And write a test :)

Comment 4

[Benedict Singer]

Attachment: Patch + test case.

This turns out to be not quite so simple as just changing a boolean, due to a) privileged callers, and b) overwriting default headers, but with some IRC discussion we think we've sorted it out. A new test case is included, and the old tests should all be passing (here's a good Try run, minus some known oranges: https://tbpl.mozilla.org/?tree=Try&rev=b08b1629020e).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 709327 [details] [diff] [review]
Patch + test case.

One other thing that needs to be done: mAlreadySetHeaders needs to be cleared in Open().  See http://www.w3.org/TR/XMLHttpRequest2/#the-open-method step 18

Please add a test for that?  Basicall call open(), set a header, call open() again, set the header again, then check that this overrode instead of merging.  You'll probably need to test with a default-set header like Accept or something, since the second open() will wipe out the HTTP channel.

r=me with that fixed and the test added.

Comment 6

[Benedict Singer]

Attachment: Final patch.

Indeed, without that someone setting 'Accept' would get it merged after a second open() call. Added a testcase to the new test for that, and fixed the issue. Nice green run (https://tbpl.mozilla.org/?tree=Try&rev=59613714b24b).

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a65fe91d5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

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

Benedict, do you mind if I submit your test to the spec's test suite?

Comment 10

[Benedict Singer]

No, that's fine, go ahead.

Comment 11

[Kohei Yoshino]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 12

[Hallvord R. M. Steen]

(In reply to :Ms2ger from comment #9)
> Benedict, do you mind if I submit your test to the spec's test suite?

Ms2ger, couldn't find anything that seemed relevant at https://github.com/w3c/web-platform-tests/pulls/Ms2ger - did you submit it?

Comment 13

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

No, never got around to it, I think. Feel free to do it yourself, or else I can do it at some point.

Comment 14

[Hallvord R. M. Steen]

Ms2ger: submitted at https://github.com/w3c/web-platform-tests/pull/239 (somewhat rewritten) - merge?

Comment 15

[Jean-Yves Perrier [:teoli]]

David: Last month, I've added "If this method is called several times with the same header, the values are merged into one single request header." to https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#setRequestHeader%28%29

Do you think it needs more docs?

Comment 16

[David Bruant]

Oh... and I missed kohei comment too. My mistake.

Comment 17

[Jean-Yves Perrier [:teoli]]

David: no problem, better a double check than none :-)

Comment 18

[caldasjdaniel]

This behavior is still the same for Firefox 92.0, while Chrome 93.0 (for instance) does not append headers for this API. Is this still something that the team is considering fixing?