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
(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.
(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.
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.
(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.
(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.
> 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.
(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.
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?
(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.
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.
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.
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.
(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?
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
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
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
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
(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.
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
(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.
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
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
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
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
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.
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.
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.
2012-10-05 04:54 PDT, Dominik Röttsches (drott)
2012-10-08 05:24 PDT, Dominik Röttsches (drott)
2012-10-08 06:41 PDT, Dominik Röttsches (drott)
2014-01-14 08:08 PST, youenn fablet
2014-01-14 09:04 PST, Build Bot
2014-01-14 09:28 PST, Build Bot
2014-01-14 10:27 PST, Build Bot
2014-02-07 04:10 PST, youenn fablet
2014-07-11 05:27 PDT, youenn fablet
2014-11-07 07:58 PST, youenn fablet
2015-11-10 03:23 PST, youenn fablet
2015-11-10 04:10 PST, Build Bot
2015-11-10 04:12 PST, Build Bot
2015-11-10 04:14 PST, Build Bot
2015-11-10 05:19 PST, youenn fablet
2015-11-10 06:08 PST, Build Bot
2015-11-10 07:53 PST, youenn fablet
2015-11-12 04:45 PST, youenn fablet