Last Comment Bug 819051 - XMLHttpRequest.setRequestHeader() overwrites instead of combines values for the same header.
: XMLHttpRequest.setRequestHeader() overwrites instead of combines values for t...
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 17 Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla21
Assigned To: Benedict Singer
: Andrew Overholt [:overholt]
Depends on:
Blocks: 726433
  Show dependency treegraph
Reported: 2012-12-06 12:43 PST by jeff.vanvoorst
Modified: 2013-09-14 16:43 PDT (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch + test case. (7.50 KB, patch)
2013-02-01 18:56 PST, Benedict Singer
bzbarsky: review+
Details | Diff | Splinter Review
Final patch. (9.05 KB, patch)
2013-02-03 16:57 PST, Benedict Singer
no flags Details | Diff | Splinter Review

Description jeff.vanvoorst 2012-12-06 12:43:30 PST
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();"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 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] 2012-12-06 18:36:58 PST
Same for the XHR Living Standard.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-12-06 19:13:34 PST
> 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 2012-12-06 21:33:54 PST
Yup, sounds about right. And write a test :)
Comment 4 Benedict Singer 2013-02-01 18:56:36 PST
Created attachment 709327 [details] [diff] [review]
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:
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2013-02-01 19:45:16 PST
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 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 2013-02-03 16:57:39 PST
Created attachment 709539 [details] [diff] [review]
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 (
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-02-04 04:18:34 PST
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-02-04 16:37:31 PST
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2013-02-05 05:00:09 PST
Benedict, do you mind if I submit your test to the spec's test suite?
Comment 10 Benedict Singer 2013-02-05 11:21:18 PST
No, that's fine, go ahead.
Comment 11 Kohei Yoshino [:kohei] 2013-02-23 23:29:55 PST
I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
Comment 12 Hallvord R. M. Steen 2013-07-07 13:26:41 PDT
(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 - did you submit it?
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2013-07-07 13:28:58 PDT
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 2013-07-08 02:28:20 PDT
Ms2ger: submitted at (somewhat rewritten) - merge?
Comment 15 Jean-Yves Perrier [:teoli] 2013-09-14 16:32:36 PDT
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

Do you think it needs more docs?
Comment 16 David Bruant 2013-09-14 16:39:42 PDT
Oh... and I missed kohei comment too. My mistake.
Comment 17 Jean-Yves Perrier [:teoli] 2013-09-14 16:43:33 PDT
David: no problem, better a double check than none :-)

Note You need to log in before you can comment on or make changes to this bug.