Closed Bug 1345457 Opened 7 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox68 --- fixed

People

(Reporter: annevk, Assigned: twisniewski)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Bug 1345457 - Fire up an upload abort event when mFlagHadUploadListenersOnSend is true
8.51 KB, patch
baku
: review+
Details | Diff | Splinter Review
Bug 1345457 - Assert upload.abort and upload.loadend events before abort
1.57 KB, patch
annevk
: review+
Details | Diff | Splinter Review
Bug 1345457 - correct the xhr/open-during-abort-processing.htm web platform test to match the spec; r?annevk
47 bytes, text/x-phabricator-request
Details | Review
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
Closed: 7 years ago
Resolution: --- → WORKSFORME
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)
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)
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.
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
https://hg.mozilla.org/mozilla-central/rev/40b781f6504c
https://hg.mozilla.org/mozilla-central/rev/936c6efde4e4
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: --- → fixed
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)
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: shawnjohnjr → nobody
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

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

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: nobody → twisniewski
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

https://hg.mozilla.org/mozilla-central/rev/75b190178c5a

Status: REOPENED → RESOLVED
Closed: 7 years ago → 5 years ago
status-firefox68: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: