Summary: | XMLHttpRequest: responseXML returns null if the Content-Type is valid (end in +xml) in some cases | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||||||||||||
Component: | XML | Assignee: | 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
Jian Li
2010-09-20 17:41:40 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. (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. > 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. 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.) 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. Created attachment 354374 [details]
Patch
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 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 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 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 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 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 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 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 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. Created attachment 374548 [details]
Patch
Created attachment 374555 [details]
Patch
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 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? (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 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. Created attachment 377792 [details]
Patch
Comment on attachment 377792 [details] Patch Clearing flags on attachment: 377792 Committed r249361: <https://trac.webkit.org/changeset/249361> All reviewed patches have been landed. Closing bug. |