document the fact that XMLHttpRequest::Send doesn't support streams with headers

RESOLVED FIXED

Status

()

Core
XML
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Biesinger, Assigned: Darin Fisher)

Tracking

({fixed1.8, regression})

Trunk
fixed1.8, regression
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

XMLHttpRequest always passes a content type to SetUploadStream, which means that
any possibly existing headers in the input stream will be sent as data.

In 1.7.x, that was different:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/xmlextras/base/src/nsXMLHttpRequest.cpp&rev=MOZILLA_1_7_BRANCH&cvsroot=/cvsroot&mark=1487#1483

We should maybe still allow streams with headers.
Keywords: regression
OS: Linux → All
Hardware: PC → All
Either that or fix the documentation accordingly...

In any case, we need to do one or the other for 1.8, imo.

Darin, bsmedberg, thoughts?
Flags: blocking1.8b5?
How does this affect web authors?
It doesn't, but it affects extension authors or any other UniversalXPConnect
code that's using XMLHttpRequest.
When we fix this, we should make sure not to cause bug 308484 on the 1.8 branch.
Depends on: 308484
(Assignee)

Comment 5

12 years ago
Why should we fix this?  Do we know of many extensions that try to set headers
at the top of their data stream?  If possible, we should drop support for
headers in the data stream.  It is a messy way of specifying headers.

It looks like the change was introduced by the patch for bug 246651.
at least nsIXMLHttpRequest.idl needs to be fixed, which says the headers need to
be specified in the stream.
(Assignee)

Comment 7

12 years ago
Created attachment 196262 [details] [diff] [review]
v1 patch: fix comments
Attachment #196262 - Flags: superreview?(jst)
Attachment #196262 - Flags: review?(cbiesinger)
I'd be fine with us changing the IDL appropriately, sure.  As long as our IDL
and impl are consistent and we have _a_ way to set headers, I'm happy not to do
it in the stream (which I agree is a messy way).
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

+   *		  compatible with nsIUploadChannel.setUploadStream, and a

I think it should be made clearer that this function expects no headers in the
stream - nsIUploadChannel.setUploadStream can behave either way.

should it be mentioned that a content-type for the uploaded data can be set
using setRequestHeader, or is that obvious?
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

sr=jst
Attachment #196262 - Flags: superreview?(jst) → superreview+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 11

12 years ago
> I think it should be made clearer that this function expects no headers in the
> stream - nsIUploadChannel.setUploadStream can behave either way.
> 
> should it be mentioned that a content-type for the uploaded data can be set
> using setRequestHeader, or is that obvious?

good points.  i'll extend the comments to mention those.
Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

r=biesi with those comment changes.
Attachment #196262 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 13

12 years ago
Created attachment 196381 [details] [diff] [review]
v1.1 patch

OK, here's the version that I checked in.  I would like to land this on the 1.8
branch for consistency.
Assignee: xml → darin
Attachment #196262 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #196381 - Flags: approval1.8b5?
(Assignee)

Comment 14

12 years ago
marking FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: XMLHttpRequest::Send doesn't support streams with headers → document the fact that XMLHttpRequest::Send doesn't support streams with headers
Could we add a quick blurb about this change linked off
http://developer.mozilla.org/docs/Adapting_XUL_Applications_for_Firefox_1.5 ?

Comment 16

12 years ago
Comment on attachment 196381 [details] [diff] [review]
v1.1 patch

Can you write a blurb for an update to devmo and send to cbeard/deb?
Attachment #196381 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 17

12 years ago
fixed1.8
Keywords: fixed1.8
(Assignee)

Comment 18

12 years ago
(In reply to comment #15)
> Could we add a quick blurb about this change linked off
> http://developer.mozilla.org/docs/Adapting_XUL_Applications_for_Firefox_1.5 ?

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