Bug 168724 - XMLHttpRequest: do not sniff text/html, and do not sniff XML when responseType is set to "text"
Summary: XMLHttpRequest: do not sniff text/html, and do not sniff XML when responseTyp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on: 178172
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-22 06:53 PST by Anne van Kesteren
Modified: 2017-10-11 17:28 PDT (History)
9 users (show)

See Also:


Attachments
WIP Patch (72.36 KB, patch)
2017-10-11 10:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.62 KB, patch)
2017-10-11 13:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2017-10-11 13:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.33 MB, application/zip)
2017-10-11 14:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.02 MB, application/zip)
2017-10-11 14:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (995.80 KB, application/zip)
2017-10-11 14:41 PDT, Build Bot
no flags Details
Patch (25.37 KB, patch)
2017-10-11 14:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Chris Dumez 2017-10-11 09:10:05 PDT
Blink patch:
https://bugs.chromium.org/p/chromium/issues/detail?id=695045
Comment 2 Chris Dumez 2017-10-11 10:14:11 PDT
Created attachment 323424 [details]
WIP Patch
Comment 3 Chris Dumez 2017-10-11 13:26:44 PDT
Created attachment 323453 [details]
Patch
Comment 4 Chris Dumez 2017-10-11 13:35:43 PDT
Created attachment 323455 [details]
Patch
Comment 5 Build Bot 2017-10-11 14:24:32 PDT
Comment on attachment 323455 [details]
Patch

Attachment 323455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4827931

New failing tests:
http/tests/xmlhttprequest/response-encoding.html
Comment 6 Build Bot 2017-10-11 14:24:33 PDT
Created attachment 323461 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-10-11 14:41:11 PDT
Comment on attachment 323455 [details]
Patch

Attachment 323455 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4827990

New failing tests:
http/tests/xmlhttprequest/response-encoding.html
Comment 8 Build Bot 2017-10-11 14:41:12 PDT
Created attachment 323462 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-10-11 14:41:52 PDT
Comment on attachment 323455 [details]
Patch

Attachment 323455 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4828066

New failing tests:
http/tests/xmlhttprequest/response-encoding.html
Comment 10 Build Bot 2017-10-11 14:41:53 PDT
Created attachment 323463 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Chris Dumez 2017-10-11 14:49:53 PDT
Created attachment 323465 [details]
Patch
Comment 12 youenn fablet 2017-10-11 15:03:44 PDT
Patch seems fine.
I guess the compat issue is really low since Chrome probably shipped this a while ago.
Comment 13 Chris Dumez 2017-10-11 15:09:31 PDT
(In reply to youenn fablet from comment #12)
> Patch seems fine.
> I guess the compat issue is really low since Chrome probably shipped this a
> while ago.

Full disclosure, the Blink patch landed yesterday so they did not ship this yet. They will definitely ship it before us though and we can always revert if they run into trouble.
Comment 14 youenn fablet 2017-10-11 15:17:42 PDT
Do you know what IE/MS are doing here?
Comment 15 youenn fablet 2017-10-11 15:17:56 PDT
IE/Firefox...
Comment 16 Chris Dumez 2017-10-11 15:20:44 PDT
(In reply to youenn fablet from comment #15)
> IE/Firefox...

I do not know about IE but Firefox aligned with the spec. Info available at:
- https://github.com/w3c/web-platform-tests/pull/4935
- https://bugzilla.mozilla.org/show_bug.cgi?id=1341260

So basically, both Firefox and Chrome aligned. Anne may know about IE / Edge.
Comment 17 Ryosuke Niwa 2017-10-11 15:21:11 PDT
Comment on attachment 323465 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:1062
> +        }

Don't we need to check if the response is HTML? We used to do that in the old code.
Comment 18 Chris Dumez 2017-10-11 16:58:47 PDT
Comment on attachment 323465 [details]
Patch

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

>> Source/WebCore/xml/XMLHttpRequest.cpp:1062
>> +        }
> 
> Don't we need to check if the response is HTML? We used to do that in the old code.

No, if I add this check then a WPT subtest would no longer start passing.

Also note that Chrome is not doing the HTML check either in this case.

> LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/responsetext-decoding-expected.txt:3
> +PASS XMLHttpRequest: responseText decoding (text/html %3C!doctype%20html%3E%3Cmeta%20charset%3Dwindows-1252%3E%3Cx%3E%e6%a9%9f%3C%2Fx%3E empty) 

This subtest would fail again.
Comment 19 WebKit Commit Bot 2017-10-11 17:27:27 PDT
Comment on attachment 323465 [details]
Patch

Clearing flags on attachment: 323465

Committed r223217: <https://trac.webkit.org/changeset/223217>
Comment 20 WebKit Commit Bot 2017-10-11 17:27:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2017-10-11 17:28:48 PDT
<rdar://problem/34946063>