Bug 46146 – XMLHttpRequest: responseXML returns null if the Content-Type is valid (end in +xml) in some cases

• Component: XML
• Status: RESOLVED FIXED

[Jian Li]

XMLHttpRequest: responseXML returns null if the Content-Type is valid (end in +xml) in some cases

Per the XHR spec:
  2. If final MIME type is not null, text/html, text/xml, application/xml, or does not end in +xml terminate these steps and return null.
  3. If final MIME type is text/html let document be a cookie-free Document object that represents the response entity body parsed following the rules set forth in the HTML specification for an HTML parser with scripting disabled and then terminate this algorithm. [HTML5]

Test: http://tc.labs.opera.com/apis/XMLHttpRequest/responsexml-media-type.htm

[Alexey Proskuryakov]

One of the subtests is definitely wrong - we aren't going to stop creating responseXML for text/xsl. Requesting XSL style sheets is a common use of XMLHttpRequest.

Not sure why "bogus" or "application" are expected to have responseXML, this also seems wrong.

It's surprising that "application/xhtml+xml" and "image/svg+xml" subtests fail, out code looks like they should work.

[Anne van Kesteren]

(In reply to comment #1)
> One of the subtests is definitely wrong - we aren't going to stop creating responseXML for text/xsl. Requesting XSL style sheets is a common use of XMLHttpRequest.

Gecko does not do it.


> Not sure why "bogus" or "application" are expected to have responseXML, this also seems wrong.

They are equal to not having a Content-Type header (as they are not valid media types) and therefore the same as XML.

[Alexey Proskuryakov]

> Gecko does not do it.

This isn't necessarily sufficient for us, as we need to handle resources loaded via non-HTTP protocols, such as custom protocols defined by applications. Since text/xsl is the most common type for these, failing on it would be unexpected.

> They are equal to not having a Content-Type header (as they are not valid media types) and therefore the same as XML.

Interesting, thanks for explanation! I can't find where XMLHttpRequest2 (or RFC2616?) says that - I guess we'll just need to test what other browsers do.

[Anne van Kesteren]

XMLHttpRequest says that if parsing Content-Type fails response MIME type will be null. This matches Firefox. (And Opera, which treats anything as XML at the moment, regardless of media type.)

[Alexey Proskuryakov]

Got it. Perhaps "parsed properly" could be re-phrased in a more formal manner, as I've been expecting "Content-Type: foobar" to be considered something that's parsed properly, even though the resulting media type is invalid.

[Rob Buis]

Created attachment 354374 [details]
Patch

[EWS Watchlist]

Comment on attachment 354374 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/supported-xml-content-types.html

[EWS Watchlist]

Created attachment 354381 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354374 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/supported-xml-content-types.html

[EWS Watchlist]

Created attachment 354382 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 354374 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/supported-xml-content-types.html

[EWS Watchlist]

Created attachment 354391 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354374 [details]
Patch

Attachment 354374 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9930513

New failing tests:
http/tests/xmlhttprequest/supported-xml-content-types.html

[EWS Watchlist]

Created attachment 354416 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Frédéric Wang (:fredw)]

Comment on attachment 354374 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:865
> +            if (isValidContentType(extractedMIMEType))

I wonder if extractMIMETypeFromMediaType should be rewritten to return a StringView so that we don't actually need to allocate a new string when the content type is invalid.

[Rob Buis]

Created attachment 374548 [details]
Patch

[Rob Buis]

Created attachment 374555 [details]
Patch

[Rob Buis]

I verified locally that the "invalid types" from supported-xml-content-types.html actually are accepted due to the text/xml fallback in Firefox as well. If desired I can also make a patch adding these subtypes to WPT test xhr/responsexml-media-type.htm.

[Alexey Proskuryakov]

Comment on attachment 374555 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Behavior matches Firefox.

Only Firefox?

[Rob Buis]

(In reply to Alexey Proskuryakov from comment #19)
> Comment on attachment 374555 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374555&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Behavior matches Firefox.
> 
> Only Firefox?

Yes, chromium have a tracking bug but no fix yet (https://bugs.chromium.org/p/chromium/issues/detail?id=651750).

The only place they differ from WebKit is that they PASS the 'bogus+xml' case, 'bogus' and 'application' subtests are FAILs in chromium as well.

[youenn fablet]

Comment on attachment 374555 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:844
> +            mimeType = m_response.httpHeaderField(HTTPHeaderName::ContentType);

Reusing mimeType makes the code more difficult to read.
How about creating a new variable named contentType instead.

> Source/WebCore/xml/XMLHttpRequest.cpp:848
> +            mimeType = parsedContentType->mimeType();

We could return early with "return parsedContentType->mimeType();"

> Source/WebCore/xml/XMLHttpRequest.cpp:850
>              mimeType = "text/xml"_s;

Ditto to return early.

[Rob Buis]

Created attachment 377792 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377792 [details]
Patch

Clearing flags on attachment: 377792

Committed r249361: <https://trac.webkit.org/changeset/249361>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54912609>