Bug 11049 – XMLHttpRequest always defaults Content-Type to application/xml, while it should depend on data type

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Alexey Proskuryakov]

In bug 3565, we changed the default XMLHttpRequest Content-Type from "application/x-www-form-urlencoded" to "application/xml", to better match Firefox.

The current draft of XHR spec describes somewhat different behavior - most notably, Content-Type shouldn't be automatically set for non-Document data at all.

"If the argument to send() is a Document and no Content-Type header has been set user agents must set it to application/xml for XML documents and to the most appropriate media type for other documents (using intrinsic knowledge about the document)."

[Anne van Kesteren]

Since it already needs to be serialized as XML I dropped the bit about other document types for the moment.

[Carol Szabo]

A more recent version of the specification http://www.w3.org/TR/XMLHttpRequest/#the-send-method specifies that the default Content-Type should be text/plain if the send method is called with a string. Which makes sense since there is no information about the content of the string.
Proposed Patch coming soon.

[Alexey Proskuryakov]

Sounds good.

I'd like to see results of comparison to other major browsers though - although the current spec test has been extensively discussed, I'm not sure how well it matches behavior of shipping browsers. Knowing that will help foresee possible compatibility problems.

[Carol Szabo]

Compatibility survey:
According to my tests:
Mozilla uses (like the current version of WebKit): application/xml
IE8: does not set the header at all.

To me neither is quite correct, but I'd pick IE over Mozilla since the content of the string may not necessarily be XML. On the other hand we could go with what the spec says.
To me if the client does not set a content-type, probably the server doesn't care either about the content-type, so there shouldn't be compatibility issues, but one never knows.
Maybe Anne is wiling to change his mind, and alter the standard to say that since the name of the object is XMLHttpRequest, it should probably assume that by default it sends XML unless otherwise specified.
This way I can make my testers happy too.

[Alexey Proskuryakov]

> Mozilla uses (like the current version of WebKit): application/xml
> IE8: does not set the header at all.
> 
> To me neither is quite correct, but I'd pick IE over Mozilla since the content
> of the string may not necessarily be XML.

That's actually a violation of RFC2616: "Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field defining the media type of that body."

Based on the above data, I'd say that we should probably preserve our behavior for now, and consult with Mozilla on when/if they are going to change Gecko behavior.

[Carol Szabo]

I acknowledged that IE is not correct but it is a violation of RFC2616 to put a content type that defines a media type and then provide a body of different media type. text/plain for a string is a safe choice in my opinion and I think that this is why Anne chose that for the spec.
The spec can be changed, though, to specify application/xml and to require the user to set the content type if the string contains something else then xml, as I said, the name of the object is XMLHttpRequest, not GenericHttpRequest.

(In reply to comment #5)
> > Mozilla uses (like the current version of WebKit): application/xml
> > IE8: does not set the header at all.
> > 
> > To me neither is quite correct, but I'd pick IE over Mozilla since the content
> > of the string may not necessarily be XML.
> 
> That's actually a violation of RFC2616: "Any HTTP/1.1 message containing an
> entity-body SHOULD include a Content-Type header field defining the media type
> of that body."
> 
> Based on the above data, I'd say that we should probably preserve our behavior
> for now, and consult with Mozilla on when/if they are going to change Gecko
> behavior.

[Anne van Kesteren]

I rather not change the spec. We will add a FormData object soon which will make it default to multipart/form-data. Jonas (Mozilla) told me he would fix Mozilla XMLHttpRequest bugs so maybe you could file a bug on them?

[Carol Szabo]

Created attachment 42613 [details]
Proposed patch

According to Anne's comment above, the standard isn't likely to change in the regards that affect this bug, also, apparently, there is commitment from Firefox to change their behavior to match the current version of the standard. IE8 will probably converge too as their current behavior is in clear violation of HTTP standard.
Since the risk of breaking anything through this change is low I suggest that we go forwaard and match the standard now.

[Alexey Proskuryakov]

I'm not sufficiently opposed to this to mark the patch r-, but changing long-standing behavior without a good reason is not great. We don't even know if Mozilla has a bug filed about this, let alone what their timeframe for fixing would be.

[Darin Adler]

(In reply to comment #8)
> Since the risk of breaking anything through this change is low

How did you determine that?

[Carol Szabo]

(In reply to comment #10)
> (In reply to comment #8)
> > Since the risk of breaking anything through this change is low
> 
> How did you determine that?

Since IE does not send back any Content-Type and most websites work with IE, I assume that all those websites do not care about the Content-Type, they just assume the content is formatted as they expect it.
Furthermore, if somebody was careless enough to not set a content type on their post it sounds improbable to me that they would be careful to check if the content-arrives with the right type at the other end.
Since cross-site XHR is experimental, I assumed that the same team if not person designed both the JS sending the XHR and the code receiving it.
Pretty much this is my reasoning and I am aware that it is not bullet proof, but I weigh heavily conformance to standards, versus maintaining legacy dubious behavior after the standard is relatively solid.
If both IE and Mozilla had the same behavior, I would have agreed that there is a de facto standard whose legacy we should be mindful of, but since handling has not been consistent in the past, and the standard seems to be sensible I would suggest that we move on and support it.

[Anne van Kesteren]

Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=527123

[Eric Seidel (no email)]

Comment on attachment 42613 [details]
Proposed patch

The test could be made much better using shouldBe.  Or at least something which checks the expected value against the expected instead of requiring visual inspection.

Is this covered by any spec?  if so, that should be listed in the Changelog.  Likewise the ChangeLog should mention if this brings us closer or further from FF/IE's behavior.

[Eric Seidel (no email)]

Sorry, I clearly didn't read enough of the bug above before my review.  My comments still stand though: for easy review, the ChangeLog should mention the resolution of this bug, including possible specs and other browser behaviors.  Basically, ask yourself what you'd want to see when reviewing this, or when trying to figure out why the hell we'd made this change 5 years from now.  That information should all be in the ChangeLog for easy review and understanding 5 years from now. :)

[Carol Szabo]

Created attachment 45336 [details]
Proposed Patch

Addressed Eric's concerns about the test case and the ChangeLog.
There is no consensus in browser behavior. Mozilla was said to intend to align to the standard. IE has the unacceptable behavior of not sending a content type at all, which is non-conforming with HTTP.
Here is a link to the latest version of the XMR standard: http://dev.w3.org/2006/webapi/XMLHttpRequest/#the-send-method

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45336 [details] without any errors.

[Alexey Proskuryakov]

Comment on attachment 45336 [details]
Proposed Patch

I'm against changing our behavior to something that matches no other browser. We can revisit this once Mozilla takes some action with the bug Anne filed.

The fact that IE and Firefox behave differently gives some hope for servers not depending on either behavior, but it is also quite probable that client-side JS has different code paths for different browsers, adjusting Content-Type as necessary.

[Carol Szabo]

(In reply to comment #17)
> (From update of attachment 45336 [details])
> I'm against changing our behavior to something that matches no other browser.
> We can revisit this once Mozilla takes some action with the bug Anne filed.
> 
> The fact that IE and Firefox behave differently gives some hope for servers not
> depending on either behavior, but it is also quite probable that client-side JS
> has different code paths for different browsers, adjusting Content-Type as
> necessary.

This bug only applies to cases when JS writers do not actually care to set the content type, hence they do not have different paths for Mozilla and IE in this regard.
Yes, you are right that probably servers do not care, or otherwise IE or Mozilla would not work, but wouldn't it be nice for WebKit to set a good example and be the first to do the reasonable thing.

If somebody in power (i.e. reviewer) is staunchly opposed to doing anything on this issue, which is indeed of minute importance, maybe it would be productive to close the bug as won't fix, so that test developers know what to expect, and others stop wasting time trying to address it, maybe Anne can be convinced to change the standard by vigurous opposition to actually implementing it, which I personally would not like, but it seems better than the status quo, where we have a standard to which the major players do not adhere and there is no movement toward it either.

[Darin Adler]

(In reply to comment #18)
> wouldn't it be nice for WebKit to set a good
> example and be the first to do the reasonable thing.

The tradeoff is against breaking something that currently works. If we had evidence that making this change would not have an adverse effect on existing websites or other uses of WebKit, then it would be an easy decision. We could just switch to "the reasonable thing" and set a good example. Although even then we could fall prey to the problem where we implement something in a standard, and then the standard later changes. That's happened to use on the WebKit project before.

Since there is a real chance this change could affect websites, we may need to be a bit more cautious.

[Alexey Proskuryakov]

*** Bug 34654 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 46149 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Test: http://tc.labs.opera.com/apis/XMLHttpRequest/send-content-type-string.htm

[Alexey Proskuryakov]

*** Bug 99690 has been marked as a duplicate of this bug. ***

[Alexander Shalamov]

Created attachment 169849 [details]
Patch 1

Moving proposed patch from https://bugs.webkit.org/show_bug.cgi?id=99690
- Removed ::send(String) and ::send(Blob) related fixes from original patch, should be fixed separately.
- Rebased to master.

[Alexey Proskuryakov]

Comment on attachment 169849 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=169849&action=review

I think that it would be better to have a separate bug for this, too, and either keep this one as a meta, or just close it.

This needs a study of other browsers' behavior, and some discussion about why servers and proxies are likely to accept content type strings like these.

Charset in particular seems fairly dangerous and useless, because UTF-8 is the default anyway.

> LayoutTests/http/tests/xmlhttprequest/post-document-content-type-expected.txt:8
> +PASS Content-type: application/xml;charset=encoding
> +PASS Content-type: text/html;charset=encoding
> +PASS Content-type: application/xml;charset=encoding

Ideally, a script test like this makes it clear what happens in a subtest. Here, there is no indication why the results are different, yet all are a PASS. Also, it's unclear why an unknown encoding name "encoding" is a PASS.

[Alexander Shalamov]

(In reply to comment #25)
> I think that it would be better to have a separate bug for this, too, and either keep this one as a meta, or just close it.

Ok, I will create separate bug.

> This needs a study of other browsers' behavior, and some discussion about why servers and proxies are likely to accept content type strings like these.

I executed same tests in Opera and Mozilla and they have the same output.

> Ideally, a script test like this makes it clear what happens in a subtest. Here, there is no indication why the results are different, yet all are a PASS. Also, it's unclear why an unknown encoding name "encoding" is a PASS.

I was thinking that we could have different encodings on bots (for document).
If not, I could output actual encoding that is returned from the document.

[Darin Adler]

Comment on attachment 169849 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=169849&action=review

Thanks for tackling this. Looks good. I have a few suggestions for improvement.

> Source/WebCore/xml/XMLHttpRequest.cpp:562
> +        String documentMIMEType = document->isHTMLDocument() ? "text/html" : "application/xml";

There is no need to use a String just to hold one or two different ASCII literals. Should just use const char* instead.

Also seems unnecessary to do this out here since it’s only used if contentType is empty. Should move this code closer to where it’s used.

> Source/WebCore/xml/XMLHttpRequest.cpp:592
> +        if (!encoding.isValid() || encoding == UTF16BigEndianEncoding() || encoding == UTF16LittleEndianEncoding())

The function TextEncoding::closestByteBasedEquivalent() takes care of mapping UTF-16 to UTF-8, and that is what we should use here instead of specifically special casing UTF-16.

[youenn fablet]

The current XHR spec has a simpler charset handling: UTF-8 should be used in all cases.
It still mandates using text/html for HTMLDocument and application/xml otherwise, which would be good to have.

[Darin Adler]

Comment on attachment 169849 [details]
Patch 1

Clearing the review flag on this because it seems we will want to do something different given the change in the XHR spec.

[youenn fablet]

Created attachment 237486 [details]
Patch

[youenn fablet]

(In reply to comment #30)
> Created an attachment (id=237486) [details]
> Patch

This patch updates default Content-Type for string to text/plain;charset=UTF-8
This aligns with http://w3c-test.org/XMLHttpRequest/send-content-type-charset.htm, Firefox and Blink.

This patch also changes HTML document default content-Type to text/html;charset=UTF-8.
This aligns with the spec. From a quick check, Blink and Firefox are still using application/xml as mime type for HTML documents.

[Build Bot]

Comment on attachment 237486 [details]
Patch

Attachment 237486 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5581839196487680

New failing tests:
http/tests/xmlhttprequest/workers/shared-worker-methods-async.html
http/tests/xmlhttprequest/methods.html
http/tests/xmlhttprequest/workers/shared-worker-methods.html
http/tests/xmlhttprequest/workers/methods-async.html
http/tests/xmlhttprequest/methods-async.html
http/tests/xmlhttprequest/workers/methods.html

[Build Bot]

Created attachment 237488 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 237486 [details]
Patch

Attachment 237486 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5118068627865600

New failing tests:
http/tests/xmlhttprequest/workers/shared-worker-methods-async.html
http/tests/xmlhttprequest/methods.html
http/tests/xmlhttprequest/workers/shared-worker-methods.html
http/tests/xmlhttprequest/workers/methods-async.html
http/tests/xmlhttprequest/methods-async.html
http/tests/xmlhttprequest/workers/methods.html

[Build Bot]

Created attachment 237497 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 237486 [details]
Patch

Attachment 237486 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5697537562378240

New failing tests:
http/tests/xmlhttprequest/workers/shared-worker-methods-async.html
http/tests/xmlhttprequest/methods.html
http/tests/xmlhttprequest/workers/shared-worker-methods.html
http/tests/xmlhttprequest/workers/methods-async.html
http/tests/xmlhttprequest/methods-async.html
http/tests/xmlhttprequest/workers/methods.html

[Build Bot]

Created attachment 237502 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[youenn fablet]

Created attachment 237509 [details]
Adding new baselines for mac and efl

[Darin Adler]

Comment on attachment 237509 [details]
Adding new baselines for mac and efl

View in context: https://bugs.webkit.org/attachment.cgi?id=237509&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:596
> +                setRequestHeaderInternal("Content-Type", document->isHTMLDocument() ? "text/html;charset=UTF-8":"application/xml;charset=UTF-8");

Wrong formatting here. There should be spaces around the colon.

All the string literals that are being passed as String should have ASCIILiteral around them, to eliminate unnecessary memory allocation.

[WebKit Commit Bot]

Comment on attachment 237509 [details]
Adding new baselines for mac and efl

Clearing flags on attachment: 237509

Committed r173254: <http://trac.webkit.org/changeset/173254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 99973 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 125889 has been marked as a duplicate of this bug. ***