1187239 - XHR upload progress event does not reach 100% in FireFox until the beginning of the server response is received

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[guohonglin1984]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20150624141335

Steps to reproduce:

Steps to reproduce:

1.I am using Dojo Progress Bar to show document upload progress
2.Adding documents in Firefox (file size is 1~2M or smaller,largre size can not reproduce)
3.Progress bar can not reach 100%
4. Debug in firebug and found that in Firefox ESR 24,the progress 100 % event is not fired. In ESR 31 and 38,found that 100% event is fired, but 100% percentage does not render in FireFox browser.
Same code run on Chrome or IE 11, does not find this issue.




Actual results:

Progress bar can not reach 100%



Expected results:

Show 100% progress correctly

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thank you for the report.  This sounds like an issue in XHR processing, not the DevTools, unless I am misunderstanding.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Can you please link to a page showing the problem for you?

Comment 3

[guohonglin1984]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO July 27 - 29) from comment #1)
> Thank you for the report.  This sounds like an issue in XHR processing, not
> the DevTools, unless I am misunderstanding.

yes,it is. Component should be DOM

Comment 4

[guohonglin1984]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Can you please link to a page showing the problem for you?

it is our product under development,so no page can be linked.

and I found that "load" event of XHR upload is not fired in Firefox ESR 24,31,38.

code is like:

this.xmlHttpRequest.upload.addEventListener("load", myfunction, false);

as load event is fired when the request has successfully completed. it is a work around for 100% progress event. but it is not fired. 

And I found that some one reported this issue in stackoverflow 
http://stackoverflow.com/questions/17932058/progress-event-in-firefox-with-ajax-does-not-reach-100

Comment 5

[Boris Zbarsky [:bzbarsky]]

> it is our product under development,so no page can be linked.

OK, that makes it rather hard to do anything with this bug report...

> code is like:

So I just tried simple code like that.  See http://web.mit.edu/bzbarsky/www/testcases/xmlhttprequest/simple-upload-load-listener.html which shows us firing the "load" event on the upload object just fine.  Not surprising, since our automated test suite tests this too.

So what's different about your case that causes the load event to not fire?

And is the load event thing really related to the progress event issue this bug was initially filed on?

Anyway, I tried some testcases with progress listeners as well:

http://web.mit.edu/bzbarsky/www/testcases/xmlhttprequest/simple-upload-progress-listener.html
http://web.mit.edu/bzbarsky/www/testcases/xmlhttprequest/medium-data-upload-progress-listener.html
http://web.mit.edu/bzbarsky/www/testcases/xmlhttprequest/large-data-upload-progress-listener.html

And they all work correctly as far as I can tell, in Firefox 39.  Any information on how to actually reproduce the problem you're seeing would be very much appreciated

Comment 6

[guohonglin1984]

Attachment: sample project to reproduce this issue.zip file.

Comment 7

[guohonglin1984]

Attachment: sample project to reproduce this issue.war file.

Comment 8

[guohonglin1984]

Attachment: UseThisFileToAddToReproduceIssue.doc

Comment 9

[guohonglin1984]

Hi,

I have created a sample project to reproduce this issue on Firefox.Please take a look and accept this issue as one Firefox defect.

I attached zip/war files of my sample project and the document used to add in to reproduce this issue.

Steps to reproduce:

1. Deploy the project into Tomcat(My environment is Tomcat 6)
2. Access to  http://servername:portnumber/SampleProject/ in browser(My environment is FF ESR 38.2.1)
3. Click "Browse" button and choose the document I attached which size is 3.48M .(you can also use other files which size is several megabytes or smaller,larger size file can not reproduce this issue on Firefox)
4. Click "Upload" button.(Progress bar reach to 97% and STOP.this percentage number may be different for every file and every time.Waiting for 10 seconds,progress bar reached 100%)
5. Function in this sample project is 
       I. added the selected file into one temp folder on server
       II. When adding file is completed, wait for 10 seconds.
       III.Then delete temp file and temp folder.

so correct behavior is progress bar reaches 100% before waiting 10 seconds because the file uploading has completed. Now progress bar on Firefox is waiting there for 10 seconds and then reach to 100%. It is not correct. it seems that 100% progress event is blocked until the action in server side is finished and XHR request is finished. 

6. Do these same steps on Chrome(My environment is version 42), the behavior is correct.

expected result:

Upload progress bar reach to 100% when uploading is completed in Firefox.


if you have any questions,please let me know,Thank you!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Will need to find someone with a Tomcat setup.  :(

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, wait.  No, none of that is needed, I expect.

So the issue, basically, is that the last progress event is not fired until the server actually responds.  That's entirely plausible given the way necko sends its notifications, I expect.  Honza, can you check whether necko would fail to send the last upload progress notification until OnStartRequest in this sort of situation?

Comment 12

[guohonglin1984]

Hi,
Any update for this problem? As our product will be released recently and it's a critical defect for our product. so I'd like to know when it can be fixed and included in Firefox, Thank you!

Comment 13

[Honza Bambas (:mayhemer)]

Patrick, could you help here please?  Comment 11.  Thanks.

Comment 14

[Patrick McManus [:mcmanus]]

this is a fun bug - thanks

the input stream is basically being queried re-entrantly for its status during its output method.. so the counters haven't been updated yet, which makes each status update be one update behind where it should be - and the last one is dropped. (the first one is reported as 0, which is a tell). The progress listener cleans it up when the response arrives.

I'll figure out a fix.

Comment 15

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d3115829a7c

Comment 16

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2343b2687b10

Comment 17

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f757567ec592

Comment 18

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2fa358b68b2

Comment 19

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9333c3a0148b

Comment 20

[Patrick McManus [:mcmanus]]

there is an xhr issue in here too, where it essentially relies on the upload events being short of a full deck and if they are fixed to be correct it suppresses the last notification before the state is changed to ready.

Comment 21

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb15b1815496

Comment 22

[Patrick McManus [:mcmanus]]

re comment 20, testing/web-platform/tests/XMLHttpRequest/event-upload-progress.htm is a test (among many) that breaks if upload.onprogress() is not delivered before readystate changes in OnStartRequest

Comment 23

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=062c47b34208

Comment 24

[Patrick McManus [:mcmanus]]

there are some jetpack fails in the comment 23 try run, but I made another run without the patch and they have the same problem

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ae97d5c38d

Comment 25

[Patrick McManus [:mcmanus]]

Attachment: ontransportstatus SENDING_TO should not use request stream re-entrantly

Comment 26

[Patrick McManus [:mcmanus]]

boris, I r?'d you for the XHR portion

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8662512 [details] [diff] [review]
ontransportstatus SENDING_TO should not use request stream re-entrantly

Review of attachment 8662512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsXMLHttpRequest.cpp
@@ +3497,5 @@
>      mUploadLengthComputable = lengthComputable;
>      mUploadTransferred = loaded;
>      mProgressSinceLastProgressEvent = true;
>  
> +    MaybeDispatchProgressEvents((mUploadTransferred == mUploadTotal));

without this change, XHR always dispatches the last upload progress event either
1] off a timer
or 2] from onstartrequest if necko hasn't sent it proper progress reports yet

Prior to this patch, necko generally didn't send the last upload progress event at all, so #2 was the common case. But upon fixing it things reverted to the timer, which isn't good if you want the upload progress events to preceed the readystate change (which one of the web platform tests checks).

::: netwerk/base/nsTransportUtils.h
@@ -16,5 @@
>   * be delivered.  The progress reported will be that from the second event.
> - * If aCoalesceAllEvents is true, then any undelivered event will be replaced
> - * with the next event if it arrives early enough.  This option should be used
> - * cautiously since it can cause states to be effectively skipped.  Coalescing
> - * events can help prevent a backlog of unprocessed transport events in the

I removed the ability of necko to sometimes coalesce events across state changes. It was a micro-optimization that added a lot of complexit.

Coalescing lots of progress events of the same type makes sense, but the status events are important markers - for instance the devtools tests are basically confused because they cannot count on receiving these. And the status transitions are not the bulk of the data.

Furthermore, the receiver would get very different data streams depending on the state of other components and whether they had registered activity distributors - so behavior was different for xhr depending on whether devtools was open or not. The interface is better as consistent, and I don't think the optimization was a big one.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +346,5 @@
>                                 mReqHeaderBuf.Length());
>      if (NS_FAILED(rv)) return rv;
>  
> +    mHasRequestBody = !!requestBody;
> +    if (mHasRequestBody) {

this is another latent bug. We don't send upsteam onprogress notifications to channels without a request body, but some methods without a body other than GET set the uploadstream from XHR along with a content-length of 0. Because the underlying bug here often dropped the last upload onprogress notification it papered over the fact that the test for mHasRequestBody needs to be more than if just any stream was provided.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8662512 [details] [diff] [review]
ontransportstatus SENDING_TO should not use request stream re-entrantly

r=me on the XHR part, I think...  That code is finicky.  :(

Comment 29

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a56db89602545da44a7dd7396a683ddf1d695f1
bug 1187239 - ontransportstatus SENDING_TO should not use request stream re-entrantly r=hurley r=bz

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2a56db896025

Comment 31

[guohonglin1984]

Hi,
When Firefox 43 can be released with this fix ? and is there plan to include this fix in ESR version ？

Comment 32

[Boris Zbarsky [:bzbarsky]]

The release schedule is at https://wiki.mozilla.org/RapidRelease/Calendar#Future_branch_dates -- Firefox 43 will be released near end of January 2016.

I don't expect the ESR drivers would approve this fix; they typically take only fixes for sufficiently severe security bugs.

Comment 33

[Patrick McManus [:mcmanus]]

fwiw I think firefox 43 is in mid december..

anyhow, this is essentially a 10 year old bug so I expect it will ride the trains in the usual way without any backports.