window.stop() should fire abort events on XMLHttpRequest asynchronously #14406

cdumez · 2023-05-26T22:26:57Z

`02e1d3e`

window.stop() should fire abort events on XMLHttpRequest asynchronously
https://bugs.webkit.org/show_bug.cgi?id=257397

Reviewed by Darin Adler and Youenn Fablet.

window.stop() should fire an abort event on XMLHttpRequest asynchronously, to match
the behavior of Blink and Gecko.

It should schedule a asynchronous task, which should get cancelled if the
JavaScript calls `open()` on the XMLHttpRequest again. This is covered by WPT
tests.

* LayoutTests/imported/w3c/web-platform-tests/xhr/abort-after-stop.window-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/open-after-stop.window-expected.txt:
Rebaseline tests now that more checks are passing.

* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-basic-cors-not-enabled-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-basic-cors-not-enabled.htm:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-basic-setrequestheader-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-basic-setrequestheader.htm:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-setrequestheader-no-cred-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-setrequestheader-no-cred.htm:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-network-error-sync-events.sub-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/xhr/send-network-error-sync-events.sub.htm:
Fix tests to use get_host_info() instead of hardcoding custom domains which don't work with the
WebKit layout tests infrastructure. This improves test coverage. I'll upstream those changes.

* Source/WebCore/xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::XMLHttpRequest):
(WebCore::XMLHttpRequest::open):
(WebCore::XMLHttpRequest::abort):
(WebCore::XMLHttpRequest::internalAbort):
(WebCore::XMLHttpRequest::abortError):
(WebCore::XMLHttpRequest::handleCancellation):
(WebCore::XMLHttpRequest::didFail):
* Source/WebCore/xml/XMLHttpRequest.h:
Align our behavior with Blink and Gecko.

Canonical link: https://commits.webkit.org/264765@main

c67bf03

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
❌ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-26T22:33:35Z

EWS run on previous version of this PR (hash 8801765)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-27T03:27:49Z

EWS run on previous version of this PR (hash 646a595)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-30T17:32:26Z

EWS run on previous version of this PR (hash 304b06f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

youennf

r=me.
Please land the WPT PR first though.

youennf · 2023-05-31T06:07:36Z

Source/WebCore/page/LocalDOMWindow.cpp

@@ -1149,6 +1149,7 @@ void LocalDOMWindow::stop()
    if (!frame)
        return;

+    SetForScope isStopped { m_isStopping, true };


isStopping?
Also, shouldn't we need to write it as m_isStopping, true, false?

isStopping indeed, will fix.

With regards to your other comment, I don't think so. My version is a little safer in case of re-entrancy because it will restore whatever value m_isStopping had before (which is likely false but not necessarily in case we re-enter this function).

youennf · 2023-05-31T06:12:34Z

Source/WebCore/xml/XMLHttpRequest.cpp

@@ -682,7 +681,7 @@ void XMLHttpRequest::abort()
 {
    Ref<XMLHttpRequest> protectedThis(*this);

-    m_wasAbortedByClient = true;
+    m_wasAbortedByClient = false;


The web page is calling abort() so it seems m_wasAbortedByClient should be true.
If we change the meaning of m_wasAbortedByClient, we should change its name.
In this case, maybe we can remove it altogether.

You're right, not sure what happened here. I'll restore.

youennf · 2023-05-31T06:21:26Z

Source/WebCore/xml/XMLHttpRequest.cpp

+        if (auto* window = document->domWindow())
+            m_wasAbortedByClient |= window->isStopping();
+    }
+
    // The XHR specification says we should only fire an abort event if the cancelation was requested by the client.
    if (m_wasAbortedByClient && error.isCancellation()) {


With this patch, m_wasAbortedByClient is always false, except in this method, and it is the only place we check it. Can we use a local variable?

While it’s non-obvious, I studied the code and verified that @youennf is right about this. No other code is looking at m_wasAbortedByClient, and it will always be false entering this function, so we don’t need m_wasAbortedByClient any more.

It is set to true in abort().

But abort then unconditionally calls immediateAbort. That sets m_error unconditionally. So we will return early in that case and after the early return we are guaranteed that m_wasAbortedByClient is false. So I am sure we can get the same behavior without a data member.

Oh, I see. Thanks for clarifying. I'll look into it now.

youennf · 2023-05-31T06:22:05Z

Source/WebCore/xml/XMLHttpRequest.cpp

@@ -889,18 +889,28 @@ String XMLHttpRequest::statusText() const
    return m_response.httpStatusText();
 }

-void XMLHttpRequest::didFail(const ResourceError& error)
+void XMLHttpRequest::handleCancellation(const ResourceError&)


Why having a const ResourceError&?

cdumez · 2023-05-31T15:36:06Z

Upstream PR: web-platform-tests/wpt#40325

webkit-early-warning-system · 2023-05-31T15:47:19Z

EWS run on previous version of this PR (hash 04acea0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/xml/XMLHttpRequest.cpp

darinadler · 2023-05-31T16:22:25Z

Source/WebCore/xml/XMLHttpRequest.cpp

+        if (auto* window = document->domWindow())
+            m_wasAbortedByClient |= window->isStopping();
+    }
+
    // The XHR specification says we should only fire an abort event if the cancelation was requested by the client.
    if (m_wasAbortedByClient && error.isCancellation()) {


While it’s non-obvious, I studied the code and verified that @youennf is right about this. No other code is looking at m_wasAbortedByClient, and it will always be false entering this function, so we don’t need m_wasAbortedByClient any more.

annevk · 2023-05-31T17:04:45Z

LayoutTests/imported/w3c/web-platform-tests/xhr/send-network-error-sync-events.sub.htm

@@ -17,7 +17,7 @@
        {
            var xhr = new XMLHttpRequest();

-            xhr.open("POST", "http://nonexistent.{{host}}:{{ports[http][0]}}", false);
+            xhr.open("POST", "http://{{host}}:3", false); // Bad port.


3 is not a bad port as far as I can tell from https://fetch.spec.whatwg.org/#bad-port.

Thanks, fixed!

webkit-early-warning-system · 2023-05-31T17:13:28Z

EWS run on previous version of this PR (hash 3d0dcaa)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	✅ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🧪 gtk-wk2~~
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc-arm64	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 tv-sim
	⏳ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-05-31T17:25:11Z

EWS run on current version of this PR (hash c67bf03)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 tv-sim
❌ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=257397 Reviewed by Darin Adler and Youenn Fablet. window.stop() should fire an abort event on XMLHttpRequest asynchronously, to match the behavior of Blink and Gecko. It should schedule a asynchronous task, which should get cancelled if the JavaScript calls `open()` on the XMLHttpRequest again. This is covered by WPT tests. * LayoutTests/imported/w3c/web-platform-tests/xhr/abort-after-stop.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/open-after-stop.window-expected.txt: Rebaseline tests now that more checks are passing. * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-basic-cors-not-enabled-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-basic-cors-not-enabled.htm: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-basic-setrequestheader-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-basic-setrequestheader.htm: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-setrequestheader-no-cred-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-authentication-cors-setrequestheader-no-cred.htm: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-network-error-sync-events.sub-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/xhr/send-network-error-sync-events.sub.htm: Fix tests to use get_host_info() instead of hardcoding custom domains which don't work with the WebKit layout tests infrastructure. This improves test coverage. I'll upstream those changes. * Source/WebCore/xml/XMLHttpRequest.cpp: (WebCore::XMLHttpRequest::XMLHttpRequest): (WebCore::XMLHttpRequest::open): (WebCore::XMLHttpRequest::abort): (WebCore::XMLHttpRequest::internalAbort): (WebCore::XMLHttpRequest::abortError): (WebCore::XMLHttpRequest::handleCancellation): (WebCore::XMLHttpRequest::didFail): * Source/WebCore/xml/XMLHttpRequest.h: Align our behavior with Blink and Gecko. Canonical link: https://commits.webkit.org/264765@main

webkit-commit-queue · 2023-05-31T23:27:02Z

Committed 264765@main (02e1d3e): https://commits.webkit.org/264765@main

Reviewed commits have been landed. Closing PR #14406 and removing active labels.

cdumez self-assigned this May 26, 2023

cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 26, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 27, 2023

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 27, 2023

cdumez force-pushed the xhr_async_abortEvent branch from 8801765 to 646a595 Compare May 27, 2023 03:14

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 27, 2023

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 30, 2023

cdumez force-pushed the xhr_async_abortEvent branch from 646a595 to 304b06f Compare May 30, 2023 17:29

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 30, 2023

cdumez marked this pull request as ready for review May 30, 2023 19:06

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 30, 2023

cdumez requested review from achristensen07, youennf and darinadler May 30, 2023 19:07

youennf approved these changes May 31, 2023

View reviewed changes

cdumez force-pushed the xhr_async_abortEvent branch from 304b06f to 04acea0 Compare May 31, 2023 15:47

darinadler approved these changes May 31, 2023

View reviewed changes

annevk reviewed May 31, 2023

View reviewed changes

cdumez force-pushed the xhr_async_abortEvent branch from 04acea0 to 3d0dcaa Compare May 31, 2023 17:13

cdumez force-pushed the xhr_async_abortEvent branch from 3d0dcaa to c67bf03 Compare May 31, 2023 17:24

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 31, 2023

cdumez removed the merging-blocked Applied to prevent a change from being merged label May 31, 2023

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 31, 2023

cdumez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 31, 2023

webkit-commit-queue force-pushed the xhr_async_abortEvent branch from c67bf03 to 0c53c73 Compare May 31, 2023 23:23

webkit-commit-queue force-pushed the xhr_async_abortEvent branch from 0c53c73 to 02e1d3e Compare May 31, 2023 23:27

webkit-commit-queue merged commit 02e1d3e into WebKit:main May 31, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

window.stop() should fire abort events on XMLHttpRequest asynchronously #14406

window.stop() should fire abort events on XMLHttpRequest asynchronously #14406

cdumez commented May 26, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 26, 2023 •

edited

webkit-early-warning-system commented May 27, 2023 •

edited

webkit-early-warning-system commented May 30, 2023 •

edited

youennf left a comment

youennf May 31, 2023

cdumez May 31, 2023

youennf May 31, 2023

cdumez May 31, 2023

youennf May 31, 2023

darinadler May 31, 2023

cdumez May 31, 2023

darinadler May 31, 2023 •

edited

cdumez May 31, 2023

youennf May 31, 2023

cdumez May 31, 2023

cdumez commented May 31, 2023

webkit-early-warning-system commented May 31, 2023 •

edited

darinadler May 31, 2023

annevk May 31, 2023

cdumez May 31, 2023

webkit-early-warning-system commented May 31, 2023 •

edited

webkit-early-warning-system commented May 31, 2023 •

edited

webkit-commit-queue commented May 31, 2023

window.stop() should fire abort events on XMLHttpRequest asynchronously #14406

window.stop() should fire abort events on XMLHttpRequest asynchronously #14406

Conversation

cdumez commented May 26, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented May 26, 2023 • edited

webkit-early-warning-system commented May 27, 2023 • edited

webkit-early-warning-system commented May 30, 2023 • edited

youennf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darinadler May 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdumez commented May 31, 2023

webkit-early-warning-system commented May 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 31, 2023 • edited

webkit-early-warning-system commented May 31, 2023 • edited

webkit-commit-queue commented May 31, 2023

cdumez commented May 26, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented May 26, 2023 •

edited

webkit-early-warning-system commented May 27, 2023 •

edited

webkit-early-warning-system commented May 30, 2023 •

edited

darinadler May 31, 2023 •

edited

webkit-early-warning-system commented May 31, 2023 •

edited

webkit-early-warning-system commented May 31, 2023 •

edited

webkit-early-warning-system commented May 31, 2023 •

edited