Bug 37771 – XMLHttpRequestUpload events do not fire when sending a raw file or FormData object

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[eLod]

Webkit does not seem to fire various events (onload, onprogress) when uploading files with XMLHTTPRequest.send. I found bug https://bugs.webkit.org/show_bug.cgi?id=26979 and used FormData to send the files (I added Firefox's get/sendAsBinary way to test FF, see: https://developer.mozilla.org/en/Using_files_from_web_applications and http://blog.igstan.ro/2009/01/pure-javascript-file-upload.html, hopefully FF soon lands FormData too, so it automatically handles headers).

The test I used: http://pastie.org/925509, server side: http://pastie.org/925510. (I tested with "real server" with my home connection, uploading a pdf file about 700KB.)

Output with webkit (nighlty, r57720):
(0) request readystatechange: 1
(0) request start
(0) upload start
(0) request readystatechange: 2
(0) request progress: 60 / 60 = 100%
(0) request readystatechange: 3
(0) request readystatechange: 4
(0) request finished: upload success fileinput : file.pdf

The inspector shows 2.4s for the XHR.

The same test with Firefox (v3.6.3):
(0) request readystatechange: 1
(0) request readystatechange: 1
(0) request start
(0) upload start
(0) upload progress: 126361 / 693664 = 18.21645638234073%
(0) upload progress: 132601 / 693664 = 19.116027356183974%
(0) upload progress: 146841 / 693664 = 21.16889445033907%
(0) upload progress: 167321 / 693664 = 24.121332518337407%
(0) upload progress: 186177 / 693664 = 26.839651473912443%
(0) upload progress: 208281 / 693664 = 30.026208654334088%
(0) upload progress: 224665 / 693664 = 32.38815910873276%
(0) upload progress: 244097 / 693664 = 35.18951538497024%
(0) upload progress: 264369 / 693664 = 38.111967753840474%
(0) upload progress: 282009 / 693664 = 40.654985699128105%
(0) upload progress: 302489 / 693664 = 43.60742376712645%
(0) upload progress: 322969 / 693664 = 46.559861835124785%
(0) upload progress: 342561 / 693664 = 49.38428403376851%
(0) upload progress: 359833 / 693664 = 51.874250357521795%
(0) upload progress: 380313 / 693664 = 54.82668842552014%
(0) upload progress: 400793 / 693664 = 57.779126493518476%
(0) upload progress: 421273 / 693664 = 60.73156456151681%
(0) upload progress: 441753 / 693664 = 63.684002629515156%
(0) upload progress: 458137 / 693664 = 66.04595308391383%
(0) upload progress: 478617 / 693664 = 68.99839115191216%
(0) upload progress: 499097 / 693664 = 71.9508292199105%
(0) upload progress: 519577 / 693664 = 74.90326728790885%
(0) upload progress: 538041 / 693664 = 77.5650747335886%
(0) upload progress: 560537 / 693664 = 80.80814342390552%
(0) upload progress: 575441 / 693664 = 82.9567340960465%
(0) upload progress: 595961 / 693664 = 85.9149386446464%
(0) upload progress: 616233 / 693664 = 88.83739101351664%
(0) upload progress: 638361 / 693664 = 92.02740808229922%
(0) upload progress: 658841 / 693664 = 94.97984615029755%
(0) upload progress: 679321 / 693664 = 97.93228421829589%
(0) upload progress: 687513 / 693664 = 99.11325944549523%
(0) request readystatechange: 2
(0) upload finished
(0) request readystatechange: 3
(0) request readystatechange: 4
(0) request finished: upload success fileinput : file.pdf

Firebug reports 1.87s for the XHR.

[eLod]

same with nightly r57791

[Alexey Proskuryakov]

I see upload progress events when sending a string, but not with FormData indeed.

[Jian Li]

*** This bug has been marked as a duplicate of bug 36661 ***

[Alexey Proskuryakov]

Bug 36661 seems to be about a general improvement to upload events, while this seems to be specific to DOMFormData. Is this indeed a duplicate?

[Jian Li]

Yes, upload progress event for sending a raw file is broken since the patch introduced in 18654. 

No matter whether we send a file or a FormData, I only see progress event fired, not upload progress event. This happens in the recent nightly build.

[Julien Chaffraix]

(In reply to comment #5)
> Yes, upload progress event for sending a raw file is broken since the patch
> introduced in 18654. 
> 
> No matter whether we send a file or a FormData, I only see progress event
> fired, not upload progress event. This happens in the recent nightly build.

I agree with Alexey and I don't follow your thinking. If bug 18654 broke the upload progress events - that is we do not dispatch them for each byte send - then you should file a REGRESSION bug about it and blame me.

Bug 36661 is about bringing the same behaviour as bug 18654 to upload progress events.

[Jian Li]

Sorry I do not get what you mean for bug 18654. Let me reopen this bug as the bucket to track the upload progress event firing problem. I've updated the bug title to reflect this.

[Julien Chaffraix]

> Sorry I do not get what you mean for bug 18654.

Ok, let me explain: we used to dispatch a download progress event for each byte received. The XHR2 specification is different here and states that: we should "dispatch a progress event called progress about every 50ms or for every byte received, whichever is least frequent."
Matching this behaviour was the purpose of bug 18654 (bug 36661 is about bringing this same change to upload progress events).

> Let me reopen this bug as the
> bucket to track the upload progress event firing problem. I've updated the bug
> title to reflect this.

Thanks. You seem to say that this is a regression from bug 18654 (r56394). Maybe worth flagging it as REGRESSION and upgrading it to P1?

[Jian Li]

This seems to be caused by r47291, specially the following change in XMLHttpRequest::createRequest():

    // We also remember whether upload events should be allowed for this request in case the upload listeners are
    // added after the request is started.
    m_uploadEventsAllowed = !isSimpleCrossOriginAccessRequest(m_method, m_requestHeaders);

We should change this check to also allow upload events for same origin request. I am going to fix this.

[Jian Li]

Created attachment 54758 [details]
Proposed Patch

[Jian Li]

Created attachment 54759 [details]
Proposed Patch

Updated Skipped list for platforms that do not support the test.

[David Levin]

Comment on attachment 54759 [details]
Proposed Patch

Yep, it is a regression caused by http://trac.webkit.org/changeset/47291. It looks like there are other cases in which we should be sending progress events but aren't.

However, this is definitely a step in the right direction. Feel free to file a bug on the fact that we probably don't send progress events in some cases (and reference this bug) and assign it to me.

Just a few comments below. If you do any big changes (like making the test interactive, which would be cool if not too hard) to address them, then I'll be happy to re-review.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        XMLHttpRequestUpload events do not fire when sending a raw file or FormData object.
> +        https://bugs.webkit.org/show_bug.cgi?id=37771
> +
> +        Add a layout test to test upload events firing.
> +
> +        * http/tests/local/formdata/resources/send-form-data-common.js:
> +        (dumpResponse):
> +        (sendFormData):
> +        (testSendingFormData):
> +        * http/tests/local/formdata/script-tests/upload-events.js: Added.
> +        * http/tests/local/formdata/upload-events-expected.txt: Added.
> +        * http/tests/local/formdata/upload-events.html: Added.
> +        * platform/gtk/Skipped:
> +        * platform/qt/Skipped:
> +        * platform/win/Skipped:

Per file or function comments would be nice.

For example, you told me why we are skipping the tests on several of these platforms but it would be nice to note that reason in the ChangeLog.

> +
>  2010-04-29  Anton Muhin  <[e-mail]>
>  
>          Reviewed by Darin Adler.
> diff --git a/LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js b/LayoutTests/http/tests/local/formdata/resources/send-form-data-common.js

> +function sendFormData(formDataList, fileSliced, sendAsAsync, xhrSetupCallback)

Consider s/xhrSetupCallback/addEventHandlers/


> diff --git a/LayoutTests/http/tests/local/formdata/script-tests/upload-events.js b/LayoutTests/http/tests/local/formdata/script-tests/upload-events.js

> +var progressEventFired = false;
> +function progressHandler(evt)
> +{
> +    // Dump progress event only once in order to get consistent result.

Consider    // Dump the progress event only once in order to get a consistent result.


> +function setupXMLHttpRequest(xhr, fileSliced)

Consider s/setupXMLHttpRequest/addXHREventHandlers/


> +if (window.eventSender) {
> +    window.jsTestIsAsync = true;
> +    runTest();
> +    // Clean up after ourselves
> +    fileInput.parentNode.removeChild(fileInput);

Why is this needed? (I found it confusing because I didn't see it being added, but then I found that it is added by the "send-form-data-common.js" script.

> +} else {
> +    testFailed("This test is not interactive, please run using DumpRenderTree");

How hard would it be to make it interactive with instructions like "Drag a file to here."?



> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        XMLHttpRequestUpload events do not fire when sending a raw file or FormData object.
> +        https://bugs.webkit.org/show_bug.cgi?id=37771
> +
> +        Test: http/tests/local/formdata/upload-events.html
> +
> +        * xml/XMLHttpRequest.cpp:
> +        (WebCore::XMLHttpRequest::createRequest):

A function level comment would be nice here.

[Jian Li]

All fixed except commented ones and landed as http://trac.webkit.org/changeset/58841.

> > +if (window.eventSender) {
> > +    window.jsTestIsAsync = true;
> > +    runTest();
> > +    // Clean up after ourselves
> > +    fileInput.parentNode.removeChild(fileInput);
> 
> Why is this needed? (I found it confusing because I didn't see it being added,
> but then I found that it is added by the "send-form-data-common.js" script.
This is because we need to exclude it from the result. I changed to put it in a helper
function and moved it to the common js file in order to avoid the confusion.

> 
> > +} else {
> > +    testFailed("This test is not interactive, please run using DumpRenderTree");
> 
> How hard would it be to make it interactive with instructions like "Drag a file
> to here."?
> 
I will investigate how we could make the local file test interactive.