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

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Benjamin Smedberg]

How does this affect web authors?

Comment 3

[Boris Zbarsky [:bzbarsky]]

It doesn't, but it affects extension authors or any other UniversalXPConnect
code that's using XMLHttpRequest.

Comment 4

[Boris Zbarsky [:bzbarsky]]

When we fix this, we should make sure not to cause bug 308484 on the 1.8 branch.

Comment 5

[Darin Fisher]

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.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

at least nsIXMLHttpRequest.idl needs to be fixed, which says the headers need to
be specified in the stream.

Comment 7

[Darin Fisher]

Attachment: v1 patch: fix comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Christian :Biesinger (don't email me, ping me on IRC)]

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 10

[Johnny Stenback (:jst)]

Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

sr=jst

Comment 11

[Darin Fisher]

> 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 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196262 [details] [diff] [review]
v1 patch: fix comments

r=biesi with those comment changes.

Comment 13

[Darin Fisher]

Attachment: 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.

Comment 14

[Darin Fisher]

marking FIXED

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[Mike Schroepfer]

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?

Comment 17

[Darin Fisher]

fixed1.8

Comment 18

[Darin Fisher]

(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