Bug 40952 – Onloadend event is not supported in XMLHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[Stuart Ng]

- Send XHR request for website that should return OK
- Check that load and loadstart event handler was fired
- onloadend event handler was never fired. According to W3C Spec onloadend event should fired upon successful completion of XHR request.

[Joe Mason]

http://atg05-yyz/html5/xhrasync/stateTest.htm shows this bug.  (I can't seem to add it to the url field.  Guess I don't have access?)

[Joe Mason]

(In reply to comment #1)
> http://atg05-yyz/html5/xhrasync/stateTest.htm shows this bug.  (I can't seem to add it to the url field.  Guess I don't have access?)

Oops, that's not a public url.  Ignore.

[Hans Muller]

Per the W3C XMLHttpRequest2 Working Draft, http://www.w3.org/TR/XMLHttpRequest2, the loadend ProgressEvent should be dispatched by the XMLHttpRequest or it's XMLHttpRequestUpload after the load, error, and abort events for requests that have successfully completed, failed, or been aborted respectively.

Support for loadend requires a small number of changes:
- adding an onloadend EventListener attribute to Source/WebCore/xml/XMLHttpRequest.idl,XMLHttpRequestUpload.idl
- Similarly defining the event listener in Source/WebCore/xml/XMLHttpRequest.h, XMLHttpRequestUpload.h
- Dispatching the loadend after load/error/abort events are dispatched in Source/WebCore/xml/XMLHttpRequest.cpp

[Hans Muller]

Created attachment 118587 [details]
Patch

[Hans Muller]

I've attached a patch that adds the feature as proposed in comment #3.  I've included five tests which verify that the loadend progress event is dispatched last, as specfied in the XMLHttpRequest2 spec.  The tests haven't been combined to keep them simple and because they test independent code paths.   I did not include a test that verifies that upload error progress events are followed by a loadend because I wasn't sure how to generate a network error in that scenario.

[WebKit Review Bot]

Attachment 118587 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 118587 [details]
Patch

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

You are going in the right direction, some comments inlined. Also you are only testing half of the change: what happened to the synchronous case.

> Source/WebCore/ChangeLog:15
> +

The ChangeLog should have some information on the exact change you did. Here is an example of what you could write:

Patches the different sites where we dispatch a 'load', 'error' or 'abort' event to also dispatch a 'loadend' event. Also added the plumbing for the new 'loadend' event.

You can also put the details next to the lines below too (some reviewers prefer it this way but it is not mandated by our coding style).

> Source/WebCore/xml/XMLHttpRequest.cpp:346
> +        m_progressEventThrottle.dispatchEvent(XMLHttpRequestProgressEvent::create(eventNames().loadendEvent));

It would actually be more bullet proof for you to patch XMLHttpRequestProgressEventThrottle and XMLHttpRequestUpload. It's super easy to forget to add the loadend event at one call site and we would like to avoid that.

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-abort-expected.txt:7
> +XHR Events: loadstart readyState=DONE abort loadend

Global remarks on the tests: it looks like the result could be checked automatically by the script and just print PASSED instead of the string. It would make the output a passing case a lot more readable. You would have to do some simple post-processing though. 

There are some examples in the same directory that follow this format, see xmlhttprequest-multiple-open.html for example.

> LayoutTests/http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html:7
> +<p> Verify that a loadend ProgressEvent is dispatched after the error ProgressEvent when an async upload request is aborted.</p>

looks like this description is not up to date as you are not expecting any errors.

[Hans Muller]

I've made all of the changes you suggested.   I added dispatchEventAndLoadEnd() methods to XMLHttpRequestUpload and XMLHttpRequestProgressEventThrottle which ensures that a loadend progress event is dispatched so long as the ...AndLoadEnd() method is called.  I could make the existing dispatchEvent() methods do as much automtically but that seems a presumptious, since the (I guess) the progress event protocol might not always pair events in this way?

[WebKit Review Bot]

Comment on attachment 118587 [details]
Patch

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

New failing tests:
media/event-attributes.html

[Julien Chaffraix]

(In reply to comment #8)
> I added dispatchEventAndLoadEnd() methods to XMLHttpRequestUpload and XMLHttpRequestProgressEventThrottle which ensures that a loadend progress event is dispatched so long as the ...AndLoadEnd() method is called.

Great!

> I could make the existing dispatchEvent() methods do as much automtically but that seems a presumptious, since the (I guess) the progress event protocol might not always pair events in this way?

I agree with you on this one. Not necessarily based on the ordering argument but more on the fact that it would be confusing for dispatchEvent() to have this side effect. dispatchEvent() should be just about properly filling an Event and dispatching it in the DOM.

[Hans Muller]

Created attachment 118792 [details]
Patch

[Julien Chaffraix]

Comment on attachment 118792 [details]
Patch

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

I still don't see the synchronous case covered even if you mentioned it in the patch description.

> Source/WebCore/ChangeLog:10
> +        and XMLHttpRequestUpload to foolproof the common case of dispatching a load,abort,error

Space after the comma.

> Source/WebCore/xml/XMLHttpRequest.cpp:772
> +        if (m_upload && m_uploadEventsAllowed) 

Trailing whitespace.

> Source/WebCore/xml/XMLHttpRequest.cpp:784
> +        if (m_upload && m_uploadEventsAllowed) 

Ditto.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:84
> +void XMLHttpRequestProgressEventThrottle::dispatchEventAndLoadEnd(PassRefPtr<Event> event, ProgressEventAction progressEventAction)

This is really not much more fool-proof than calling the 2 methods together at the call sites. We really want to prevent people from mis-using the API as much as we could so at least white-listing the events you expect here with an ASSERT is a minimum.

It would be really neat to have some checks at the dispatchEvent level to prevent people from forgetting dispatchEventAndLoadEnd. I would really love to see your patch handle that properly. It would involve having 2 code path in dispatchEvent (2 functions like dispatchEvent and dispatchEventWithoutLoadEnd - in which case dispatchEventWithLoadEnd would be a better fit for this function), one common to every one and one specific for events that don't need a 'loadend'. The latter would black-list the events we expect to go through dispatchEventAndLoadEnd. You may see some issues with this approach, feel free to push back in this case.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:53
> +    void dispatchEventAndLoadEnd(PassRefPtr<Event>, ProgressEventAction = DoNotFlushProgressEvent);

I did not see any call sites passing |ProgressEventAction| so let's just remove it and pass this value down to dispatchEvent.

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-abort.html:59
> +    xhr.open("GET", "resources/get.txt");

Nit: for consistency it would be nice to put |true| here (also for people who don't know that the default is true).

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-error.html:58
> +    xhr.open("GET", "resources/infinite-loop.php");

Ditto.

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-load.html:57
> +    xhr.open("GET", "resources/get.txt"); 

Ditto.

[Hans Muller]

Created attachment 119020 [details]
Patch

[Hans Muller]

Sorry about leaving out the new sync tests, I'm still getting used to git addition.  

I've made all of the changes you suggested except for the one about the new dispatchEventAndLoadEnd() methods.  Assuming I'm understanding you correctly, adding an ASSERT to the two new methods which verifies that the event parameter is either abort, error, or load, makes sense.    The other approach you suggested, if I'm likewise understanding what you meant, would mean adding dispatch methods for each progress event type, where the abort, error, and load event methods would be aptly named because they would dispatch a loadend event too.   Then, so long as XMLHttpRequest.cpp did not call dispatchEvent() directly, it might be a little clearer that the implementation wasn't dispatching unpaired xx/loadend events.   Perhaps I'm not seeing the point, but this seems like a lot of defensive code to diminish the probability of one kind of error?

[Julien Chaffraix]

(In reply to comment #14)
> Sorry about leaving out the new sync tests, I'm still getting used to git addition.  
> 
> Assuming I'm understanding you correctly, adding an ASSERT to the two new methods which verifies that the event parameter is either abort, error, or load, makes sense.  

You're understanding correctly.

>  Perhaps I'm not seeing the point, but this seems like a lot of defensive code to diminish the probability of one kind of error?

But an extremely easy to make. If we decide to refactor our code to support some new XHR feature, you could end up just calling dispatchEvent([...].errorEvent). That's the use case I am trying to prevent.

I don't deny that it will be some work to get that implemented. I still think it would be worthwhile but if that doesn't cut, then I am OK to go without that check.

[Hans Muller]

I've refactored the changes to reduce the probability of dispatching a solitary load, error, timeout, or abort event.  Events are no longer dispatched directly (with dispatchEvent()) in XMLHttpRequest. If this sounds like what you wanted, let me know and I'll upload a new patch.  Having done this, it doesn't seem to me like the extra complexity balances the added safety, however I'm new to the code.

- Added the following public methods to XMLHttpRequestUpload.  Upload 

    void dispatchLoadStartProgressEvent();
    void dispatchProgressEvent(bool lengthComputable, unsigned long long loaded, unsigned long long total);
    void dispatchFinalProgressEvent(PassRefPtr<Event>); // ASSERTs that event is load, abort, error, or timeout


- Added similar public methods to XMLHttpRequestProgressEventThrottle as well as a method to dispatch the readystatechange event.   Note that XMLHttpRequestProgressEventThrottle::dispatchProgressEvent() was already defined.

    void dispatchReadyStateChangeEvent(ProgressEventAction = DoNotFlushProgressEvent);
    void dispatchLoadStartProgressEvent();
    void dispatchFinalProgressEvent(PassRefPtr<Event>); // ASSERTs that event is load, abort, error, or timeout

I've overridden the dispatchEvent() methods with an ASSERT that the event parameter is *not* load, abort, error, or timeout.

[Julien Chaffraix]

(In reply to comment #16)
> I've refactored the changes to reduce the probability of dispatching a solitary load, error, timeout, or abort event.  Events are no longer dispatched directly (with dispatchEvent()) in XMLHttpRequest. If this sounds like what you wanted, let me know and I'll upload a new patch. 
 
It was exactly what I wanted. I would be happy to look at such a patch (even if it is rough just to assess the added complexity).

> Having done this, it doesn't seem to me like the extra complexity balances the added safety, however I'm new to the code.

Let me look at that and tell you if your gut feeling is OK.

Thanks for following up!

[Hans Muller]

Created attachment 119636 [details]
Patch

[Hans Muller]

I've uploaded a version of the patch that implements the changes I specified in comment #16 except for the XMLHttpRequestUpload::dispatchEvent() override.  This patch is just for the sake of review:

- The XMLHttpRequestUpload::dispatchEvent() override is needed.

- XMLHttpRequestProgressEventThrottle::dispatchEvent() should probably "black list" the events that should be dispatched with dispatchFinalProgressEvent() rather than asserting which events are OK.

- The ChangeLogs haven't been updated yet.

[Julien Chaffraix]

Comment on attachment 119636 [details]
Patch

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

OK, I think your gut feeling was right, If you just upload something along the line of the previous patch with the different comments addressed and the synchronous test cases from this patch, it should be fine by me. Thanks.

> Source/WebCore/ChangeLog:11
> +        or error event followed by a loadend event.

ChangeLog not up-to-date as mentioned.

> Source/WebCore/loader/ImageLoader.cpp:1
> +

Unrelated (bad) change.

> Source/WebCore/xml/XMLHttpRequest.cpp:345
> +        m_progressEventThrottle.dispatchFinalProgressEvent(XMLHttpRequestProgressEvent::create(eventNames().loadEvent));

You are subtly changing the semantics of the willLoadXHR / didLoadXHR here as we now include 2 events instead of one. I would rather see that the loadendEvent is dispatched after didLoadXHR was called and a bug filed about giving access to the "loadend" event from the WebInspector.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:51
> +    virtual void dispatchEvent(PassRefPtr<Event>, ProgressEventAction = DoNotFlushProgressEvent);

It shouldn't be virtual.

> Source/WebCore/xml/XMLHttpRequestUpload.h:68
> +        void dispatchProgressEvent(bool lengthComputable, unsigned long long loaded, unsigned long long total);

Those 2 should be removed as they don't seem to add much. Same remarks for the XHR throttle.

> Source/WebCore/xml/XMLHttpRequestUpload.h:69
> +        void dispatchFinalProgressEvent(PassRefPtr<Event>);

I think I preferred the old name dispatchEventAndLoadendEvent(). ProgressEvent hints a the "progress" event and yet you allow other type of events.

> LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html:93
> +    xhr.open("GET", "resources/get.txt", false); 

Great that you added the synchronous case! For coherency, it would be better to have those splitted but it's not a strong requirement.

[Hans Muller]

Created attachment 120053 [details]
Patch

[Hans Muller]

This patch is the same as one uploaded on 2011-12-13 08:19 PST, with the following additional changes:

Added ASSERT to XMLHttpRequestProgressEventThrottle::dispatchEventAndLoadEnd(), XMLHttpRequestProgressEventUpload::dispatchEventAndLoadEnd():

    ASSERT(event->type() == eventNames().loadEvent || event->type() == eventNames().abortEvent || event->type() == eventNames().errorEvent || event->type() == eventNames().timeoutEvent);

Removed use of dispatchEventAndLoadEnd() from XMLHttpRequest::callReadyStateChangeListener() to avoid subtly changing the semantics of InspectorInstrumentation:: willLoadXHR()/didLoadXHR().  Will file a bug per (review) comment #20.

[Julien Chaffraix]

Comment on attachment 120053 [details]
Patch

Please don't forget to file a WebInspector bug about supporting loadend.

Also if you want your patch to be committed, you should set for the cq flag to '?'.

[WebKit Review Bot]

Comment on attachment 120053 [details]
Patch

Clearing flags on attachment: 120053

Committed r103502: <http://trac.webkit.org/changeset/103502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hans Muller]

(In reply to comment #23)
> (From update of attachment 120053 [details])
> Please don't forget to file a WebInspector bug about supporting loadend.

Sorry about the delay, it's https://bugs.webkit.org/show_bug.cgi?id=75483