Bug 231138 - REGRESSION (r277818): XHR with requestType document broken for larger HTML files
Summary: REGRESSION (r277818): XHR with requestType document broken for larger HTML files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 15
Hardware: All Unspecified
: P2 Critical
Assignee: Antti Koivisto
URL:
Keywords: InRadar
: 231375 (view as bug list)
Depends on:
Blocks: 224609
  Show dependency treegraph
 
Reported: 2021-10-03 08:47 PDT by erik.witt
Modified: 2021-12-14 11:06 PST (History)
16 users (show)

See Also:


Attachments
Failing test (2.28 MB, patch)
2021-10-04 08:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Failing Test (2.28 MB, patch)
2021-10-04 09:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2021-10-08 05:16 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2021-10-08 05:31 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description erik.witt 2021-10-03 08:47:13 PDT
When requesting HTML documents via XHRHttpRequests the response object is missing even when the request itself was successful.

Ways to reproduce:

1. Go to https://www.decathlon.de
2. In the console execute this code
```
var xhr = new XMLHttpRequest();
xhr.withCredentials = true;
xhr.open('GET', 'https://www.decathlon.de/browse/c0-alle-sportarten-a-z/c1-fahrrad-welt/c3-mountainbike/_/N-obq78x', true); 
xhr.responseType = 'document';

xhr.addEventListener('readystatechange', () => {
  if (xhr.readyState === 4) {
    console.log('Success')
    console.log('Res: ' + xhr.response)
  }
});

// Handle errors of XHR connection
xhr.addEventListener('error', (e) => {
  
    console.log('Error', e)
});
xhr.send();
```

3. Observe that the xhr.response is null even though the request was successful.


The same issue is reproducable on a lot of domains but does not happen for all HTML files. It seems to be related to the size of the HTML document though. For example, if you swap the URL in the example above for this one https://www.decathlon.de/p/mountainbike-e-st-520-27-5-zoll-grau-gelb/_/R-p-311400 everything works fine and the response is available.

This issue is highly critical for a lot of our customers. Is there a workaround to still use XHR for those files until there is a fix for the issue?
Comment 1 Alexey Proskuryakov 2021-10-03 19:48:47 PDT
Ugh. Has this been the case before Safari 15 too?

From looking at the code, there isn't a lot that can go wrong. Perhaps some confusion about MIME types that would be easy to see in a debugger.

As a workaround, it looks like fetching as string and parsing the response using DOMParser works.
Comment 2 erik.witt 2021-10-03 22:05:07 PDT
It works perfectly fine in Safari 14 (tested on Mac with Version 14.1.2 (16611.3.10.1.6)). It only breaks in Safari 15 (tested on Mac with Release 133 (Safari 15.4, WebKit 16613.1.2.2)).

As I said, it breaks a lot of customer sites, so we see the error in a lot of production websites in Safari 15 both on Mac and iPhones.

Are you able to reproduce with the code snippet I supplied?
Comment 3 Alexey Proskuryakov 2021-10-03 22:34:55 PDT
Yes, I can reproduce. Thank you for the easy to follow steps.

Please let us know if the suggested workaround doesn't help.
Comment 4 Radar WebKit Bug Importer 2021-10-03 22:35:17 PDT
<rdar://problem/83823282>
Comment 5 erik.witt 2021-10-03 22:49:39 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Yes, I can reproduce. Thank you for the easy to follow steps.
> 
> Please let us know if the suggested workaround doesn't help.

I think the workaround works functionally. Since we use that code in a very performance critical phase, I am not sure it will work well for us though.

Not sure how it works in Safari in particular but in Chrome for example the native DOM parsing of the XHRHttpRequest is more efficient compared to using DOMParser with the document as string. The parsing of the XHR request is for example interlaced with JS execution of the page while DOMParser will block the main thread until the document is parsed (probably as it is not streamed into the parser)
Comment 6 Chris Dumez 2021-10-04 08:04:37 PDT
r279000: BAD
r277000: GOOD

Will bisect.
Comment 7 Chris Dumez 2021-10-04 08:24:51 PDT
Regression range:
https://trac.webkit.org/log/webkit/trunk?mode=follow_copy&rev=277818&stop_rev=277809

Almost certainly Antti's https://trac.webkit.org/r277818.
Comment 8 Chris Dumez 2021-10-04 08:58:09 PDT
Created attachment 440066 [details]
Failing test
Comment 9 Chris Dumez 2021-10-04 09:02:53 PDT
Created attachment 440068 [details]
Failing Test
Comment 10 Chris Dumez 2021-10-04 10:10:14 PDT
Note that the test case I uploaded relies on the actual page from decathlon which may not be suitable to commit to our repo. I tried generating a large lorem Ipsum HTML file instead but it didn't reproduce.
Comment 11 Jonathan Bedard 2021-10-04 12:12:38 PDT
Comment on attachment 440068 [details]
Failing Test

Changing this so it's not marked as a patch, EWS is having a rough time with it.
Comment 12 Aakash Jain 2021-10-05 08:27:12 PDT
Marked as patch again to test EWS. EWS is able to process it efficiently now (after the fix in Bug 231190).
Comment 13 erik.witt 2021-10-07 10:57:04 PDT
Hi folks,

I think things are more severely broken than just XHR with response type document.

Trying out the workaround via DOMParser I discovered that one is broken in iOS 15 as well. See bug report https://bugs.webkit.org/show_bug.cgi?id=231375
Comment 14 Chris Dumez 2021-10-07 11:28:14 PDT
(In reply to erik.witt from comment #13)
> Hi folks,
> 
> I think things are more severely broken than just XHR with response type
> document.
> 
> Trying out the workaround via DOMParser I discovered that one is broken in
> iOS 15 as well. See bug report https://bugs.webkit.org/show_bug.cgi?id=231375

I guess that isn't too surprising. XHR response type document and DOMParser are probably using the same logic for parsing.

@antti: Should we consider reverting your change until we have a fix? Seems like a bad regression.
Comment 15 erik.witt 2021-10-07 23:24:51 PDT
You are right, it is not too surprising but it eliminates the workaround Alexey suggested.

Is there another way to get to the parsed document that currently works in Safari 15? We urgently need one
Comment 16 Antti Koivisto 2021-10-08 05:16:47 PDT
Created attachment 440596 [details]
Patch
Comment 17 Antti Koivisto 2021-10-08 05:31:29 PDT
Created attachment 440597 [details]
Patch
Comment 18 Antti Koivisto 2021-10-08 05:48:46 PDT
There are several possible workarounds, at least

1) Since the bug only happens when the parser yields before a script tag you can avoid it by removing or mangling any <script> in the input source before giving it to HTMLParser (scripts don't execute anyway).

2) Use <template> element

3) Use <iframe sandbox="">
Comment 19 Antti Koivisto 2021-10-08 06:23:29 PDT
> giving it to HTMLParser (scripts don't execute anyway).

DOMParser that is.
Comment 20 Darin Adler 2021-10-08 08:17:29 PDT
Comment on attachment 440597 [details]
Patch

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

> Source/WebCore/dom/DocumentParser.h:54
> +    virtual void appendSynchronously(RefPtr<StringImpl>&& inputSource) { append(WTFMove(inputSource)); }

Totally not about this patch: I do not think we should ever use the type RefPtr<StringImpl> since that’s exactly how String is implemented. I could understand StringImpl* or Ref<StringImpl>, but RefPtr<StringImpl> in particular is just an alternate way to say String, and I think it’s annoying that we mix the two.
Comment 21 EWS 2021-10-08 08:45:01 PDT
Committed r283803 (242696@main): <https://commits.webkit.org/242696@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440597 [details].
Comment 22 erik.witt 2021-10-08 09:01:39 PDT
Hi all,

sorry for the stupid question. But since the status is set to resolved, when does the fix land in the Safari release und which release will it be?
Comment 23 Antti Koivisto 2021-10-08 09:05:00 PDT
Can't comment on release but I'll try to update this bug when the fix is available in a public beta.
Comment 24 Alexey Proskuryakov 2021-10-11 10:04:40 PDT
*** Bug 231375 has been marked as a duplicate of this bug. ***
Comment 25 erik.witt 2021-10-28 08:32:20 PDT
Hey folks!

Since Safari 15.1 has landed and the issue seems to still be there, is there already an open version somewhere where we can test the fix?

Or is there any way of seeing in which release this fix is included?
Comment 26 Chris Dumez 2021-10-28 08:34:51 PDT
(In reply to erik.witt from comment #25)
> Hey folks!
> 
> Since Safari 15.1 has landed and the issue seems to still be there, is there
> already an open version somewhere where we can test the fix?
> 
> Or is there any way of seeing in which release this fix is included?

Safari Technology Preview 134 seems to have the fix:
https://developer.apple.com/safari/download/

I can confirm this has not shipped yet in mainline Safari.
Comment 27 Antti Koivisto 2021-12-14 11:06:50 PST
This should be fixed now in Safari 15.2