- Revision
- 254652
- Author
- cdu...@apple.com
- Date
- 2020-01-15 15:55:04 -0800 (Wed, 15 Jan 2020)
Log Message
Regression(r253213) Load hang and high CPU usage when trying to load myuhc.com
https://bugs.webkit.org/show_bug.cgi?id=206315
<rdar://problem/58139842>
Reviewed by Geoffrey Garen.
Source/WebCore:
Starting in r253213, we now throw when trying to do a sync XHR during unload. Unfortunately, this is confusing the script
on myuhc.com and it ends up retrying the sync XHR in a tight loop. To address the issue, I am putting in a safety net which
ignores calls to XMLHttpRequest.send() instead of throwing, once we've reached 5 sync XHR failures during unload.
Throwing is useful because this gives a change for Web authors to fall back to using Beacon API or Fetch KeepAlive if the
sync XHR fails. There is already code out there doing just that. You could imagine content doing more than one sync XHR
during unload, each one with a good beacon API fallback. For this reason, I put in a limit of 5 sync failures before
we stop throwing. Having a limit is important to break bad loops when the content simply retries the same sync XHR load
when the sync XHR send() call throws.
Tests: fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html
fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html
* dom/Document.cpp:
(WebCore::Document::didRejectSyncXHRDuringPageDismissal):
(WebCore::Document::shouldIgnoreSyncXHRs const):
* dom/Document.h:
* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::DocumentThreadableLoader):
* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::prepareToSend):
LayoutTests:
Add layout test coverage.
* fast/xmlhttprequest/resources/xmlhttprequest-multiple-sync-xhr-during-unload-iframe.html: Added.
* fast/xmlhttprequest/resources/xmlhttprequest-sync-xhr-failure-loop-during-unload-iframe.html: Added.
* fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload-expected.txt: Added.
* fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html: Added.
* fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload-expected.txt: Added.
* fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (254651 => 254652)
--- trunk/LayoutTests/ChangeLog 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/LayoutTests/ChangeLog 2020-01-15 23:55:04 UTC (rev 254652)
@@ -1,3 +1,20 @@
+2020-01-15 Chris Dumez <cdu...@apple.com>
+
+ Regression(r253213) Load hang and high CPU usage when trying to load myuhc.com
+ https://bugs.webkit.org/show_bug.cgi?id=206315
+ <rdar://problem/58139842>
+
+ Reviewed by Geoffrey Garen.
+
+ Add layout test coverage.
+
+ * fast/xmlhttprequest/resources/xmlhttprequest-multiple-sync-xhr-during-unload-iframe.html: Added.
+ * fast/xmlhttprequest/resources/xmlhttprequest-sync-xhr-failure-loop-during-unload-iframe.html: Added.
+ * fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload-expected.txt: Added.
+ * fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html: Added.
+ * fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload-expected.txt: Added.
+ * fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html: Added.
+
2020-01-15 Commit Queue <commit-qu...@webkit.org>
Unreviewed, rolling out r254576.
Added: trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-multiple-sync-xhr-during-unload-iframe.html (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-multiple-sync-xhr-during-unload-iframe.html (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-multiple-sync-xhr-during-unload-iframe.html 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,22 @@
+<script>
+let xhrExceptionCount = 0;
+
+function doSyncXHR(i)
+{
+ try {
+ var xhr = new XMLHttpRequest();
+ xhr.open("GET", "xmlhttprequest-responsetype-json.json?" + i, false);
+ xhr.send(null);
+ } catch(e) {
+ xhrExceptionCount++;
+ return false;
+ }
+ return true;
+}
+
+_onunload_ = () => {
+ for (let i = 0; i < 5; i++)
+ doSyncXHR(i);
+ top.frameDidUnload(xhrExceptionCount);
+}
+</script>
Added: trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-sync-xhr-failure-loop-during-unload-iframe.html (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-sync-xhr-failure-loop-during-unload-iframe.html (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-sync-xhr-failure-loop-during-unload-iframe.html 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,18 @@
+<script>
+function doSyncXHR()
+{
+ try {
+ var xhr = new XMLHttpRequest();
+ xhr.open("GET", "xmlhttprequest-responsetype-json.json", false);
+ xhr.send(null);
+ } catch(e) {
+ return false;
+ }
+ return true;
+}
+
+_onunload_ = () => {
+ while (!doSyncXHR()) { };
+ top.frameDidUnload();
+}
+</script>
Added: trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload-expected.txt (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload-expected.txt 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,16 @@
+frame "<!--frame1-->" - has 1 onunload handler(s)
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json?0.
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json?1.
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json?2.
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json?3.
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json?4.
+Makes sure that we throw when doing sync XHRs during unload.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS xhrExceptionCount is 5
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<iframe id="testFrame" src=""
+<script>
+description("Makes sure that we throw when doing sync XHRs during unload.");
+jsTestIsAsync = true;
+
+frameDidUnload = (_xhrExceptionCount) => {
+ xhrExceptionCount = _xhrExceptionCount;
+ shouldBe("xhrExceptionCount", "5");
+ finishJSTest();
+}
+
+_onload_ = () => {
+ testFrame.remove();
+}
+</script>
+</html>
Added: trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload-expected.txt (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload-expected.txt 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,18 @@
+frame "<!--frame1-->" - has 1 onunload handler(s)
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: XMLHttpRequest cannot load xmlhttprequest-responsetype-json.json.
+CONSOLE MESSAGE: line 7: Ignoring XMLHttpRequest.send() call for 'xmlhttprequest-responsetype-json.json' because the maximum number of synchronous failures was reached.
+Makes sure that retrying failed sync XHRs in a loop during unload does not cause a hang.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS No hang while unloading the iframe
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html (0 => 254652)
--- trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html (rev 0)
+++ trunk/LayoutTests/fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html 2020-01-15 23:55:04 UTC (rev 254652)
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<iframe id="testFrame" src=""
+<script>
+description("Makes sure that retrying failed sync XHRs in a loop during unload does not cause a hang.");
+jsTestIsAsync = true;
+
+frameDidUnload = () => {
+ testPassed("No hang while unloading the iframe");
+ finishJSTest();
+}
+
+_onload_ = () => {
+ testFrame.remove();
+}
+</script>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (254651 => 254652)
--- trunk/Source/WebCore/ChangeLog 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/Source/WebCore/ChangeLog 2020-01-15 23:55:04 UTC (rev 254652)
@@ -1,3 +1,33 @@
+2020-01-15 Chris Dumez <cdu...@apple.com>
+
+ Regression(r253213) Load hang and high CPU usage when trying to load myuhc.com
+ https://bugs.webkit.org/show_bug.cgi?id=206315
+ <rdar://problem/58139842>
+
+ Reviewed by Geoffrey Garen.
+
+ Starting in r253213, we now throw when trying to do a sync XHR during unload. Unfortunately, this is confusing the script
+ on myuhc.com and it ends up retrying the sync XHR in a tight loop. To address the issue, I am putting in a safety net which
+ ignores calls to XMLHttpRequest.send() instead of throwing, once we've reached 5 sync XHR failures during unload.
+
+ Throwing is useful because this gives a change for Web authors to fall back to using Beacon API or Fetch KeepAlive if the
+ sync XHR fails. There is already code out there doing just that. You could imagine content doing more than one sync XHR
+ during unload, each one with a good beacon API fallback. For this reason, I put in a limit of 5 sync failures before
+ we stop throwing. Having a limit is important to break bad loops when the content simply retries the same sync XHR load
+ when the sync XHR send() call throws.
+
+ Tests: fast/xmlhttprequest/xmlhttprequest-multiple-sync-xhr-during-unload.html
+ fast/xmlhttprequest/xmlhttprequest-sync-xhr-failure-loop-during-unload.html
+
+ * dom/Document.cpp:
+ (WebCore::Document::didRejectSyncXHRDuringPageDismissal):
+ (WebCore::Document::shouldIgnoreSyncXHRs const):
+ * dom/Document.h:
+ * loader/DocumentThreadableLoader.cpp:
+ (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):
+ * xml/XMLHttpRequest.cpp:
+ (WebCore::XMLHttpRequest::prepareToSend):
+
2020-01-15 Ross Kirsling <ross.kirsl...@sony.com>
Unreviewed build fix for ENABLE_ACCESSIBILITY=OFF following r254566.
Modified: trunk/Source/WebCore/dom/Document.cpp (254651 => 254652)
--- trunk/Source/WebCore/dom/Document.cpp 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/Source/WebCore/dom/Document.cpp 2020-01-15 23:55:04 UTC (rev 254652)
@@ -8350,6 +8350,24 @@
top.setHasEvaluatedUserAgentScripts();
}
+void Document::didRejectSyncXHRDuringPageDismissal()
+{
+ ++m_numberOfRejectedSyncXHRs;
+ if (m_numberOfRejectedSyncXHRs > 1)
+ return;
+
+ postTask([this, weakThis = makeWeakPtr(*this)](auto&) mutable {
+ if (weakThis)
+ m_numberOfRejectedSyncXHRs = 0;
+ });
+}
+
+bool Document::shouldIgnoreSyncXHRs() const
+{
+ const unsigned maxRejectedSyncXHRsPerEventLoopIteration = 5;
+ return m_numberOfRejectedSyncXHRs > maxRejectedSyncXHRsPerEventLoopIteration;
+}
+
#if ENABLE(APPLE_PAY)
bool Document::isApplePayActive() const
Modified: trunk/Source/WebCore/dom/Document.h (254651 => 254652)
--- trunk/Source/WebCore/dom/Document.h 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/Source/WebCore/dom/Document.h 2020-01-15 23:55:04 UTC (rev 254652)
@@ -762,6 +762,9 @@
void setFocusNavigationStartingNode(Node*);
Element* focusNavigationStartingNode(FocusDirection) const;
+ void didRejectSyncXHRDuringPageDismissal();
+ bool shouldIgnoreSyncXHRs() const;
+
enum class NodeRemoval { Node, ChildrenOfNode };
void adjustFocusedNodeOnNodeRemoval(Node&, NodeRemoval = NodeRemoval::Node);
void adjustFocusNavigationNodeOnNodeRemoval(Node&, NodeRemoval = NodeRemoval::Node);
@@ -2053,7 +2056,7 @@
RefPtr<Worklet> m_paintWorklet;
HashMap<String, Ref<PaintWorkletGlobalScope>> m_paintWorkletGlobalScopes;
#endif
-
+ unsigned m_numberOfRejectedSyncXHRs { 0 };
bool m_hasEvaluatedUserAgentScripts { false };
bool m_isRunningUserScripts { false };
bool m_mayBeDetachedFromFrame { true };
Modified: trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp (254651 => 254652)
--- trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/Source/WebCore/loader/DocumentThreadableLoader.cpp 2020-01-15 23:55:04 UTC (rev 254652)
@@ -133,6 +133,7 @@
ASSERT(m_async || m_referrer.isEmpty());
if (document.settings().disallowSyncXHRDuringPageDismissalEnabled() && !m_async && (!document.page() || !document.page()->areSynchronousLoadsAllowed())) {
+ document.didRejectSyncXHRDuringPageDismissal();
logErrorAndFail(ResourceError(errorDomainWebKitInternal, 0, request.url(), "Synchronous loads are not allowed at this time"));
return;
}
Modified: trunk/Source/WebCore/xml/XMLHttpRequest.cpp (254651 => 254652)
--- trunk/Source/WebCore/xml/XMLHttpRequest.cpp 2020-01-15 23:25:16 UTC (rev 254651)
+++ trunk/Source/WebCore/xml/XMLHttpRequest.cpp 2020-01-15 23:55:04 UTC (rev 254652)
@@ -413,6 +413,11 @@
auto& context = *scriptExecutionContext();
+ if (is<Document>(context) && downcast<Document>(context).shouldIgnoreSyncXHRs()) {
+ logConsoleError(scriptExecutionContext(), makeString("Ignoring XMLHttpRequest.send() call for '", m_url.string(), "' because the maximum number of synchronous failures was reached."));
+ return ExceptionOr<void> { };
+ }
+
if (readyState() != OPENED || m_sendFlag)
return ExceptionOr<void> { Exception { InvalidStateError } };
ASSERT(!m_loader);
