|
Description
Dominik Röttsches (drott)
2012-10-04 06:58:31 PDT
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? (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. Created attachment 167308 [details]
Reducing event firing to else case.
(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. Created attachment 167525 [details]
Patch
(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. 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 Created attachment 167533 [details]
Updated to fix test failure.
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 (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? > 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. Alexey, is there still some additional info that would be needed? 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 #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 Alex considers the same behavior a bug in in FF: https://bugzilla.mozilla.org/show_bug.cgi?id=525816#c32 (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 Do we match the spec now? Can the bug be closed? (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? (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. 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
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 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 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
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 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
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 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
Created attachment 234760 [details]
added fixme and updated email address
(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. Patch still works although it needs to be rebased, hence removing 'cq?'. Anyone to review it? Created attachment 241181 [details]
Improving test comments
Created attachment 265161 [details]
Patch
(In reply to comment #41) > Created attachment 265161 [details] > Patch Rebasing patch against web-platform-tests tests 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 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
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 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
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 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 265169 [details]
Rebasing tests
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 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
Created attachment 265178 [details]
Fixing async-xhr-onerror.html
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. Created attachment 265383 [details]
Patch for landing
Comment on attachment 265383 [details] Patch for landing Clearing flags on attachment: 265383 Committed r192361: <http://trac.webkit.org/changeset/192361> All reviewed patches have been landed. Closing bug. *** Bug 126574 has been marked as a duplicate of this bug. *** |