WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
37771
XMLHttpRequestUpload events do not fire when sending a raw file or FormData object
https://bugs.webkit.org/show_bug.cgi?id=37771
Summary
XMLHttpRequestUpload events do not fire when sending a raw file or FormData o...
eLod
Reported
2010-04-18 03:15:01 PDT
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.
Attachments
Proposed Patch
(7.63 KB, patch)
2010-04-29 17:19 PDT
,
Jian Li
jianli
: commit-queue-
Details
Formatted Diff
Diff
Proposed Patch
(9.34 KB, patch)
2010-04-29 17:25 PDT
,
Jian Li
levin
: review+
jianli
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
eLod
Comment 1
2010-04-18 04:38:52 PDT
same with nightly
r57791
Alexey Proskuryakov
Comment 2
2010-04-18 11:27:18 PDT
I see upload progress events when sending a string, but not with FormData indeed.
Jian Li
Comment 3
2010-04-28 14:56:05 PDT
*** This bug has been marked as a duplicate of
bug 36661
***
Alexey Proskuryakov
Comment 4
2010-04-28 15:01:39 PDT
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
Comment 5
2010-04-28 15:10:44 PDT
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
Comment 6
2010-04-28 17:55:23 PDT
(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
Comment 7
2010-04-28 18:08:10 PDT
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
Comment 8
2010-04-28 18:33:26 PDT
> 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
Comment 9
2010-04-29 15:45:18 PDT
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
Comment 10
2010-04-29 17:19:13 PDT
Created
attachment 54758
[details]
Proposed Patch
Jian Li
Comment 11
2010-04-29 17:25:00 PDT
Created
attachment 54759
[details]
Proposed Patch Updated Skipped list for platforms that do not support the test.
David Levin
Comment 12
2010-05-05 13:43:40 PDT
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 > +2010-04-29 Jian Li <
jianli@chromium.org
> > + > + 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 <
antonm@chromium.org
> > > 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 > +2010-04-29 Jian Li <
jianli@chromium.org
> > + > + 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
Comment 13
2010-05-05 15:46:22 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug