New issue
Advanced search Search tips

Issue 879620 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

window.stop() XMLHttpRequest should fire abort event asynchronously

Project Member Reported by timothygu@chromium.org, Aug 31

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.


 
Cc: domenic@chromium.org foolip@chromium.org
Owner: timothygu@chromium.org
Status: Started (was: Available)
Components: -Blink>Loader
I'm happy to review your change.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment