Bug 184645 – Adjust XMLHttpRequest Content-Type handling

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anne van Kesteren]

See https://github.com/whatwg/xhr/pull/176 and https://github.com/w3c/web-platform-tests/pull/8422.

In particular, XMLHttpRequest Content-Type manipulation is now to be done in terms of the MIME type parser defined in MIME Sniffing.

[Anne van Kesteren]

https://github.com/whatwg/xhr/pull/224 and https://github.com/web-platform-tests/wpt/pull/13779 have minor tweaks to this to make this more web-compatible.

[Chris Dumez]

Test is xhr/send-content-type-charset.htm.

[Chris Dumez]

Quite a few failures on imported/w3c/web-platform-tests/xhr/send-content-type-charset.htm:
FAIL header with invalid MIME type is not changed assert_equals: expected "text; charset=ascii" but got "text; charset=UTF-8"
PASS header with invalid MIME type (empty string) is not changed
PASS known charset but bogus header - missing MIME type
PASS bogus charset and bogus header - missing MIME type
FAIL If charset= param is UTF-8 (case-insensitive), it should not be changed assert_equals: expected "text/plain;charset=utf-8" but got "text/plain;charset=UTF-8"
PASS If no charset= param is given, implementation should not add one - unknown MIME
PASS If no charset= param is given, implementation should not add one - known MIME
PASS If no charset= param is given, implementation should not add one - known MIME, unknown param, two spaces
FAIL charset given but wrong, fix it (unknown MIME, bogus charset) assert_equals: expected "text/x-thepiano;charset=UTF-8" but got "text/x-thepiano;charset= UTF-8"
FAIL If charset= param is UTF-8 (case-insensitive), it should not be changed (bogus charset) assert_equals: expected "text/plain;charset=utf-8;charset=waddup" but got "text/plain;charset=UTF-8;charset=UTF-8"
PASS charset given but wrong, fix it (known MIME, actual charset)
FAIL Multiple non-UTF-8 charset parameters deduplicate, bogus parameter dropped assert_equals: expected "text/x-pink-unicorn;charset=UTF-8" but got "text/x-pink-unicorn; charset=UTF-8; charset=UTF-8; notrelated; charset=UTF-8"
PASS No content type set, give MIME and charset
FAIL charset with space that is UTF-8 does not change assert_equals: expected "text/plain;charset= utf-8" but got "text/plain;charset= UTF-8"
FAIL charset in double quotes that is UTF-8 does not change assert_equals: expected "text/plain;charset=\"utf-8\"" but got "text/plain;charset=\"UTF-8\""
FAIL charset in double quotes with space assert_equals: expected "text/plain;charset=UTF-8" but got "text/plain;charset=\" UTF-8\""
FAIL charset in double quotes with backslashes that is UTF-8 does not change assert_equals: expected "text/plain;charset=\"u\\t\f-8\"" but got "text/plain;charset=\"UTF-8\""
FAIL unknown parameters need to be preserved assert_equals: expected "yo/yo;charset=UTF-8;yo=YO;x=y" but got "YO/yo;charset=UTF-8;yo=YO; X=y"

[Alexey Proskuryakov]

I think that the challenge here is to separate cases that have the potential of fixing or breaking websites from irrelevant subtests.

[Rob Buis]

Created attachment 363117 [details]
Patch

[EWS Watchlist]

Comment on attachment 363117 [details]
Patch

Attachment 363117 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11306680

New failing tests:
http/tests/xmlhttprequest/request-encoding2.html

[EWS Watchlist]

Created attachment 363123 [details]
Archive of layout-test-results from ews102 for mac-highsierra

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

[Rob Buis]

Created attachment 363126 [details]
Patch

[Rob Buis]

Created attachment 363208 [details]
Patch

[youenn fablet]

Comment on attachment 363208 [details]
Patch

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

> Source/WebCore/platform/network/ParsedContentType.cpp:351
> +    m_parameterValues.set("charset", charset);

s/"charset"/"charset"_s/

Could also be String&&/WTFMove

> Source/WebCore/xml/XMLHttpRequest.cpp:77
>  static void replaceCharsetInMediaType(String& mediaType, const String& charsetValue)

Can we make replaceCharsetInMediaType closer to spec by no longer passing charsetValue, since it is always UTF-8?
We could then change replaceCharsetInMediaType prototype so that it takes a const String& and return a String.

> Source/WebCore/xml/XMLHttpRequest.cpp:79
> +    Optional<ParsedContentType> parsedContentType = ParsedContentType::create(mediaType);

s/Optional<ParsedContentType>/auto/

[Rob Buis]

Created attachment 363318 [details]
Patch

[Rob Buis]

Comment on attachment 363208 [details]
Patch

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

>> Source/WebCore/platform/network/ParsedContentType.cpp:351
>> +    m_parameterValues.set("charset", charset);
> 
> s/"charset"/"charset"_s/
> 
> Could also be String&&/WTFMove

Done.

>> Source/WebCore/xml/XMLHttpRequest.cpp:77
>>  static void replaceCharsetInMediaType(String& mediaType, const String& charsetValue)
> 
> Can we make replaceCharsetInMediaType closer to spec by no longer passing charsetValue, since it is always UTF-8?
> We could then change replaceCharsetInMediaType prototype so that it takes a const String& and return a String.

Good idea, done.

>> Source/WebCore/xml/XMLHttpRequest.cpp:79
>> +    Optional<ParsedContentType> parsedContentType = ParsedContentType::create(mediaType);
> 
> s/Optional<ParsedContentType>/auto/

Done.

[youenn fablet]

Comment on attachment 363318 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:77
> +static String replaceCharsetInMediaType(const String& mediaType)

I would rename it to replaceCharsetInMediaTypeIfNeeded.
Thinking about it, chances are that mediaType will be returned without any change.
We could take a String&& and return a String. Or go back to void/String&.

> Source/WebCore/xml/XMLHttpRequest.cpp:84
> +    return parsedContentType->serialize();

"text/plain; charset=UTF-8" should remain as is and not become "text/plain;charset=UTF-8", right?
Can we add a test if we do not cover this case?

> LayoutTests/imported/w3c/ChangeLog:8
> +        Sync test and uodate test expectation (all PASSes).

s/uodate/update/

[Rob Buis]

Created attachment 363337 [details]
Patch

[Rob Buis]

Comment on attachment 363318 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:77
>> +static String replaceCharsetInMediaType(const String& mediaType)
> 
> I would rename it to replaceCharsetInMediaTypeIfNeeded.
> Thinking about it, chances are that mediaType will be returned without any change.
> We could take a String&& and return a String. Or go back to void/String&.

I thought about it but thought the name would get kind of long. Anyway fine with it :)

>> Source/WebCore/xml/XMLHttpRequest.cpp:84
>> +    return parsedContentType->serialize();
> 
> "text/plain; charset=UTF-8" should remain as is and not become "text/plain;charset=UTF-8", right?
> Can we add a test if we do not cover this case?

We pass that, I tried it locally. I would like to cover this case in send-content-type-charset.htm, but in a follow-up commit to WPT.

>> LayoutTests/imported/w3c/ChangeLog:8
>> +        Sync test and uodate test expectation (all PASSes).
> 
> s/uodate/update/

Fixed.

[WebKit Commit Bot]

Comment on attachment 363337 [details]
Patch

Clearing flags on attachment: 363337

Committed r242284: <https://trac.webkit.org/changeset/242284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48521015>