1505389 - Navigation Cancels XHR Requests and Calls Onerror Callback

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P3)

Description

[Honza Bambas (:mayhemer)]

+++ This bug was initially created as a clone of Bug #1418134 +++

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38

Steps to reproduce:

Make some XMLHttpRequests, and then before receiving a response, navigate away by setting the window location.

Plnkr here to demonstrate (run it in a new window, and use preserve log in the devtools so that you can see the logs after the redirect has happened): https://plnkr.co/edit/10q3RE0cZwvzfckphnj8


Actual results:

The requests are cancelled, and the onerror callback and the onloadend callback are called on the XMLHttpRequest.


Expected results:

The requests should be cancelled, however Safari and Chrome on macOS don't call the onerror callback, they just call the onloadend callback. I believe this is the correct behaviour as onerror is reserved for low level network errors and this doesn't seem to fit the description. Plus, it is inconsistent with other browsers.

Comment 1

[Honza Bambas (:mayhemer)]

Daniel, can you please open this bug?  I made a clone of bug 1418134 and forgot to un-tick the Security-Sensitive Network Bug checkbox.  This is not a sec issue.  Thanks.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Relevant bits that didn't get copied from bug 1418134:

The relevant spec bit is starting at https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigating-across-documents step 9 which calls into https://html.spec.whatwg.org/multipage/browsing-the-web.html#abort-a-document which does:

  Cancel any instances of the fetch algorithm in the context of document

Nothing defines what "cancel" means, here or in <https://fetch.spec.whatwg.org/>.  What Gecko does is return  a "network error" response in the sense of https://fetch.spec.whatwg.org/#concept-network-error and that of course triggers the "error" event on the XHR.  Since the behavior _does_ look like a network error from the point of view of the page (akin to a connection drop), this looks appropriate to me.

Anne says:

I think this is basically https://github.com/whatwg/html/issues/4007, ergo this requires tests, coordination with other browsers, and someone to come up with a plan for desirable behavior.

(As for intent, I think it's roughly what Firefox is doing, though perhaps with the "abort" event rather than "error", but that intent was from a time when investigations into behavior were far less thorough and understanding of abuse was more naïve. Not that I'd classify this as a security issue though.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I think this is basically https://github.com/whatwg/html/issues/4007

I don't think so.  The events in question are firing while the document is still active.

Comment 4

[Anne (:annevk)]

Apologies, though I think my "ergo" is still apt (i.e., this needs investigation), but I cannot find the canonical issue here (https://github.com/whatwg/html/issues/3837 seems related). Also, from what I recall browsers are not aligned on the quoted step above and some browsers terminate the fetches at a later stage at which point they do not update the document in question.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  Given that, I don't think this should be P2 or major, since we're following the current spec here.  In particular, I don't think we should make changes here unless and until a different behavior is clearly agreed on in the spec.

Comment 6

[Honza Bambas (:mayhemer)]

P2 carried from the original bug.  P3 based on comment 5

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

With the revised reproducer I see only the onabort callback is called instead of onerror. https://plnkr.co/edit/GViDNXbJzbEDBT2E

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Initially I thought this would be as easy as detecting whether the global is dying or not. But apparently the global only dies shortly after the abort event is dispatched via XMLHttpRequestMainThread::OnStopRequest, and thus nsIGlobalObject::IsDying gives false. Still something should be triggering the network abort during the teardown and we should be able to read some state from there, but I'm not familiar about that part.

Maybe Kershaw knows better.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1505389 - Remove redundant XMLHttpRequestMainThread::mContext r=#necko-reviewers

Comment 11

[Phabricator Automation]

Comment on attachment 9501259 [details]
Bug 1505389 - Remove redundant XMLHttpRequestMainThread::mContext r=#necko-reviewers

Revision D257685 was moved to bug 1977642. Setting attachment 9501259 [details] to obsolete.

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #9)

Initially I thought this would be as easy as detecting whether the global is dying or not. But apparently the global only dies shortly after the abort event is dispatched via XMLHttpRequestMainThread::OnStopRequest, and thus nsIGlobalObject::IsDying gives false. Still something should be triggering the network abort during the teardown and we should be able to read some state from there, but I'm not familiar about that part.

Maybe Kershaw knows better.

Sorry, not familiar with this code.
Maybe Sunil can help?

Comment 13

[Sunil Mayya]

I will take this

Comment 14

[Greg Hess]

moving to priority queue as blocking P1 webcompat issue.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

My apologies Sunil; I wanted to get the webcompat issue out of the way and so took a shot a this

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1505389: Don't make navigation block XHR and call onerror/abort r=#necko-reviewers!

Fires readystatechange and loadend intead of abort

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b59d39283fb8
https://hg.mozilla.org/integration/autoland/rev/50784aac4b40
Don't make navigation block XHR and call onerror/abort r=necko-reviewers,valentin,smaug

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cacbfc165992
https://hg.mozilla.org/integration/autoland/rev/9385cee19c9b
Revert "Bug 1505389: Don't make navigation block XHR and call onerror/abort r=necko-reviewers,valentin,smaug" for causing wpt failures at XMLHttpRequest

Comment 19

[Silaghi Andreea]

Backed out for causing wpt failures atXMLHttpRequest
Backout Link
Push with failures
Failure Log
Failure line TEST-UNEXPECTED-FAIL | /xhr/abort-after-stop.window.html | XMLHttpRequest : abort event should fire when stop() method is used - assert_equals: expected true but got false

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8fd7383628ad
https://hg.mozilla.org/integration/autoland/rev/167faaa0aec2
Don't make navigation block XHR and call onerror/abort r=necko-reviewers,valentin,smaug

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/167faaa0aec2

Comment 22

[Sunil Mayya]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #15)

My apologies Sunil; I wanted to get the webcompat issue out of the way and so took a shot a this

No worries Randell, this unblocks Bug 833462 and Bug 1732512 for me. thank you

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/59603 for changes under testing/web-platform/tests

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot