Bug 98404 – XHR abort() event firing does not match spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dominik Röttsches (drott)]

The XHR abort implementation does not correctly move from step 6 to 7:
http://www.w3.org/TR/XMLHttpRequest/#the-abort-method
That means, it's firing abort events, even 
if the state is "UNSENT, OPENED with the send() flag being unset, or DONE"

This was found when implementing XHR2 timeout and needs to be fixed before that. 
When corrected, this causes issues with the following test cases 
fast/events/event-fire-order.html 
http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html 
http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html

[Alexey Proskuryakov]

Did you check current version of the spec, <http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html>? What do IE and Firefox do?

[Dominik Röttsches (drott)]

(In reply to comment #1)
> Did you check current version of the spec, <http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html>? 

Same algorithm in the dvcs version.

> What do IE and Firefox do?

I will check this abort behavior specifically, also in IE, but Firefox passes the large set of XHR timeout tests that I am going to import. And during those it doesn't fire an extra "abort" event like WebKit does here.

[Dominik Röttsches (drott)]

Created attachment 167308 [details]
Reducing event firing to else case.

[Dominik Röttsches (drott)]

(In reply to comment #1)
> What do IE and Firefox do?

For the abort() case in OPENED or DONE state, IE, FF and Opera do not send an extra abort() event.

[Alexey Proskuryakov]

Comment on attachment 167308 [details]
Reducing event firing to else case.

View in context: https://bugs.webkit.org/attachment.cgi?id=167308&action=review

> Source/WebCore/ChangeLog:12
> +        fast/events/event-fire-order.html

There is something wrong with this test. Both original and updated versions pass in current WebKit ToT. Can you explain this?

> Source/WebCore/ChangeLog:13
> +        http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html

This test fails in Firefox when the patch is applied. What does this mean?

> Source/WebCore/ChangeLog:14
> +        http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html

Ditto.

[Dominik Röttsches (drott)]

(In reply to comment #5)
> (From update of attachment 167308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167308&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        fast/events/event-fire-order.html
> 
> There is something wrong with this test. Both original and updated versions pass in current WebKit ToT. Can you explain this?

Will check this one on Monday.

> > Source/WebCore/ChangeLog:13
> > +        http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
> 
> This test fails in Firefox when the patch is applied. What does this mean?
> 
> > Source/WebCore/ChangeLog:14
> > +        http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html
> 
> Ditto.

What failure do you see? IIRC it fails in FF because FF sends an extra "error" event, which is not correct, in my opinion.

[Dominik Röttsches (drott)]

Created attachment 167525 [details]
Patch

[Dominik Röttsches (drott)]

(In reply to comment #5)
> (From update of attachment 167308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167308&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        fast/events/event-fire-order.html
> 
> There is something wrong with this test. Both original and updated versions pass in current WebKit ToT. Can you explain this?

I should pass an empty list [] as an argument, otherwise the result comparison function fails to correctly compare the lists. Fixed.

> > Source/WebCore/ChangeLog:13
> > +        http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html

> > Source/WebCore/ChangeLog:14
> > +        http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html

> This test fails in Firefox when the patch is applied. What does this mean?

The results for both tests now with FF are:
FAILED results : ' [unexpected ProgressEvent: load]' expected : 'undefined' FAILED results : ' [unexpected ProgressEvent: load] [unexpected ProgressEvent: loadend]' expected : 'undefined' FAILED results : ' [unexpected ProgressEvent: load] [unexpected ProgressEvent: loadend]' expected : '' 

So, in short, FF fires an unespected load and unexpected loadend result. In my opinion, this is a bug in FF. According to the spec, "Infrastructure for the send() method"

"When it is said to switch to the DONE state run these steps:
1 If the synchronous flag is set, update the response entity body.
2) Unset the synchronous flag.
3) Change the state to DONE.
4) Fire an event named readystatechange.
5) Fire a progress event named progress.
6) Fire a progress event named load.
7) Fire a progress event named loadend."

The tests calls abort() in the readystatechange callback, which is equivalent to step 5 of the above. Step 1 of 
abort() method in the spec says:
"Terminate the send() algorithm."

Despite this termination, FF now proceeds to send load, loadend. WebKit does not. In this case, I think WebKit's behavior is correct.

[WebKit Review Bot]

Comment on attachment 167525 [details]
Patch

Attachment 167525 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14215329

New failing tests:
fast/events/event-fire-order.html

[Dominik Röttsches (drott)]

Created attachment 167533 [details]
Updated to fix test failure.

[Build Bot]

Comment on attachment 167533 [details]
Updated to fix test failure.

Attachment 167533 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14209493

New failing tests:
http/tests/workers/terminate-during-sync-operation.html

[Dominik Röttsches (drott)]

(In reply to comment #11)
> (From update of attachment 167533 [details])
> Attachment 167533 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14209493
> 
> New failing tests:
> http/tests/workers/terminate-during-sync-operation.html

In the queue output that the bot posted here the test passes. I don't see what's the reason is here, false negative?

[Alexey Proskuryakov]

> Despite this termination, FF now proceeds to send load, loadend. WebKit does not. In this case, I think WebKit's behavior is correct.

Firing loadend seems to make sense. The whole reason for this event is that it's sent any time a load ends, so that cleanup that doesn't depend on how load ended should be put in loadend handler.

If both Firefox and WebKit fire this event in this case today, then it's a de facto standard, no need to "fix" that part.

[Dominik Röttsches (drott)]

(In reply to comment #13)
> > Despite this termination, FF now proceeds to send load, loadend. WebKit does not. In this case, I think WebKit's behavior is correct.
> 
> Firing loadend seems to make sense. The whole reason for this event is that it's sent any time a load ends, so that cleanup that doesn't depend on how load ended should be put in loadend handler.
> 
> If both Firefox and WebKit fire this event in this case today, then it's a de facto standard, no need to "fix" that part.

I am not changing the load/loadend behavior in WebKit, only the abort event sending behavior, actually to match FF and the spec. The load/loadend is today fired only by FF.

[Alexey Proskuryakov]

Comment on attachment 167533 [details]
Updated to fix test failure.

View in context: https://bugs.webkit.org/attachment.cgi?id=167533&action=review

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html:107
> -    completeTest(" abort loadend");
> +    // No abort/loadend events expected when aborting in DONE state.
> +    completeTest("");

Why is this test changed to not expect loadend then?

[Dominik Röttsches (drott)]

(In reply to comment #15)
> (From update of attachment 167533 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167533&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html:107
> > -    completeTest(" abort loadend");
> > +    // No abort/loadend events expected when aborting in DONE state.
> > +    completeTest("");
> 
> Why is this test changed to not expect loadend then?

I forgot that the function ends with "...AndLoadEnd" :-(
dispatchEventAndLoadEnd - so I do change that since I don't fire abort anymore and then no loadend. 

So, would you prefer if I fire a loadend in any case? Still, according to the spec that would be incorrect. And regarding a generic "loadend" cleanup, I agree, that would have some justification, but in those cases where we don't fire the abort, the XHR is basically idle and there's nothing to cleanup.

[Dominik Röttsches (drott)]

As discussed on IRC, here's the result for the 
http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
case.

I'm testing using
http://roettsch.es/xhrabort/onloadend-event-after-sync-requests.html
which comments out the first two tests, only looking at testAbort(),
also I'm commenting out completeTest() in logUnexpectedProgress event to not exit too early.

The result for IE 9 in Quirks Mode as well as in IE 9 Standards Mode is:
FAILED results : '', expected : ' abort loadend' 

So, it doesn't dispatch abort and neither loadend.

[Dominik Röttsches (drott)]

Testing using http://roettsch.es/xhrabort/loadendIE.html
shows that in IE 9 for also for testNormal case (i.e. an XHR that completes without any error), there's no loadend event, only a load.

[Dominik Röttsches (drott)]

Alexey, is there still some additional info that would be needed?

[Alexey Proskuryakov]

Comment on attachment 167533 [details]
Updated to fix test failure.

I'm not quite sure what your question is.

The current iteration of the patch seems undesirable, since it doesn't dispatch loadend in a case where it's entirely reasonable to authors to expect it, and where we used to match Firefox. Also, it didn't pass Mac EWS.

If you find the current spec unclear on this point, it would be a good step to propose a correction to it.

[Dominik Röttsches (drott)]

(In reply to comment #20)
> If you find the current spec unclear on this point, it would be a good step to propose a correction to it.

Spec issue filed:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19470

[Dominik Röttsches (drott)]

Alex considers the same behavior a bug in in FF:
https://bugzilla.mozilla.org/show_bug.cgi?id=525816#c32

[Dominik Röttsches (drott)]

(In reply to comment #21)
> (In reply to comment #20)
> > If you find the current spec unclear on this point, it would be a good step to propose a correction to it.
> 
> Spec issue filed:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19470

Spec updated:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19470#c5

[Alexey Proskuryakov]

Do we match the spec now? Can the bug be closed?

[youenn fablet]

(In reply to comment #24)
> Do we match the spec now? Can the bug be closed?

I do not think we match the spec right now.

The spec seems to mandate that when switching to DONE, a load and loadend events are sent just after a readystatechange event.
This is not the case when calling abort() in readystatechange (DONE) event, as  
XMLHttpRequest::callReadyStateChangeListener will not send the expected load and loadend events.

We should probably update both XMLHttpRequest::callReadyStateChangeListener and XMLHttpRequest::abort to align with the spec.

Firefox seems to be aligned with the spec on this.
Blink seems to send neither abort nor loadend event.

Dominik, are you continuing on this bug or may I take over?

[Dominik Röttsches (drott)]

(In reply to comment #25)

> Dominik, are you continuing on this bug or may I take over?

I am not going to work on this, please feel free to take over.

[youenn fablet]

Created attachment 221163 [details]
Ensuring load and loadend events are sent even if abort() function is called just after siwtching to DONE state

Patch may need to be completed to cope with ready state change event or load event callbacks that call open() on the same object

[EFL EWS Bot]

Comment on attachment 221163 [details]
Ensuring load and loadend events are sent even if abort() function is called just after siwtching to DONE state

Attachment 221163 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/6584648184365056

[Build Bot]

Comment on attachment 221163 [details]
Ensuring load and loadend events are sent even if abort() function is called just after siwtching to DONE state

Attachment 221163 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5547358418894848

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html

[Build Bot]

Created attachment 221168 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221163 [details]
Ensuring load and loadend events are sent even if abort() function is called just after siwtching to DONE state

Attachment 221163 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5026163600654336

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html

[Build Bot]

Created attachment 221169 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221163 [details]
Ensuring load and loadend events are sent even if abort() function is called just after siwtching to DONE state

Attachment 221163 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5726004899217408

New failing tests:
fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html

[Build Bot]

Created attachment 221176 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[youenn fablet]

(In reply to comment #33)
> (From update of attachment 221163 [details])
> Attachment 221163 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/5726004899217408
> 
> New failing tests:
> fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html

The patch changes when the state goes from DONE to UNSENT.
Before the patch, state goes to UNSENT before the ProgressEvent are sent.
With the patch, state goes to UNSENT just after the ProgressEvent are sent.

fast/xmlhttprequest/xmlhttprequest-responsetype-abort.html is failing due to that change of behavior (state is DONE, and XHR response will return a value even if error flag is set).
Fixing bug 127050 should allow passing this test.

[youenn fablet]

Created attachment 223448 [details]
Simplified patch

This ispatch should align WebKit with Firefox. It may be better though to emit load event before resetting response buffers

[youenn fablet]

Created attachment 234760 [details]
added fixme and updated email address

[youenn fablet]

(In reply to comment #37)
> Created an attachment (id=234760) [details]
> added fixme and updated email address

This patch tries to align with the spec, Firefox and Chrome.

[youenn fablet]

Patch still works although it needs to be rebased, hence removing 'cq?'.
Anyone to review it?

[youenn fablet]

Created attachment 241181 [details]
Improving test comments

[youenn fablet]

Created attachment 265161 [details]
Patch

[youenn fablet]

(In reply to comment #41)
> Created attachment 265161 [details]
> Patch

Rebasing patch against web-platform-tests tests

[Build Bot]

Comment on attachment 265161 [details]
Patch

Attachment 265161 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/409798

New failing tests:
http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-aborted.html
http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-aborted.html
fast/events/event-fire-order.html

[Build Bot]

Created attachment 265162 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 265161 [details]
Patch

Attachment 265161 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/409803

New failing tests:
fast/events/event-fire-order.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-aborted.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-aborted.html
http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
http/tests/contentextensions/async-xhr-onerror.html
http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html

[Build Bot]

Created attachment 265163 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 265161 [details]
Patch

Attachment 265161 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/409793

New failing tests:
http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-aborted.html
http/tests/xmlhttprequest/upload-onloadend-event-after-sync-requests.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-aborted.html
fast/events/event-fire-order.html

[Build Bot]

Created attachment 265164 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 265169 [details]
Rebasing tests

[Build Bot]

Comment on attachment 265169 [details]
Rebasing tests

Attachment 265169 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/410166

New failing tests:
http/tests/contentextensions/async-xhr-onerror.html

[Build Bot]

Created attachment 265171 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[youenn fablet]

Created attachment 265178 [details]
Fixing async-xhr-onerror.html

[youenn fablet]

Comment on attachment 265178 [details]
Fixing async-xhr-onerror.html

View in context: https://bugs.webkit.org/attachment.cgi?id=265178&action=review

> LayoutTests/imported/w3c/ChangeLog:14
> +        * web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted-expected.txt:

Patch aligns with spec and get closer to what Chrome and Firefox do.
Chrome is passing all tests except abort-event-order.
Firefox is passing all tests except abort-during-upload, abort-event-abort and abort-event-order.

[Darin Adler]

Comment on attachment 265178 [details]
Fixing async-xhr-onerror.html

View in context: https://bugs.webkit.org/attachment.cgi?id=265178&action=review

> Source/WebCore/xml/XMLHttpRequest.cpp:376
> +    // Check whether sending load and loadend events before sending readystatechange event, as it may change m_error/m_state values

WebKit coding style asks for a period at the end of this sentence.

> Source/WebCore/xml/XMLHttpRequest.cpp:377
> +    bool sendLoadEvent = (m_state == DONE && !m_error);

A better name for this is shouldSendLoadEvent. We don’t want to name booleans as verbs, instead they should be predicates.

> Source/WebCore/xml/XMLHttpRequest.h:219
> +    bool m_sendFlag { false };

Is the term “send flag” from the XMLHttpRequest specification?

I don’t understand the name. It’s a sentence “send flag”. It’s vague. What does it mean when it’s true? What when false? None of that is clear from its name. But if it’s straight out of the spec, I suppose we can just use it for clarity for people who are reading the spec.

[youenn fablet]

Thanks for the review.

(In reply to comment #54)
> Comment on attachment 265178 [details]
> Fixing async-xhr-onerror.html
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265178&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:376
> > +    // Check whether sending load and loadend events before sending readystatechange event, as it may change m_error/m_state values
> 
> WebKit coding style asks for a period at the end of this sentence.

OK.

> > Source/WebCore/xml/XMLHttpRequest.cpp:377
> > +    bool sendLoadEvent = (m_state == DONE && !m_error);
> 
> A better name for this is shouldSendLoadEvent. We don’t want to name
> booleans as verbs, instead they should be predicates.

OK

> > Source/WebCore/xml/XMLHttpRequest.h:219
> > +    bool m_sendFlag { false };
> 
> Is the term “send flag” from the XMLHttpRequest specification?
> 
> I don’t understand the name. It’s a sentence “send flag”. It’s vague. What
> does it mean when it’s true? What when false? None of that is clear from its
> name. But if it’s straight out of the spec, I suppose we can just use it for
> clarity for people who are reading the spec.

The spec uses the term "send() flag" which can be set or unset (cf. https://xhr.spec.whatwg.org/#send-flag).
The spec name is probably not ideal. 
sendFlag keeps code close to the spec, like for instance: "Throws an InvalidStateError exception if either state is not opened or the send() flag is set." for setRequestHeader.

[youenn fablet]

Created attachment 265383 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 265383 [details]
Patch for landing

Clearing flags on attachment: 265383

Committed r192361: <http://trac.webkit.org/changeset/192361>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

*** Bug 126574 has been marked as a duplicate of this bug. ***