Prevent synchronous XHR in beforeunload / unload event handlers
https://bugs.webkit.org/show_bug.cgi?id=204912
<rdar://problem/57676394>
Reviewed by Darin Adler.
Source/WebCore:
Prevent synchronous XHR in beforeunload / unload event handlers. They are terrible for performance
and the Beacon API (or Fetch keepalive) are more efficient & supported alternatives.
In particular, this would cause hangs when trying to navigate away from a site or when closing
attempt, which would result in terrible user experience.
Chrome and Edge have expressed public support for this. Chrome has actually been testing this behavior
for a while now:
https://www.chromestatus.com/feature/4664843055398912
I added this new behavior behind an experimental feature flag, enabled by default.
Tests: http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html
http/tests/xmlhttprequest/sync-xhr-in-unload.html
* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::DocumentThreadableLoader):
* loader/FrameLoader.cpp:
(WebCore::PageLevelForbidScope::PageLevelForbidScope):
(WebCore::ForbidPromptsScope::ForbidPromptsScope):
(WebCore::ForbidPromptsScope::~ForbidPromptsScope):
(WebCore::ForbidSynchronousLoadsScope::ForbidSynchronousLoadsScope):
(WebCore::ForbidSynchronousLoadsScope::~ForbidSynchronousLoadsScope):
(WebCore::FrameLoader::dispatchUnloadEvents):
(WebCore::FrameLoader::dispatchBeforeUnloadEvent):
* page/Page.cpp:
(WebCore::Page::forbidSynchronousLoads):
(WebCore::Page::allowSynchronousLoads):
(WebCore::Page::areSynchronousLoadsAllowed):
* page/Page.h:
LayoutTests:
Add layout test coverage.
* http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html: Added.
* http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html: Added.
* http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt: Added.
* http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html: Added.
* http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt: Added.
* http/tests/xmlhttprequest/sync-xhr-in-unload.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253213 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 1892efe..64b8132 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,20 @@
+2019-12-06 Chris Dumez <cdumez@apple.com>
+
+ Prevent synchronous XHR in beforeunload / unload event handlers
+ https://bugs.webkit.org/show_bug.cgi?id=204912
+ <rdar://problem/57676394>
+
+ Reviewed by Darin Adler.
+
+ Add layout test coverage.
+
+ * http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html: Added.
+ * http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html: Added.
+ * http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt: Added.
+ * http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html: Added.
+ * http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt: Added.
+ * http/tests/xmlhttprequest/sync-xhr-in-unload.html: Added.
+
2019-12-06 Antti Koivisto <antti@apple.com>
Support for resolving highlight pseudo element style
diff --git a/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html b/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html
new file mode 100644
index 0000000..47c6f4c
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-beforeunload-window.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+window.addEventListener("beforeunload", (event) => {
+ xhr = new XMLHttpRequest();
+ xhr.open("GET", "/resources/dummy.xml", false);
+ try {
+ xhr.send(null);
+ opener.testFailed("Doing a synchronous XHR in the beforeunload event handler did not throw");
+ } catch (e) {
+ opener.testPassed("Doing a synchronous XHR in the beforeunload event handler threw an exception");
+ }
+ opener.finishJSTest();
+});
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html b/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html
new file mode 100644
index 0000000..458e2e5
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/resources/sync-xhr-in-unload-window.html
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+window.addEventListener("unload", (event) => {
+ xhr = new XMLHttpRequest();
+ xhr.open("GET", "/resources/dummy.xml", false);
+ try {
+ xhr.send(null);
+ opener.testFailed("Doing a synchronous XHR in the unload event handler did not throw");
+ } catch (e) {
+ opener.testPassed("Doing a synchronous XHR in the unload event handler threw an exception");
+ }
+ opener.finishJSTest();
+});
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt
new file mode 100644
index 0000000..05f0e6d
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload-expected.txt
@@ -0,0 +1,11 @@
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load http://127.0.0.1:8000/resources/dummy.xml.
+Checks that trying to do a synchronous XHR in a beforeunload event handler is not allowed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Doing a synchronous XHR in the beforeunload event handler threw an exception
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html
new file mode 100644
index 0000000..d818342
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description("Checks that trying to do a synchronous XHR in a beforeunload event handler is not allowed.");
+jsTestIsAsync = true;
+
+if (window.testRunner)
+ testRunner.setCanOpenWindows();
+
+onload = () => {
+ w = open("resources/sync-xhr-in-beforeunload-window.html");
+ w.onload = () => {
+ setTimeout(() => {
+ w.close();
+ }, 0);
+ };
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt
new file mode 100644
index 0000000..4c3619a
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt
@@ -0,0 +1,11 @@
+main frame - has 1 onunload handler(s)
+Checks that trying to do a synchronous XHR in a unload event handler is not allowed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Doing a synchronous XHR in the unload event handler threw an exception
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload.html b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload.html
new file mode 100644
index 0000000..a5de8ae
--- /dev/null
+++ b/LayoutTests/http/tests/xmlhttprequest/sync-xhr-in-unload.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description("Checks that trying to do a synchronous XHR in a unload event handler is not allowed.");
+jsTestIsAsync = true;
+
+if (window.testRunner)
+ testRunner.setCanOpenWindows();
+
+onload = () => {
+ w = open("resources/sync-xhr-in-unload-window.html");
+ w.onload = () => {
+ setTimeout(() => {
+ w.close();
+ }, 0);
+ };
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/platform/mac-wk1/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt b/LayoutTests/platform/mac-wk1/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt
new file mode 100644
index 0000000..1bd8a43
--- /dev/null
+++ b/LayoutTests/platform/mac-wk1/http/tests/xmlhttprequest/sync-xhr-in-unload-expected.txt
@@ -0,0 +1,12 @@
+main frame - has 1 onunload handler(s)
+CONSOLE MESSAGE: line 9: XMLHttpRequest cannot load http://127.0.0.1:8000/resources/dummy.xml.
+Checks that trying to do a synchronous XHR in a unload event handler is not allowed.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Doing a synchronous XHR in the unload event handler threw an exception
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 11d21f1..3be3c29 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,42 @@
+2019-12-06 Chris Dumez <cdumez@apple.com>
+
+ Prevent synchronous XHR in beforeunload / unload event handlers
+ https://bugs.webkit.org/show_bug.cgi?id=204912
+ <rdar://problem/57676394>
+
+ Reviewed by Darin Adler.
+
+ Prevent synchronous XHR in beforeunload / unload event handlers. They are terrible for performance
+ and the Beacon API (or Fetch keepalive) are more efficient & supported alternatives.
+
+ In particular, this would cause hangs when trying to navigate away from a site or when closing
+ attempt, which would result in terrible user experience.
+
+ Chrome and Edge have expressed public support for this. Chrome has actually been testing this behavior
+ for a while now:
+ https://www.chromestatus.com/feature/4664843055398912
+
+ I added this new behavior behind an experimental feature flag, enabled by default.
+
+ Tests: http/tests/xmlhttprequest/sync-xhr-in-beforeunload.html
+ http/tests/xmlhttprequest/sync-xhr-in-unload.html
+
+ * loader/DocumentThreadableLoader.cpp:
+ (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):
+ * loader/FrameLoader.cpp:
+ (WebCore::PageLevelForbidScope::PageLevelForbidScope):
+ (WebCore::ForbidPromptsScope::ForbidPromptsScope):
+ (WebCore::ForbidPromptsScope::~ForbidPromptsScope):
+ (WebCore::ForbidSynchronousLoadsScope::ForbidSynchronousLoadsScope):
+ (WebCore::ForbidSynchronousLoadsScope::~ForbidSynchronousLoadsScope):
+ (WebCore::FrameLoader::dispatchUnloadEvents):
+ (WebCore::FrameLoader::dispatchBeforeUnloadEvent):
+ * page/Page.cpp:
+ (WebCore::Page::forbidSynchronousLoads):
+ (WebCore::Page::allowSynchronousLoads):
+ (WebCore::Page::areSynchronousLoadsAllowed):
+ * page/Page.h:
+
2019-12-06 Antti Koivisto <antti@apple.com>
Support for resolving highlight pseudo element style
diff --git a/Source/WebCore/loader/DocumentThreadableLoader.cpp b/Source/WebCore/loader/DocumentThreadableLoader.cpp
index f5e5515..3d26893 100644
--- a/Source/WebCore/loader/DocumentThreadableLoader.cpp
+++ b/Source/WebCore/loader/DocumentThreadableLoader.cpp
@@ -55,6 +55,7 @@
#include "RuntimeApplicationChecks.h"
#include "RuntimeEnabledFeatures.h"
#include "SecurityOrigin.h"
+#include "Settings.h"
#include "SharedBuffer.h"
#include "SubresourceIntegrity.h"
#include "SubresourceLoader.h"
@@ -131,6 +132,11 @@
// Setting a referrer header is only supported in the async code path.
ASSERT(m_async || m_referrer.isEmpty());
+ if (document.settings().disallowSyncXHRDuringPageDismissalEnabled() && !m_async && (!document.page() || !document.page()->areSynchronousLoadsAllowed())) {
+ logErrorAndFail(ResourceError(errorDomainWebKitInternal, 0, request.url(), "Synchronous loads are not allowed at this time"));
+ return;
+ }
+
// Referrer and Origin headers should be set after the preflight if any.
ASSERT(!request.hasHTTPReferrer() && !request.hasHTTPOrigin());
diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
index e105be0..0f1f281 100644
--- a/Source/WebCore/loader/FrameLoader.cpp
+++ b/Source/WebCore/loader/FrameLoader.cpp
@@ -210,22 +210,46 @@
return frame.document() && frame.document()->isSandboxed(mask);
}
-struct ForbidPromptsScope {
- ForbidPromptsScope(Page* page) : m_page(page)
+class PageLevelForbidScope {
+protected:
+ explicit PageLevelForbidScope(Page* page)
+ : m_page(makeWeakPtr(page))
{
- if (!m_page)
- return;
- m_page->forbidPrompts();
+ }
+
+ ~PageLevelForbidScope() = default;
+
+ WeakPtr<Page> m_page;
+};
+
+struct ForbidPromptsScope : public PageLevelForbidScope {
+ explicit ForbidPromptsScope(Page* page)
+ : PageLevelForbidScope(page)
+ {
+ if (m_page)
+ m_page->forbidPrompts();
}
~ForbidPromptsScope()
{
- if (!m_page)
- return;
- m_page->allowPrompts();
+ if (m_page)
+ m_page->allowPrompts();
+ }
+};
+
+struct ForbidSynchronousLoadsScope : public PageLevelForbidScope {
+ explicit ForbidSynchronousLoadsScope(Page* page)
+ : PageLevelForbidScope(page)
+ {
+ if (m_page)
+ m_page->forbidSynchronousLoads();
}
- Page* m_page;
+ ~ForbidSynchronousLoadsScope()
+ {
+ if (m_page)
+ m_page->allowSynchronousLoads();
+ }
};
class FrameLoader::FrameProgressTracker {
@@ -3298,6 +3322,7 @@
// We store the frame's page in a local variable because the frame might get detached inside dispatchEvent.
ForbidPromptsScope forbidPrompts(m_frame.page());
+ ForbidSynchronousLoadsScope forbidSynchronousLoads(m_frame.page());
IgnoreOpensDuringUnloadCountIncrementer ignoreOpensDuringUnloadCountIncrementer(m_frame.document());
if (m_didCallImplicitClose && !m_wasUnloadEventEmitted) {
@@ -3378,6 +3403,7 @@
{
SetForScope<PageDismissalType> change(m_pageDismissalEventBeingDispatched, PageDismissalType::BeforeUnload);
ForbidPromptsScope forbidPrompts(m_frame.page());
+ ForbidSynchronousLoadsScope forbidSynchronousLoads(m_frame.page());
domWindow->dispatchEvent(beforeUnloadEvent, domWindow->document());
}
diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
index 2271c19..3bea9d8 100644
--- a/Source/WebCore/page/Page.cpp
+++ b/Source/WebCore/page/Page.cpp
@@ -2418,6 +2418,22 @@
return !m_forbidPromptsDepth;
}
+void Page::forbidSynchronousLoads()
+{
+ ++m_forbidSynchronousLoadsDepth;
+}
+
+void Page::allowSynchronousLoads()
+{
+ ASSERT(m_forbidSynchronousLoadsDepth);
+ --m_forbidSynchronousLoadsDepth;
+}
+
+bool Page::areSynchronousLoadsAllowed()
+{
+ return !m_forbidSynchronousLoadsDepth;
+}
+
void Page::logNavigation(const Navigation& navigation)
{
String navigationDescription;
diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
index 059056e..0171238 100644
--- a/Source/WebCore/page/Page.h
+++ b/Source/WebCore/page/Page.h
@@ -584,6 +584,10 @@
void allowPrompts();
bool arePromptsAllowed();
+ void forbidSynchronousLoads();
+ void allowSynchronousLoads();
+ bool areSynchronousLoadsAllowed();
+
void mainFrameLoadStarted(const URL&, FrameLoadType);
void setLastSpatialNavigationCandidateCount(unsigned count) { m_lastSpatialNavigationCandidatesCount = count; }
@@ -918,6 +922,7 @@
unsigned m_lastSpatialNavigationCandidatesCount { 0 };
unsigned m_forbidPromptsDepth { 0 };
+ unsigned m_forbidSynchronousLoadsDepth { 0 };
Ref<SocketProvider> m_socketProvider;
Ref<CookieJar> m_cookieJar;
diff --git a/Source/WebCore/page/Settings.yaml b/Source/WebCore/page/Settings.yaml
index a3e3526..0ea6a04 100644
--- a/Source/WebCore/page/Settings.yaml
+++ b/Source/WebCore/page/Settings.yaml
@@ -54,6 +54,10 @@
type: int
initial: 0
+disallowSyncXHRDuringPageDismissalEnabled:
+ type: bool
+ initial: true
+
# Allow clients concerned with memory consumption to set a quota on session storage
# since the memory used won't be released until the Page is destroyed.
sessionStorageQuota:
diff --git a/Source/WebKit/Shared/WebPreferences.yaml b/Source/WebKit/Shared/WebPreferences.yaml
index 835fdc8..8d9595c 100644
--- a/Source/WebKit/Shared/WebPreferences.yaml
+++ b/Source/WebKit/Shared/WebPreferences.yaml
@@ -18,6 +18,13 @@
condition: ENABLE(DEVICE_ORIENTATION)
webcoreName: deviceOrientationPermissionAPIEnabled
+DisallowSyncXHRDuringPageDismissalEnabled:
+ type: bool
+ defaultValue: true
+ humanReadableName: "Disallow sync XHR during page dismissal"
+ humanReadableDescription: "Disallow synchronous XMLHttpRequest during page dismissal"
+ category: experimental
+
JavaScriptEnabled:
type: bool
defaultValue: true
