382947 - XMLHttpRequest should only set the charset in the Content-Type request header if we know the charset

Status: RESOLVED FIXED

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

Description

[Ryan Jones-Ward [:sciguyryan]]

We shouldn't make assumptions about the charset for the reason as described by bz in bug 362043 comment 12.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch attempt v1

Comment 2

[Wladimir Palant]

Ideally we would also check whether a charset header already exists. Having two charset parameters only because the caller "did things right" is not nice.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2

Same as patch v1 but also does as Wladimir suggested.

Comment 4

[Jesse Ruderman]

I'm confused.  How could we ever not know the charset of strings coming from JavaScript?

Comment 5

[Wladimir Palant]

To clarify. There are three options for the XMLHttpRequest.send parameter:

1. A plain string, AFAICT only the lower byte of the characters is interpreted in this case. This string could (and should) be the result of nsIScriptableUnicodeConverter.ConvertFromUnicode, consequently we don't know the charset.
2. A stream to be read from. Here the charset isn't known either.
3. A DOM document. That's the only case where we actually know the charset.

Comment 7

[Anne (:annevk)]

Note that for the 1 case the specification currently requires to encode as UTF-8. Not sure what case 2 is supposed to mean.

Comment 8

[Wladimir Palant]

How should one send a string in a different encoding then? I have one extension that communicates with the server in windows-1251, that will be problematic with this change.

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to comment #5)
> 1. A plain string, AFAICT only the lower byte of the characters is interpreted
> in this case. This string could (and should) be the result of
> nsIScriptableUnicodeConverter.ConvertFromUnicode, consequently we don't know
> the charset.

The code seems to be doing a conversion to UTF-8, are you saying that is not working?

(In reply to comment #7)
> Note that for the 1 case the specification currently requires to encode as
> UTF-8. Not sure what case 2 is supposed to mean.

I assume the charset parameter should be replaced in the Content-Type header set with setRequestHeader?

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #9)
> The code seems to be doing a conversion to UTF-8, are you saying that is not
> working?
> 

I think the point being argued here is that we just shouldn't set a charset parameter if we don't know the charset anyway... don't convert and just leave it alone as I understand it.

> (In reply to comment #7)
> I assume the charset parameter should be replaced in the Content-Type header
> set with setRequestHeader?
> 

We could do that too or just leave the header alone and change nothing - it was sent as being encoded in a specific charset so they probably want it encoded as such.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note that comment 5 is wrong.  If we have a string, it's an honest to God Unicode string (UTF-16) that we convert to UTF8 and send as far as XMLHttpRequest is concerned.  If someone is passing in the result of ConvertToUnicode, then it will in fact be this bizarro "byte-inflate stuff" thing that XPConnect does with the non-intl-safe ACString.  People really shouldn't do that.  Really.

The issue Peter is pointing out with the patch is that when sending a string we _do_ want to send the UTF-8 charset.  Furthermore, I'd think we always want to set (or rather reset) the charset param in the upload stream content type for strings and documents (since there we _do_ know what encoding we're sending, and it's UTF-8 or the document encoding respectively).  For streams, we should leave it as-is.

Comment 12

[Wladimir Palant]

Boris, I have an extension that has to communicate with the server in windows-1251. The only way I found to make this work was to pass XMLHttpRequest the result of ConvertToUnicode. And I absolutely agree with you that you shouldn't do that - but what *should* you do? Do I have to do bizarro "string byte-inflate to stream stuff" now?

Comment 13

[Boris Zbarsky [:bzbarsky]]

You basically got lucky, in that all the bytes you passed in coverted to a single byte in UTF-8.  If you happened to use something that didn't do that, you'd get garbage on the server end, because what XMLHttpRequest is sending when you do that is:

1)  Take the UTF-16
2)  Convert to windows-1251
3)  Insert a zero byte every other byte
4)  Treat the result as UTF-16 and convert to UTF8
5)  Send the result of that.

I'm frankly rather surprised that you didn't hit any problems; you must not have been sending many (any?) non-ASCII chars.

> Do I have to do bizarro "string byte-inflate to stream stuff" now?

No, just use convertToInputStream() instead of ConvertToUnicode() on the scriptable converter.  Then pass in that input stream.  And set your charset on the channel (as I assume you already do).

Comment 14

[Wladimir Palant]

(In reply to comment #13)
> I'm frankly rather surprised that you didn't hit any problems; you must not
> have been sending many (any?) non-ASCII chars.

Actually a lot, and this hack is already four years old (the method you suggest was added far later). No problems whatsoever as far as I know.

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 267011 [details] [diff] [review]
Patch v2

New patch coming up shortly.

Comment 16

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.0

Patch v3.0

Sets the charset data information for everything but streams.

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.1

Patch v3.1

Fixed a small error in the last patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 270717 [details] [diff] [review]
Patch v3.1

Sure, but I basically suggested this fix, so I still think peterv should look at it.

Comment 19

[Phil Ringnalda (:philor)]

content/base/src/nsXMLHttpRequest.cpp 1.187

Comment 20

[marko dudic]

This patch produce that upload of files don't work any more. Now boundary parameter is deleted from the header. Look at the bug https://bugzilla.mozilla.org/show_bug.cgi?id=393968