XHR open-during-abort-processing.htm test is incorrect per the current spec-text.
Opened this issue · 30 comments
In its onloadstart handler, the test calls abort. This triggers abort step 2, running the request error steps. Step 5 of those steps fires a readystatechange, which triggers the test's readystatechange handler, which calls open and send. By the time that's done and the request error steps resume at step 6, the upload complete flag will have been reset to false, and step 6 will not be able to fire upload.abort or upload.loadend as the test expects.
Given that Chrome is the only engine currently passing the test, I feel the test is in error. But if we would like to change the spec to match that behavior, we will need to store the state of the upload complete and upload listener flags used in request error steps 6 before firing the readystatechange, in case they are changed by that readystatechange (as is happening in this test).
In addition, the test is also expecting too few events, as there will be a second upload.onloadend for the second open-and-send, because there is still an upload-related event handler attached to the object (for the check in send() step 5). The test should either remove that listener before doing the second send, or expect the second event, or the spec needs to change send() step 5 to also take into account whether there is a body actually being sent (which seems the most reasonable change to me, but I don't know if that would cause interop issues).
@annevk, please let me know if you agree with the assessment here, or if you believe I'm misinterpreting something.
Sounds about right. @yutakahirano might be interested.
@yutakahirano, ping?
@annevk, it's been quite a while with no response; should we ping someone else about this?
https://wpt.fyi/results/xhr/open-during-abort-processing.htm?label=experimental shows that Edge is now also passing the test.
Comparing that to https://wpt.fyi/results/xhr/open-during-abort-processing.htm?label=stable, it's interesting that Firefox and Safari are now failing in the same way, where previously they did not. (Safari has changed.)
@wisniewskit, how did you come across this, is it a compat issue that has affected real web sites?
FWIW, I think I'd prefer to keep the specification as-is, as changing it is always rather involved, and file bugs against Chrome and Edge.
@thejohnjansen might you be able to help figure out why Edge has apparently changed here?
@foolip, I found this simply because I was trying to explain why Gecko was failing this, as it seemed to be following the spec. I'm unaware of any live sites being affected, but I feel it's still an interop issue worth resolving.
Sorry that I didn't notice. I will take a look.
In its onloadstart handler, the test calls abort. This triggers abort step 2, running the request error steps. Step 5 of those steps fires a readystatechange, which triggers the test's readystatechange handler, which calls open and send. By the time that's done and the request error steps resume at step 6, the upload complete flag will have been reset to false, and step 6 will not be able to fire upload.abort or upload.loadend as the test expects.
I'm confused. step 6 says
If the upload complete flag is unset, follow these substeps:
- Set the upload complete flag.
- If upload listener flag is unset, then terminate these substeps.
- Fire a progress event named event on the XMLHttpRequestUpload object with 0 and 0.
- Fire a progress event named loadend on the XMLHttpRequestUpload object with 0 and 0.
So we should fire upload.abort and upload.loadend if the upload complete flag is false, right?
So we should fire upload.abort and upload.loadend if the upload complete flag is false, right?
Oh! Sorry for the typo. I meant to say that the flag will be set to true, as during step 5 of request error steps the test's readystatechange handler calls client.send(null), which causes send step 9 to set the flag to true (since the req's body is null). So by the time we resume at step 6, it will see the flag is true, and not fire the events as the test expects.
I see. Then, as the upload complete flag is set (spec-wise),
In addition, the test is also expecting too few events, as there will be a second upload.onloadend for the second open-and-send, because there is still an upload-related event handler attached to the object (for the check in send() step 5). The test should either remove that listener before doing the second send, or expect the second event, or the spec needs to change send() step 5 to also take into account whether there is a body actually being sent (which seems the most reasonable change to me, but I don't know if that would cause interop issues).
on this point the test is correct, right?
@yutakahirano, yes, looking over that paragraph again, I think you're right.
The test is basically just mis-expecting those events from steps 6 that won't happen (abort and loadend on the upload). So the test should not be expecting those events.
That is, per spec the test should get this:
"onloadstart readyState before abort() 1"
"onreadystatechange readyState before open() 4"
"onreadystatechange readyState after open() 1"
"onloadstart readyState 1"
"client.onabort 1"
"readyState after abort() 1"
"client.onload 4"
Whereas it expects this right now:
"onloadstart readyState before abort() 1"
"onreadystatechange readyState before open() 4"
"onreadystatechange readyState after open() 1"
"onloadstart readyState 1"
"upload.onabort 1" (won't fire)
"upload.onloadend 1" (won't fire)
"client.onabort 1"
"readyState after abort() 1"
"client.onload 4"
Thanks. What is missing in Chrome implementation seems
- If req’s body is null, set the upload complete flag.
in https://xhr.spec.whatwg.org/#the-send()-method. I'm happy to change the implementation, but it would be good if I can hear Edge's voice.
@thejohnjansen, could you check if someone from the Edge team can chime in on this one?
@yutakahirano, is your concern about this issue with respect to Edge obsolete now?
Right.
I am really sorry for not responding for the last 5 months on this... It looks like I had some very poor rules setup for github notifications and I missed a LOT of them. Based on my preliminary research, it looks like we changed in order to match Chrome here. I cannot find a live site impacted, though.
I agree the concern is moot going forward given that we are Chromium based now and would not service for this without a high impact for a live site.
Ah, thanks John!
@yutakahirano, would you still rather alter Chrome to match the spec? If so, I don't mind altering this WPT to match the spec.
If you are going to make a PR to the WPT I'll be happy to review (and probably fix the implementation altogether).
@yutakahirano, just so you know the WPT change is incoming in PR #16010 (already reviewed by @annevk).
Actually, I just realized that I may have been partly wrong here, @annevk. In send() step 5, we check if there are any upload listeners, but not if there is actually anything being uploaded in the first place (if there body is null). As such we should end up sending an upload.loadend event, but not the upload.abort event.
Based on this, I think I should revise the patch in PR #16010 before we land it (or we need to consider whether it's truly correct to send any upload events if there is no body being uploaded in the first place, I suppose).
@wisniewskit I'm not sure I understand. Aren't all these events conditional upon the upload complete flag as well, which is set to true in step 9 for a null body?
Hmm. I'll take another look, maybe I'm just confusing myself.
Ah, so what's happening is that in the test-case, we hit process request end-of-body, which fires the upload.loadend event based only on whether there were upload listeners (it doesn't care whether upload-complete is true at the time, it just sets it to true).
So per spec-as-written, the upload.loadend event is supposed to fire (but not the upload.onabort).
That leaves me unsure of what to do here. Either this WPT should continue to expect upload.loadend (but not upload.abort), or the spec should presumably change to either:
- require both upload.abort and upload.loadend (which Blink and Edge seem to be doing).
- not fire any upload events if upload-complete already equals true when doing process request end-of-body (which it seems both Firefox and Safari are doing).
@annevk, I'm not sure what would be best here. To my mind if we fire any upload events in this case, we should probably fire the abort as well. But it seems the spec doesn't intend to fire any upload events in this case, yes?
I think the intention is to not fire events, right. If you fire loadend there, you'd also fire progress and load, right?
Yes, those are fired in the order given in process request end-of-body. So if we're ok with firing load instead of abort, that's what's happening right now.
If we don't want to fire any upload events on aborts, it seems that just adding a step 0.5 to process end-of-request "if the upload complete flag is set to true, return" does the trick in my reference implementation, as I would expect.
@wisniewskit so I think we should change the specification to not fire events there if that flag is set, as that's the intention of that flag. I'm a little surprised this callback is even called when there's no body.
Alright, then my patch in PR #16010 should be fine to land as-is, and we'll just need to alter the spec to match it.