Closed
Bug 819051
Opened 9 years ago
Closed 8 years ago
XMLHttpRequest.setRequestHeader() overwrites instead of combines values for the same header.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: jeff.vanvoorst, Assigned: singerb.dev)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete, Whiteboard: [mentor=bz][lang=c++])
Attachments
(1 file, 1 obsolete file)
9.05 KB,
patch
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Same for the XHR Living Standard.
Updated•9 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
![]() |
||
Comment 2•9 years ago
|
||
> 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".)
Yup, sounds about right. And write a test :)
![]() |
||
Updated•9 years ago
|
Whiteboard: [mentor=bz]
Updated•9 years ago
|
Whiteboard: [mentor=bz] → [mentor=bz][lang=c++]
Assignee | ||
Comment 4•8 years ago
|
||
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).
Assignee: nobody → singerb.dev
Attachment #709327 -
Flags: review?(bzbarsky)
![]() |
||
Comment 5•8 years ago
|
||
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.
Attachment #709327 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 6•8 years ago
|
||
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).
Attachment #709327 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14a65fe91d5
Flags: in-testsuite+
Keywords: checkin-needed
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a14a65fe91d5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 9•8 years ago
|
||
Benedict, do you mind if I submit your test to the spec's test suite?
Assignee | ||
Comment 10•8 years ago
|
||
No, that's fine, go ahead.
Comment 11•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
Ms2ger: submitted at https://github.com/w3c/web-platform-tests/pull/239 (somewhat rewritten) - merge?
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 15•8 years ago
|
||
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•8 years ago
|
||
Oh... and I missed kohei comment too. My mistake.
Keywords: dev-doc-needed → dev-doc-complete
Comment 17•8 years ago
|
||
David: no problem, better a double check than none :-)
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•