Bug 147784 – XHR should not combine empty content-type value with default one

• Component: XML
• Status: RESOLVED FIXED

[youenn fablet]

As illustrated in https://bugs.webkit.org/show_bug.cgi?id=147445, setting an empty Content-Type through setRequestHeader is leading to the generation of a bad header ,XXXX, XXXX being the default mime-type when none is set.
XHR should not try to combine these two.

[youenn fablet]

Created attachment 258500 [details]
Patch

[youenn fablet]

(In reply to comment #0)
> As illustrated in https://bugs.webkit.org/show_bug.cgi?id=147445, setting an
> empty Content-Type through setRequestHeader is leading to the generation of
> a bad header ,XXXX, XXXX being the default mime-type when none is set.
> XHR should not try to combine these two.

To be clear
xhr.setRequestHeader("Content-Type", "");
xhr.send("");
will end up in generating a header: "Content-Type: ,text/plain", text/plain being the mime type used when none is provided.
The correct behaviour is to send should be "Content-Type: ", as per the XHR spec.
Firefox and Chrome follow that pattern.

[Alexey Proskuryakov]

Comment on attachment 258500 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:600
> +        if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {

It looks strange to special case Content-Type like this. There are many headers fields that do not allow multiple values.

I think that unless we can implement it correctly for some well specified set of header fields, it should be author's responsibility to use the API correctly. What do other browsers do?

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 258500 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258500&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:600
> > +        if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {
> 
> It looks strange to special case Content-Type like this. There are many
> headers fields that do not allow multiple values.

Content-Type is not special cased for multiple values. One can set twice 'Content-Type' with setRequestHeader and the value will be combined.

WebKit is failing to detect when the web app provides a 'Content-Type' value in the case of the empty value. WebKit then tries to set the 'Content-Type' to the default value as per XHR spec, and it gets combined.

This is clearly broken.
It seems to me there are 3 options:
1. Send an empty 'Content-Type', as requested by the web app.
2. Raise an exception when trying to set an empty value for 'Content-Type'
3. Consider that the web app did not set any value and send 'Content-Type' with a default value


1 makes sense to me.
2 is not consistent with other headers that might be set meaningfully to the empty value. It is also not consistent with existing browsers that do not throw currenly in that case.
3 does not make a lot of sense to me since we are not respecting what the web author wants.

> I think that unless we can implement it correctly for some well specified
> set of header fields, it should be author's responsibility to use the API
> correctly. What do other browsers do?

I cannot check the test currently.
Looking at the source code, it seems that:
- Chrome does the same as WebKit
- Firefox is using the default value (option 3).

I will write an update at https://github.com/w3c/web-platform-tests/pull/2045

[youenn fablet]

> I will write an update at https://github.com/w3c/web-platform-tests/pull/2045

If I am not misunderstanding Annev comments at https://github.com/w3c/web-platform-tests/pull/2045, the current patch is aligning WebKit with the spec.

[Alexey Proskuryakov]

Sending an empty Content-Type (or removing it altogether) shouldn't be supported functionality of XMLHttpRequest. It doesn't matter to me how exactly setRequestHeader("") or setRequestHeader(" ") fails.

[youenn fablet]

(In reply to comment #6)
> Sending an empty Content-Type (or removing it altogether) shouldn't be
> supported functionality of XMLHttpRequest. It doesn't matter to me how
> exactly setRequestHeader("") or setRequestHeader(" ") fails.

I do not get the direction you are pushing for.

Do you want to special-case the headers that cannot have an empty value (Content-Type, Range...) so that setRequestHeader throws an exception for those headers?

Do you want to disable empty values for all headers, including headers that are defined to accept empty values?

Do you prefer the status quo (sending a garbaged not-interoperable value)?

In any case, there is an on-going effort to fix those small holes within the XHR spec. It would be great to express your concerns so that the spec and related test suite reflect those.

[youenn fablet]

Created attachment 265384 [details]
Using wpt tests

[Darin Adler]

Comment on attachment 265384 [details]
Using wpt tests

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

> Source/WebCore/xml/XMLHttpRequest.cpp:590
> +        if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {

It would be good to change all the uses of "Content-Type" to HTTPHeaderName::ContentType, not just the ones that need to be touched to fix this bug.

> Source/WebCore/xml/XMLHttpRequest.cpp:618
> -        String contentType = getRequestHeader("Content-Type");
> -        if (contentType.isEmpty()) {
> +        if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {

In this case it would be better to leave the code as is and instead check for isNull rather than isEmpty. This avoids a double hash table lookup in the case where there is a content type. If you prefer we could call m_requestHeaders.get directly rather than calling getRequestHeader; the main difference is inlining. A call to m_requestHeaders.contains or m_requestHeaders.get will be inlined, whereas a call to getRequestHeader will not be, saving on code size.

[youenn fablet]

Created attachment 265853 [details]
Patch for landing

[youenn fablet]

> It would be good to change all the uses of "Content-Type" to
> HTTPHeaderName::ContentType, not just the ones that need to be touched to
> fix this bug.

We might also want to remove the setRequestHeaderInternal and getRequestHeader helper routines.
I'll do that as a follow-up patch.

> > Source/WebCore/xml/XMLHttpRequest.cpp:618
> > -        String contentType = getRequestHeader("Content-Type");
> > -        if (contentType.isEmpty()) {
> > +        if (!m_requestHeaders.contains(HTTPHeaderName::ContentType)) {
> 
> In this case it would be better to leave the code as is and instead check
> for isNull rather than isEmpty. This avoids a double hash table lookup in
> the case where there is a content type. If you prefer we could call
> m_requestHeaders.get directly rather than calling getRequestHeader; the main
> difference is inlining. A call to m_requestHeaders.contains or
> m_requestHeaders.get will be inlined, whereas a call to getRequestHeader
> will not be, saving on code size.

isNull() sounds better indeed.
Landing patch is fixed accordingly.

[WebKit Commit Bot]

Comment on attachment 265853 [details]
Patch for landing

Clearing flags on attachment: 265853

Committed r192620: <http://trac.webkit.org/changeset/192620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.