Bug 37771

Summary: XMLHttpRequestUpload events do not fire when sending a raw file or FormData object
Product: WebKit Reporter: eLod <pota>
Component: XMLAssignee: Jian Li <jianli>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, jchaffraix, jianli, steffen.weber
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Attachments:
Description Flags
Proposed Patch
jianli: commit-queue-
Proposed Patch levin: review+, jianli: commit-queue-

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-
DetailsFormatted DiffDiff
Proposed Patch (9.34 KB, patch)
2010-04-29 17:25 PDT, Jian Li 		levin: review+
jianli: commit-queue-
DetailsFormatted DiffDiff
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.