Bug 120828 – loadend ProgressEvent of XHR can not get correct value of attribute of loaded and total

• Component: XML
• Status: RESOLVED LATER

[Zhuang Zhigang]

When we use XHR like below, we can not get correct value.

client = new XMLHttpRequest();
client.open("GET", "data.php");
...
client.onloadend = function(pe) {
    console.log("onloadend: total:"+pe.total+",loaded="+pe.loaded);
  }
client.send(); 

In the sample as the attachment, we expect "onloadend: total:1898,loaded=1898", but we get "onloadend: total:0,loaded=0".
Firefox(23.0.1) can get correct value.

[Zhuang Zhigang]

Created attachment 210704 [details]
sample

[youenn fablet]

Created attachment 217991 [details]
Initial fix

”Probably

[youenn fablet]

Probably need to fix upload events in the same way. I will check this and fix it as a separate patch if needed

[Build Bot]

Comment on attachment 217991 [details]
Initial fix

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[Build Bot]

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

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

[Build Bot]

Comment on attachment 217991 [details]
Initial fix

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

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml

[Build Bot]

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

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

[youenn fablet]

Created attachment 218001 [details]
Added load event test rebaseline

[youenn fablet]

Created attachment 218071 [details]
Variant with handling of upload events

Fixed upload ProgressEvent events. Made consistent the handling of XMLHttpRequestProgressEventthrottle and XMLHttpRequestUpload events.

[youenn fablet]

Created attachment 219647 [details]
Fixed init issue

[Alexey Proskuryakov]

Comment on attachment 219647 [details]
Fixed init issue

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

> Source/WebCore/ChangeLog:30
> +        (WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle):

I don't quite understand changes to this file. Could you please update the ChangeLog with some explanations?

The reason why prepare-ChangeLog generates these lists is because there is an expectation of per-function comments added manually.

> Source/WebCore/xml/XMLHttpRequest.cpp:1259
> +PassRefPtr<Event> XMLHttpRequest::createEvent(const AtomicString& type) 

This function should return PassRefPtr<XMLHttpRequestProgressEvent>.

> Source/WebCore/xml/XMLHttpRequest.cpp:1272
> +    long long expectedLength = m_response.expectedContentLength();
> +    bool lengthComputable = expectedLength > 0 && m_receivedLength <= expectedLength;
> +    unsigned long long total = lengthComputable ? expectedLength : 0;
> +    m_progressEventThrottle.dispatchProgressEvent(lengthComputable, m_receivedLength, total);

It looks unfortunate that this code is duplicated. Fixing this would be a bigger task though, as XMLHttpRequestProgressEventThrottle interface is somewhat ugly (it has both dispatchProgressEvent and dispatchEvent, but you can't pass ProgressEvents to dispatchEvent!)

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:33
> +#include <wtf/text/AtomicString.h>

Please don't include when a forward declaration (or including wtf/Forward.h) whould suffice.

[youenn fablet]

Comment on attachment 219647 [details]
Fixed init issue

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

>> Source/WebCore/ChangeLog:30
>> +        (WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle):
> 
> I don't quite understand changes to this file. Could you please update the ChangeLog with some explanations?
> 
> The reason why prepare-ChangeLog generates these lists is because there is an expectation of per-function comments added manually.

Yes, I should have made a better description of the changes.
I will improve this in a following patch.
Here is some information.

XMLHttpRequestProgressEventThrottle is currently resetting m_loaded and m_total to zero after sending a progress event.
m_loaded/m_total are used to store the progress event values but also to know whether to send a progress event or not.

This patch is changing this behavior as follow:
m_loaded and m_total are set to zero when a loadstart event is sent.
m_loaded and m_total are always updated within a dispatchProgressEvent call.
m_loaded and m_total are not reset after a progress event is sent.
To control whether or not a progress event should be sent, the boolean m_hasThrottledProgressEvent is introduced.

This allows XMLHttpRequestProgressEventThrottle to correctly initialize the loaded and total fields of any ProgressEvent event (loadend, load...).
This simplifies things in case of error (abort, timeout...) where XMLHttpRequest.m_response may be reset before the events are initialized.
The first patch was handling this issue by storing the m_response fields before m_response was reset.
I think Blink fixed the bug similarly to the first patch.

>> Source/WebCore/xml/XMLHttpRequest.cpp:1259
>> +PassRefPtr<Event> XMLHttpRequest::createEvent(const AtomicString& type) 
> 
> This function should return PassRefPtr<XMLHttpRequestProgressEvent>.

Will change this

>> Source/WebCore/xml/XMLHttpRequest.cpp:1272
>> +    m_progressEventThrottle.dispatchProgressEvent(lengthComputable, m_receivedLength, total);
> 
> It looks unfortunate that this code is duplicated. Fixing this would be a bigger task though, as XMLHttpRequestProgressEventThrottle interface is somewhat ugly (it has both dispatchProgressEvent and dispatchEvent, but you can't pass ProgressEvents to dispatchEvent!)

We could probably merge the two functions in a single one that would take an event name as parameter.
That would at least remove the duplicated code.

I do not particular like either the direct use of XMLHttpRequestProgressEventThrottle::dispatchEvent within XMLHttpRequest::callReadyStateChangeListener().

To improve things, we could probably introduce a XMLHttpRequestProgressEventThrottle::dispatchProgressEvent (that takes an event name as input) to dispatch all ProgressEvent events that are not throttled (loadstart, loaded...).
The current XMLHttpRequestProgressEventThrottle::dispatchProgressEvent could then be renamed as "dispatchThrottledProgressEvent" or "updateProgress".
Any thought?

Also in XMLHttpRequest::callReadyStateChangeListener, the readystatechange event is created as a ProgressEvent.
According the XHR spec, its interface should be Event. I wonder whether this should be fixed or not.

>> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.h:33
>> +#include <wtf/text/AtomicString.h>
> 
> Please don't include when a forward declaration (or including wtf/Forward.h) whould suffice.

Will fix that

[Alexey Proskuryakov]

> To improve things, we could probably introduce a XMLHttpRequestProgressEventThrottle::dispatchProgressEvent (that takes an event name as input) to dispatch all ProgressEvent events that are not throttled (loadstart, loaded...).
>The current XMLHttpRequestProgressEventThrottle::dispatchProgressEvent could then be renamed as "dispatchThrottledProgressEvent" or "updateProgress".
>Any thought?

It would be helpful to make this code look better, but I think that this needn't block landing the fix.

> Also in XMLHttpRequest::callReadyStateChangeListener, the readystatechange event is created as a ProgressEvent.
> According the XHR spec, its interface should be Event. I wonder whether this should be fixed or not.

I don't care very much. What do other browsers do?

[youenn fablet]

Created attachment 220431 [details]
Updated according review

[youenn fablet]

(In reply to comment #13)
> > Also in XMLHttpRequest::callReadyStateChangeListener, the readystatechange event is created as a ProgressEvent.
> > According the XHR spec, its interface should be Event. I wonder whether this should be fixed or not.
> 
> I don't care very much. What do other browsers do?

Blink is similar to WebKit.
Mozilla is directly instantiating a DOM event.
The change is included in the current patch.

[youenn fablet]

Created attachment 220526 [details]
Code refactoring

refactored code to ease full alignment with XHR spec (see bugs 126574 and 126575)

[WebKit Commit Bot]

Comment on attachment 220526 [details]
Code refactoring

Clearing flags on attachment: 220526

Committed r161532: <http://trac.webkit.org/changeset/161532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 126677

[Alexey Proskuryakov]

This made all XMLHttpRequest tests crash with assertion failures, rolling out: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r161538%20(15178)/results.html>.

[Alexey Proskuryakov]

Something is wrong with this bug, it refuses to re-open!

[Alexey Proskuryakov]

I guess we'll need a new bug to re-land this once fixed.

[youenn fablet]

(In reply to comment #22)
> I guess we'll need a new bug to re-land this once fixed.

I opened bug 126681 for that purpose.