window.stop() XMLHttpRequest should fire abort event asynchronously |
||||
Issue description
Chrome Version: (copy from chrome://version) 70.0.3537.0 (Official Build) canary (64-bit)
OS: (e.g. Win10, MacOS 10.12, etc...) macOS 10.13.6
What steps will reproduce the problem?
<script>
const x = new XMLHttpRequest();
x.open('GET', 'data:text/plain,abc');
x.send();
x.onabort = () => { console.log("abort"); };
window.stop();
console.log("done");
</script>
What is the expected result?
"abort" is printed after "done". This is the case in Firefox.
What happens instead?
"abort" is printed before "done".
Unlike XMLHttpRequest's abort() method [1], window.stop() per spec does not have the synchronous abort event firing behavior. Instead, it just aborts the fetch, which bubbles down to "Queue a fetch task on |request| to process response for |response|" [2]. The "queue a fetch task" implies that the "process response" algorithm is to be run asynchronously. Then, the "process response" algorithm in XHR will fire the abort event [3][4].
Even more significant is that fact that firing the abort event synchronously allows an entire attack surface to be exposed. Issues like bug 879366 is an exemplification of a crash that uses the fact that abort event is fired synchronously to detach the frame while some unsuspecting code that depends on the frame is running, thus wreaking havoc. In https://crrev.com/c/1198022, japhet@ mentioned "XHR onabort strikes again!" suggesting that this is a real systematic issue, and bug 879366 is not alone.
While we should always have fixes for that class of bugs specifically, it might be worthwhile to align with Firefox and the spec and just fire the abort event asynchronously from places other than XMLHttpRequest's abort() method.
[1]: https://xhr.spec.whatwg.org/#dom-xmlhttprequest-abort
[2]: https://fetch.spec.whatwg.org/#ref-for-process-response%E2%91%A0
[3]: https://xhr.spec.whatwg.org/#ref-for-process-response
[4]: https://xhr.spec.whatwg.org/#handle-errors
Please use labels and text to provide additional information.
If this is a regression (i.e., worked before), please consider using the
bisect tool (https://www.chromium.org/developers/bisect-builds-py) to help
us identify the root cause and more rapidly triage the issue.
For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.
,
Sep 6
,
Sep 6
,
Sep 6
I'm happy to review your change.
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/177405e7c668ac98062daf7d48361b38ec0db283 commit 177405e7c668ac98062daf7d48361b38ec0db283 Author: Timothy Gu <timothygu@chromium.org> Date: Fri Sep 07 21:03:21 2018 Fire XHR abort event from a task A test for this new behavior is included as external/wpt/xhr/abort-after-stop.htm. This also means that XHR abort and readystatechange events from navigation is no longer fired. Because of that, external/wpt/xhr/open-url-multi-window-4.htm now times out rather than fails on an assertion, aligning with Firefox. A TestExpectations entry has been added with a bug that tracks fixing this test in spec or upstream in WPT. A few other test expectations were updated as well to account for this. http/tests/navigation/reentrant-xhr-onabort-crash-during-commit.html no longer works as intended, as abort event is no longer fired synchronously (and thus does not expose the same crash surface). It is replaced with an EventSource-based test that functions the same way as XHR before this CL. Note, currently EventSource's error event suffers from the same problem as XHR's abort event, and the class might undergo the same change as this in the future. We will look into an alternative for this test when that change is done. Bug: 879620 , 881180 Change-Id: I5a91047086d06347794656f92511a53c22401b5e Reviewed-on: https://chromium-review.googlesource.com/1208672 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Timothy Gu <timothygu@chromium.org> Cr-Commit-Position: refs/heads/master@{#589634} [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/external/wpt/xhr/abort-after-stop.htm [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/external/wpt/xhr/open-after-abort.htm [add] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/external/wpt/xhr/open-after-stop.window.js [delete] https://crrev.com/34610c1dc8f4716d2d088a766f4eaad54c7a8b02/third_party/WebKit/LayoutTests/external/wpt/xhr/open-url-multi-window-4-expected.txt [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt [add] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/navigation/reentrant-eventsource-onerror-crash-during-commit-expected.txt [add] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/navigation/reentrant-eventsource-onerror-crash-during-commit.html [delete] https://crrev.com/34610c1dc8f4716d2d088a766f4eaad54c7a8b02/third_party/WebKit/LayoutTests/http/tests/navigation/reentrant-xhr-onabort-crash-during-commit-expected.txt [delete] https://crrev.com/34610c1dc8f4716d2d088a766f4eaad54c7a8b02/third_party/WebKit/LayoutTests/http/tests/navigation/reentrant-xhr-onabort-crash-during-commit.html [add] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/navigation/resources/event-stream-with-delay.php [add] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/navigation/resources/reentrant-eventsource-onerror-crash-during-commit-iframe.html [delete] https://crrev.com/34610c1dc8f4716d2d088a766f4eaad54c7a8b02/third_party/WebKit/LayoutTests/http/tests/navigation/resources/reentrant-xhr-onabort-crash-during-commit-iframe.html [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/frame-load-cancelled-abort-expected.txt [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/frame-unload-abort-crash-expected.txt [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/navigation-should-abort-expected.txt [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/navigation-should-abort.html [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc [modify] https://crrev.com/177405e7c668ac98062daf7d48361b38ec0db283/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.h
,
Sep 11
|
||||
►
Sign in to add a comment |
||||
Comment 1 by timothygu@chromium.org
, Aug 31