Closed Bug 819051 Opened 12 years ago Closed 11 years ago

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

Categories

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

17 Branch
x86
macOS
defect
Not set
normal

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)

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.
Same for the XHR Living Standard.
Blocks: xhr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → DOM
Product: Firefox → Core
> 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 :)
Whiteboard: [mentor=bz]
Whiteboard: [mentor=bz] → [mentor=bz][lang=c++]
Attached patch Patch + test case. (obsolete) — Splinter Review
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 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+
Attached patch Final patch.Splinter Review
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a14a65fe91d5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Benedict, do you mind if I submit your test to the spec's test suite?
No, that's fine, go ahead.
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
(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?
No, never got around to it, I think. Feel free to do it yourself, or else I can do it at some point.
Ms2ger: submitted at https://github.com/w3c/web-platform-tests/pull/239 (somewhat rewritten) - merge?
Keywords: dev-doc-needed
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?
Oh... and I missed kohei comment too. My mistake.
David: no problem, better a double check than none :-)
Component: DOM → DOM: Core & HTML

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?

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

Attachment

General

Created:
Updated:
Size: