Last Comment Bug 819051 - XMLHttpRequest.setRequestHeader() overwrites instead of combines values for the same header.
: XMLHttpRequest.setRequestHeader() overwrites instead of combines values for t...
Status: RESOLVED FIXED
[mentor=bz][lang=c++]
: 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]
Mentors:
http://xhr.spec.whatwg.org/#the-setre...
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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();
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] 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: https://tbpl.mozilla.org/?tree=Try&rev=b08b1629020e).
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 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 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 (https://tbpl.mozilla.org/?tree=Try&rev=59613714b24b).
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-02-04 04:18:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a65fe91d5
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-02-04 16:37:31 PST
https://hg.mozilla.org/mozilla-central/rev/a14a65fe91d5
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.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21
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 https://github.com/w3c/web-platform-tests/pulls/Ms2ger - 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 https://github.com/w3c/web-platform-tests/pull/239 (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 https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#setRequestHeader%28%29

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.