Bug 120828

Summary: loadend ProgressEvent of XHR can not get correct value of attribute of loaded and total
Product: WebKit Reporter: Zhuang Zhigang <zhuangzg>
Component: XMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Normal CC: ap, buildbot, commit-queue, rniwa, youennf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 126677    
Bug Blocks: 126575    
Attachments:
Description Flags
sample
none
Initial fix
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Added load event test rebaseline
none
Variant with handling of upload events
none
Fixed init issue
none
Updated according review
none
Code refactoring none

Description Zhuang Zhigang 2013-09-05 22:58:34 PDT
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.
Comment 1 Zhuang Zhigang 2013-09-05 23:01:05 PDT
Created attachment 210704 [details]
sample
Comment 2 youenn fablet 2013-11-28 05:34:30 PST
Created attachment 217991 [details]
Initial fix

‚ÄĚProbably
Comment 3 youenn fablet 2013-11-28 05:35:59 PST
Probably need to fix upload events in the same way. I will check this and fix it as a separate patch if needed
Comment 4 Build Bot 2013-11-28 06:27:23 PST
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
Comment 5 Build Bot 2013-11-28 06:27:24 PST
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
Comment 6 Build Bot 2013-11-28 06:54:58 PST
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
Comment 7 Build Bot 2013-11-28 06:55:00 PST
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
Comment 8 youenn fablet 2013-11-28 07:14:12 PST
Created attachment 218001 [details]
Added load event test rebaseline
Comment 9 youenn fablet 2013-11-29 08:56:39 PST
Created attachment 218071 [details]
Variant with handling of upload events

Fixed upload ProgressEvent events. Made consistent the handling of XMLHttpRequestProgressEventthrottle and XMLHttpRequestUpload events.
Comment 10 youenn fablet 2013-12-19 06:33:01 PST
Created attachment 219647 [details]
Fixed init issue
Comment 11 Alexey Proskuryakov 2013-12-20 12:04:44 PST
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.
Comment 12 youenn fablet 2013-12-24 02:03:55 PST
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
Comment 13 Alexey Proskuryakov 2014-01-02 01:06:57 PST
> 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?
Comment 14 youenn fablet 2014-01-06 06:32:59 PST
Created attachment 220431 [details]
Updated according review
Comment 15 youenn fablet 2014-01-06 06:34:23 PST
(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.
Comment 16 youenn fablet 2014-01-07 07:51:22 PST
Created attachment 220526 [details]
Code refactoring

refactored code to ease full alignment with XHR spec (see bugs 126574 and 126575)
Comment 17 WebKit Commit Bot 2014-01-08 18:08:01 PST
Comment on attachment 220526 [details]
Code refactoring

Clearing flags on attachment: 220526

Committed r161532: <http://trac.webkit.org/changeset/161532>
Comment 18 WebKit Commit Bot 2014-01-08 18:08:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Commit Bot 2014-01-08 19:49:19 PST
Re-opened since this is blocked by bug 126677
Comment 20 Alexey Proskuryakov 2014-01-08 19:50:42 PST
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>.
Comment 21 Alexey Proskuryakov 2014-01-08 19:51:17 PST
Something is wrong with this bug, it refuses to re-open!
Comment 22 Alexey Proskuryakov 2014-01-08 19:54:28 PST
I guess we'll need a new bug to re-land this once fixed.
Comment 23 youenn fablet 2014-01-08 23:47:08 PST
(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.