Bug 46146

Summary: XMLHttpRequest: responseXML returns null if the Content-Type is valid (end in +xml) in some cases
Product: WebKit Reporter: Jian Li <jianli>
Component: XMLAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: annevk, ap, commit-queue, dimich, ews-watchlist, fishd, fred.wang, jianli, levin, rbuis, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews115 for mac-sierra
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch
none
Patch none

Description Jian Li 2010-09-20 17:41:40 PDT
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
Comment 1 Alexey Proskuryakov 2010-09-21 09:50:19 PDT
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.
Comment 2 Anne van Kesteren 2010-09-23 03:14:34 PDT
(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.
Comment 3 Alexey Proskuryakov 2010-09-23 09:09:21 PDT
> 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.
Comment 4 Anne van Kesteren 2010-09-24 00:52:45 PDT
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.)
Comment 5 Alexey Proskuryakov 2010-09-24 01:16:08 PDT
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.
Comment 6 Rob Buis 2018-11-09 12:41:16 PST
Created attachment 354374 [details]
Patch
Comment 7 EWS Watchlist 2018-11-09 13:52:06 PST
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
Comment 8 EWS Watchlist 2018-11-09 13:52:08 PST
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
Comment 9 EWS Watchlist 2018-11-09 13:59:45 PST
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
Comment 10 EWS Watchlist 2018-11-09 13:59:47 PST
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
Comment 11 EWS Watchlist 2018-11-09 15:01:25 PST
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
Comment 12 EWS Watchlist 2018-11-09 15:01:26 PST
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
Comment 13 EWS Watchlist 2018-11-09 16:45:56 PST
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
Comment 14 EWS Watchlist 2018-11-09 16:46:08 PST
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
Comment 15 Frédéric Wang (:fredw) 2018-11-12 23:24:23 PST
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.
Comment 16 Rob Buis 2019-07-20 06:13:01 PDT
Created attachment 374548 [details]
Patch
Comment 17 Rob Buis 2019-07-20 09:01:08 PDT
Created attachment 374555 [details]
Patch
Comment 18 Rob Buis 2019-07-20 09:05:39 PDT
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.
Comment 19 Alexey Proskuryakov 2019-07-20 11:28:05 PDT
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?
Comment 20 Rob Buis 2019-07-20 12:00:23 PDT
(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.
Comment 21 youenn fablet 2019-08-30 10:03:46 PDT
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.
Comment 22 Rob Buis 2019-08-31 01:24:25 PDT
Created attachment 377792 [details]
Patch
Comment 23 WebKit Commit Bot 2019-08-31 03:16:46 PDT
Comment on attachment 377792 [details]
Patch

Clearing flags on attachment: 377792

Committed r249361: <https://trac.webkit.org/changeset/249361>
Comment 24 WebKit Commit Bot 2019-08-31 03:16:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Radar WebKit Bug Importer 2019-08-31 03:17:23 PDT
<rdar://problem/54912609>