Closed Bug 1400748 Opened 7 years ago Closed 7 years ago

Aborting XHR at readyState 1 incorrectly works, and at 2 and 3 fails to set status to 0.

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
57 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: dfshepsis, Assigned: wisniewskit)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1400748 - Correct our handling of XHR.abort edge-cases;
(deleted), text/x-review-board-request
baku
: review+
Details
Bug 1400748 - add web platform tests coverage for what status/statusText/readyState should be in various abort() edge cases;
(deleted), text/x-review-board-request
annevk
: review+
baku
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce:

See my demo here: https://jsfiddle.net/qqh1kjg8/8/ . To use this, simply open your developer tools and read the messages. You can check different radio buttons to see what happens when the request is aborted at different readystates. Of concern are the results when aborting at 1, 2, and 3, which differ from Chrome and, apparently, the MDN.

I made an XHR and aborted it conditionally based on information in the headers (that is, during readyState === 2 (HEADERS_RECEIVED).

The same issue occurs when aborting during readyState === 3 (LOADING). 

A different issue occurs for readyState === 1 (OPENED).


Actual results:

For readyState === 1 (OPENED), the request is aborted. The readyState is set to 4 and the status is set to 0 (or, rather, it remains unchanged).

For readyState === 2 (HEADERS_RECEIVED)  the readyState is set to 4, but **the status remains at 200**. In my demo, this causes the normal-response-handling part of the request's onreadystatechange function to receive a null response, which then leads to the console.error line.

For readyState === 3 (LOADING) the same thing occurs.

readyStates 0 and 4 behave as expected.


Expected results:

For readyState 1, nothing should occur. The request should not be aborted as it has not been sent (source: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort "[...]aborts the request if it has already been sent.").

For readyStates 2 and 3, the request's status should be set to 0 rather than 200.

Run the demo on Chrome to observe the correct behavior (at least as I understand it, according to MDN).
Component: Untriaged → DOM: Events
baku, please take a look.
Component: DOM: Events → DOM
Flags: needinfo?(amarchesini)
Thomas, do you have time for this? If not, NI me again and I'll work on it.
Flags: needinfo?(amarchesini) → needinfo?(wisniewskit)
Sure, I have a few cycles today and this looks pretty simple. I'll make a patch with new web platform tests.
Flags: needinfo?(wisniewskit)
Blocks: xhr
Comment on attachment 8911491 [details]
Bug 1400748 - Correct our handling of XHR.abort edge-cases;

https://reviewboard.mozilla.org/r/182938/#review188240

::: dom/xhr/XMLHttpRequestWorker.cpp:2175
(Diff revision 1)
>    }
>  
> +  // Set our status to 0 and statusText to "" if we
> +  // will be aborting an ongoing fetch, so the upcoming
> +  // abort events we dispatch have the correct info.
> +  if ((mStateData.mReadyState == 1 && mStateData.mFlagSend) ||

can you use State::something here?
Attachment #8911491 - Flags: review?(amarchesini) → review+
Comment on attachment 8911492 [details]
Bug 1400748 - add web platform tests coverage for what status/statusText/readyState should be in various abort() edge cases;

https://reviewboard.mozilla.org/r/182940/#review188270

Mostly nits and one potentially substative thing since I don't know about the async_test style you're using. LGTM otherwise. r+ with nits/feedback addressed.

::: testing/web-platform/tests/XMLHttpRequest/abort-during-done.htm:12
(Diff revision 1)
>      <link rel="help" href="https://xhr.spec.whatwg.org/#the-abort()-method" data-tested-assertations="following-sibling::ol/li[4] following-sibling::ol/li[5]" />
>    </head>
>    <body>
>      <div id="log"></div>
>      <script>
> -      var test = async_test()
> +      async_test(document.title + " (sync)").step(function() {

I would prefer async_test(t => { ... }, title) and then use t.step_func whenever needed inside. I'm not familiar with the variant you're using here. Or is this documented too somewhere?

::: testing/web-platform/tests/XMLHttpRequest/abort-during-done.htm:82
(Diff revision 1)
> +              assert_equals(client.statusText, "")
> +              assert_equals(client.responseXML, null)
> +              assert_equals(client.getAllResponseHeaders(), "")
> +              assert_array_equals(result, expected)
> +              this.done();
> +            }.bind(this), 100); // wait a bit in case XHR timeout causes spurious event

Could you make the asserts synchronously and the .done() after a timeout?

::: testing/web-platform/tests/XMLHttpRequest/abort-during-headers-received.htm:46
(Diff revision 1)
> +            assert_equals(client.statusText, "")
> +            assert_equals(client.responseXML, null)
> +            assert_equals(client.getAllResponseHeaders(), "")
> +            assert_array_equals(result, expected)
> +            this.done();
> +          }.bind(this), 100); // wait a bit in case XHR timeout causes spurious event

Same here, better to have the asserts synchronous I think and delay the done().

::: testing/web-platform/tests/XMLHttpRequest/abort-during-loading.htm:47
(Diff revision 1)
> +            assert_equals(client.responseXML, null)
> +            assert_equals(client.getAllResponseHeaders(), "")
> +            assert_array_equals(result, expected)
> +            this.done();
> +          }.bind(this), 100); // wait a bit in case XHR timeout causes spurious event
> +        })

Same.

::: testing/web-platform/tests/XMLHttpRequest/abort-during-open.js:11
(Diff revision 1)
>      test.step(function() {
>        assert_unreached()
>      })
>    }
>    assert_equals(client.readyState, 1, "before abort()")
> +  assert_equals(client.readyState, 1)

No need to assert it twice.

::: testing/web-platform/tests/XMLHttpRequest/abort-during-open.js:16
(Diff revision 1)
> +  assert_equals(client.readyState, 1)
> +  assert_equals(client.status, 0)
> +  assert_equals(client.statusText, "")
>    client.abort()
>    assert_equals(client.readyState, 1, "after abort()")
> +  assert_equals(client.readyState, 1)

Same.

::: testing/web-platform/tests/XMLHttpRequest/abort-during-unsent.htm:27
(Diff revision 1)
> +        assert_equals(client.statusText, "")
>          client.abort()
>          assert_equals(client.readyState, 0)
> +        assert_equals(client.status, 0)
> +        assert_equals(client.statusText, "")
> +        assert_equals(client.readyState, 0)

You assert readyState twice here.
Attachment #8911492 - Flags: review?(annevk) → review+
Comment on attachment 8911491 [details]
Bug 1400748 - Correct our handling of XHR.abort edge-cases;

https://reviewboard.mozilla.org/r/182938/#review188240

> can you use State::something here?

If we want to land this before nsIXMLHttpRequest is dropped in bug 792808, I can just have it use OPENED/etc for now. If we are ok with landing 792808 first, then I will write a patch there so we can indeed use State::X here. Do you have a preference on how we should order landing these bugs?
> Do you have a preference on how we should order landing these bugs?

No. But at the end I would like to read State::Something instead of numbers. Thanks!
Ok, then I'll just adjust this patch to use the nsIXMLHttpRequest constants for now, and whichever bug lands first, I will rebase the other accordingly.
Alright, I've addressed the review comments.

A try-run only hits pre-existing intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c44f6792ae6def3f19c6088f1f4c1e6750fe7d9

Requesting check-in.
Keywords: checkin-needed
Assignee: nobody → wisniewskit
Autoland can't push this because the second patch has insufficient review in MozReview.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(wisniewskit)
Hmm, I thought Anne's review would be sufficient. Anne, who do you feel should give it a final review?
Flags: needinfo?(wisniewskit) → needinfo?(annevk)
The issue is that it needs r+ from someone with Level 3 commit access. Can baku just sign off on it too?
Baku, would you mind also signing off on the web platform test patch here so it passes l3 commit review requirements? (Anne reviewed the patch already).
Flags: needinfo?(amarchesini)
Comment on attachment 8911492 [details]
Bug 1400748 - add web platform tests coverage for what status/statusText/readyState should be in various abort() edge cases;

https://reviewboard.mozilla.org/r/182940/#review188896
Attachment #8911492 - Flags: review+
Thanks baku. Ryan, let me know if there's anything else, thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(annevk)
Flags: needinfo?(amarchesini)
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/709e623173d8
Correct our handling of XHR.abort edge-cases; r=baku
https://hg.mozilla.org/integration/autoland/rev/51f79f99b1b5
add web platform tests coverage for what status/statusText/readyState should be in various abort() edge cases; r=annevk,baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/709e623173d8
https://hg.mozilla.org/mozilla-central/rev/51f79f99b1b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Thank you for your prompt work! I do genuinely appreciate it.

I should mention that there my still be an issue, though I am uncertain. I'm using the latest version of Nightly, and I'm checking the test fiddle I posted in my initial report. Aborting at readyStates 2 and 3 now work as specified, which is great, but I think aborting at readyState 1 is still broken. Here is an updated link (only change is defaulting to readyState = 1 instead of 2): https://jsfiddle.net/qqh1kjg8/9/ .

In Chrome, I get the expected response (as previous) of the request not being aborted: https://i.imgur.com/eNKi7TK.png

In Nightly, I get the same undesired response as before, of the request being aborted: https://i.imgur.com/qvkHK7f.png

I apologize if I am mistaken here.
Thanks for following up! I agree that it's a bit subtle, but based on the spec, I think we're now doing what should be done (and Chrome is a bit off):
- we fire readystate = 1/open, which your handler receives.
- your code call abort(), and we do what the spec says to do [1]:
  - we set the readystate to 4/done, and the status to 0 (the "set to network error" part of "request error steps").
  - we fire abort and readystate events with these values.
- your code gets the readystate = 4/done, and since status is 0, it logs "Error-Handler found a killed req".
- note that we skip readystates 2 and 3 entirely, because the spec does not say to send them in this flow.

This makes sense to me intuitively, since we have not actually successfully received anything yet, so we would not have a reason to trigger readystates 2 or 3, and we have no response to base a status/etc from.

Do you feel differently about what should happen? If so, please let us know so we can decide whether to raise a spec issue, thanks!

[1] https://xhr.spec.whatwg.org/#the-abort()-method
I've looked further into it, I am not certain which behavior is correct. Let me elaborate.

The spec says this:

  1. Terminate the ongoing fetch with the aborted flag set.

  2. If state is either opened with the send() flag set, headers received, or loading, run the request error steps for event abort.

My uncertainty hinges on the definition of step 1: Terminate the ongoing fetch.

I've looked through the documentation, and it seems to me unclear. In the spec on Fetching (https://fetch.spec.whatwg.org/#ref-for-concept-fetch-terminate⑧), the doc says this:

  2. If the ongoing fetch is terminated, then:

    1. Let aborted be the termination’s aborted flag.

    2. If aborted is set, then return an aborted network error.

    3. Return a network error.

Now this does seem to imply to me that the XHR should be aborted, but it refers to the "terminations's" aborted flag, rather than the request's. I tried to look further into the spec's definition of "terminate"/"termination"/"network error", but I think it is out of my depth.

In the second step on abort() "If state is either opened with[...]", the spec seems pretty explicit that running the "request error steps for event abort" applies to requests of state=opened *only if* the send() flag is set.

I did look further into the send() flag, and as far as I can tell, it is set only by calling the request's send() method. Unfortunately, this flag doesn't seem to be directly available in the browser. However, I did make sure that the onreadystatechange event is fired for state === 1 (OPENED) before the send method is called. Moreover, the onreadystatechange event for state === 2 (HEADERS_RECEIVED) is not fired synchronously after calling send(). This makes sense, as of course there must be an undefined waiting period to send the request and receive the initial header data from the server, even if that time is usually small due to the small sizes of data being exchanged. I updated my test to confirm this: https://jsfiddle.net/qqh1kjg8/10/.

This means that, for readyState === 1, there is a period before send has been called (during which the onreadystatechange event should have fired) where the send() flag is unset, and a period after it has been called (before headers have been received) where the send flag *is* set.

The second step on abort() does refer specifically to "opened with the send() flag set", which indicates to me that a request at readystate 1 should only follow the request error steps (https://xhr.spec.whatwg.org/#request-error-steps) if abort was called *after* the send() method, but not before. And since the onreadystatechange event for readystate 1 is fired before the send() method is called, calling the abort() method from the onreadystatechange event at readystate 1 should *not* follow the request error steps (which are what set the state and response to DONE and network-error respectively).

Finally, I should mention that Nightly does not fire the onabort event when aborting at readystate 1. See https://jsfiddle.net/qqh1kjg8/11/. This is a last-minute thought while writing this, so I didn't investigate this point in so much detail.

I do hope my thought process is clear here. Again, I am rather out of my depth, so I very well may be wrong. If the spec for Fetch is actually ambiguous (rather than it just going over my head) then perhaps an issue should be raised there. 

Regardless, I would say that this behavior isn't of tremendous importance. Aborting a request before even sending it is not something I see having any application. The only reason I even discovered this discrepancy was because I included readystates 0, 1, and 4 in my initial test as a matter of completeness. Readystates 2 and 3 were my original concerns, and I am glad they have been fixed =)
---
In summary, depending on the definition of "Terminate the ongoing fetch", I think that requests at readystate 1 should be unaffected by the abort() method.
Yes, this stuff is definitely tricky, so thanks for trying to help make sense of it!

For the record, what's happening in this case is that readystatechange handler for readystate=1/open is called BEFORE send() is ever called. As such the send flag will not be set by that point, and it makes sense that we would never fire an abort event (since that would require the flag to be set).

However, also based on that, we should not be changing the response of the XHR to a network error, since the abort() method in the spec does nothing to the XHR itself in step 1 (just the underlying fetch), and we don't meet the criteria to run either steps 2 or 3. So you should be correct there.

Anne, do you agree with those assessments?
Flags: needinfo?(annevk)
Yeah, that sounds about right.
Flags: needinfo?(annevk)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: