XMLHttpRequest: not all abort events end up firing (XMLHttpRequest/open-during-abort-processing.htm)

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
28 days ago

People

(Reporter: annevk, Assigned: twisniewski)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed, firefox68 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Priority: -- → P3
If I understand correctly, we're talking about open-during-abort.htm. This test can pass. Feel free to re-open.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 2

2 years ago
No, this is about

  XMLHttpRequest/open-during-abort-processing.htm

which we fail in Nightly. We do pass

  XMLHttpRequest/open-during-abort-event.htm

which was also added by that PR, but the PR does not touch

  XMLHttpRequest/open-during-abort.htm

at all.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(In reply to Anne (:annevk) from comment #2)
> No, this is about
> 
>   XMLHttpRequest/open-during-abort-processing.htm
> 
> which we fail in Nightly. We do pass
> 
>   XMLHttpRequest/open-during-abort-event.htm
> 
> which was also added by that PR, but the PR does not touch
> 
>   XMLHttpRequest/open-during-abort.htm
> 
> at all.

Oh, I see. Thanks for your correction.
XMLHttpRequest: open() during abort processing - abort() called from onloadstart	
assert_array_equals: lengths differ, expected 9 got 7
Summary: XMLHttpRequest: not all abort events end up firing → XMLHttpRequest: not all abort events end up firing (XMLHttpRequest/open-during-abort-processing.htm)
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> XMLHttpRequest: open() during abort processing - abort() called from
> onloadstart	
> assert_array_equals: lengths differ, expected 9 got 7

So these two upload events are not there.
upload.onabort 1
upload.onloadend 1
Comment on attachment 8870370 [details] [diff] [review]
Bug 1345457 - Fire up an upload abort event when mFlagHadUploadListenersOnSend is true

Review of attachment 8870370 [details] [diff] [review]:
-----------------------------------------------------------------

Fire upload.onabort and upload.onloadend when abort() called. Original code
won't fire onabort if mUploadComplete is true. By default mUploadComplete is true, so these event won't be fired.
It breaks:
TEST-UNEXPECTED-FAIL | dom/base/test/test_bug435425.html | Wrong event target [[object XMLHttpRequestUpload],abort]!
TEST-UNEXPECTED-FAIL | /XMLHttpRequest/abort-after-send.htm | XMLHttpRequest: abort() after send() - assert_equals: expected "abort(0,0,false)" but got "upload.abort(0,0,false)"
Hi Anne,
I did not find the description regarding the xhr event order for XMLHttpRequestUpload abort.
I just want to confirm this is a issue I should raise a issue on github.

So if this test case "open-during-abort-processing" is correct.

Then we expect that the following sequence:
client.upload.onabort
client.upload.onloadend
client.onabort

However the test case "abort-after-send", do assertion:

assert_xhr_event_order_matches([1, "loadstart(0,0,false)", 4, "abort(0,0,false)", "loadend(0,0,false)"])


That assertion simply ignore upload.onabort but |assert_xhr_event_order_matches| actually register upload listeners.
I guess that test case should be also considered about upload.onabort/onloadend.

Or maybe I miss something in the specification?
Flags: needinfo?(annevk)
test_bug435425.html also made assertion to XHR without upload, that's why failure can be observed.
Fix additional tests test_bug435425.html, abort-after-send.htm.
Attachment #8872897 - Flags: review?(amarchesini) → review+
Attachment #8872899 - Flags: review?(amarchesini) → review?(annevk)
(Reporter)

Comment 18

2 years ago
Per https://xhr.spec.whatwg.org/#request-error-steps .upload.onabort fires before .onabort so I think that means you're correct.
Flags: needinfo?(annevk)
(Reporter)

Updated

2 years ago
Attachment #8872899 - Flags: review?(annevk) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #20)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=15eb00accc87e16bacdace2fb6ea5707fd1323b2

try results look good.

Comment 22

2 years ago
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40b781f6504c
Part 1: Fire up an upload abort event when mFlagHadUploadListenersOnSend is true, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/936c6efde4e4
Part 2: Assert upload.abort and upload.loadend events before abort, r=annevk

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40b781f6504c
https://hg.mozilla.org/mozilla-central/rev/936c6efde4e4
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(In reply to Anne (:annevk) from comment #18)
> Per https://xhr.spec.whatwg.org/#request-error-steps .upload.onabort fires
> before .onabort so I think that means you're correct.

Anne,
I changed abort-after-send.htm based on open-during-abort-processing.htm.
In open-during-abort-processing, it calls xhr.send(null)
https://github.com/w3c/web-platform-tests/blob/master/XMLHttpRequest/open-during-abort-processing.htm#L29
So upload complete flag is set. We shouldn't expect upload event.

Based on https://github.com/w3c/web-platform-tests/issues/6940, I'm doing things wrong here.
I wondered open-during-abort-processing.htm shall be also changed.
I think I have to backout both P1, P2 patches :(
Flags: needinfo?(annevk)
(Reporter)

Comment 25

2 years ago
Yeah, I'm sorry I missed that during review. If there's no request body there won't be any upload events.
Flags: needinfo?(annevk)
I'm wrong, I will backout everything.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

9 months ago
We're actually failing because the WPT doesn't match the spec: https://github.com/web-platform-tests/wpt/issues/7933

We *could* pass by caching some values in RequestErrorSteps:

>  bool hadUploadListenersOnSend = mFlagHadUploadListenersOnSend;
>  bool hadIncompleteUpload = mUpload && !mUploadComplete;
> 
>  // Step 5
>  FireReadystatechangeEvent();
> 
>  // Step 6
>  if (hadIncompleteUpload) {
> 
>    // Step 6-1
>    mUploadComplete = true;
> 
>    // Step 6-2
>    if (hadUploadListenersOnSend) {

But we first need to decide whether that's what should be done, which would require changing the spec to match the test. (And since only Blink passes this WPT right now, it may be that it should change instead).
Component: DOM → DOM: Core & HTML
Product: Core → Core
(Assignee)

Comment 30

a month ago

correct the xhr/open-during-abort-processing.htm web platform test to match the spec

(Assignee)

Comment 31

a month ago

Alright, it was decided in the github discussion that this WPT should be updated to match the spec. I'll do that here, since it's a trivial patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b18f30f8eb12ff05451231038160a7a7d62502cd

(Assignee)

Updated

a month ago
Assignee: nobody → twisniewski

Comment 32

a month ago
Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75b190178c5a
correct the xhr/open-during-abort-processing.htm web platform test to match the spec; r=annevk

Comment 33

a month ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 years agoa month ago
Resolution: --- → FIXED
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16010 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.