948246 - event.total changes it's value in XMLHttpRequest.upload "progress" event

Status: RESOLVED FIXED

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

Description

[Ioan]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

jsfiddle here: http://jsfiddle.net/Lz3JJ/
Set up a simple file input. Upload it using XMLHttpRequest object with a progress event handler. In the event handler display the reported uploaded and total bytes (e.loaded, e.total).


Actual results:

Uploading C:\Users\Public\Pictures\Sample Pictures\Chrysanthemum.jpg 879,394 bytes:

97803 of 879592 bytes uploaded
261643 of 879592 bytes uploaded
425483 of 879592 bytes uploaded
622091 of 879592 bytes uploaded
654859 of 879592 bytes uploaded
818699 of 879592 bytes uploaded
851467 of 851467 bytes uploaded !


Expected results:

The reported total bytes uploaded should not change during the upload. This change seems to happen only the last time the progress event is triggered.

Reproducible: Sometimes, usually on big files (100Mb+)

Comment 1

[Boris Zbarsky [:bzbarsky]]

That fiddle doesn't seem to have anything with XHR or upload events on it, right?

That said, it looks like we base out mUploadTotal on the initial size of the request body but in the end set it to mUploadTransferred (which we set to the actual transferred byte count as reported by the networking library minus the difference between our request body size and the total size the networking library thinks it's planning to upload.  If it has this last information.

So this could fail if the networking library lies to us or if it has no idea how many bytes it's planning to upload.  Which one is happening here is hard to tell without a testcase...

Comment 2

[Ioan]

Attachment: test file to reproduce the bug on localhost

Comment 3

[Ioan]

My bad. Try this fiddle: http://jsfiddle.net/Lz3JJ/2/

-or-

install on localhost the attached file: https://bugzilla.mozilla.org/attachment.cgi?id=8345275

With a large file, 143Mb, I managed to get 3 different total sizes: one for the first "progress" event, the second size for the rest of events and the third for the last "progress" event.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ioan, thank you for the testcase!

So I've managed to reproduce the problem at the end of the upload, using a file created like so:

  dd if=/dev/zero of=/Users/bzbarsky/zero.txt bs=16384 count=8100

(about 130MB).  What I see happen is that a bunch of OnProgress() calls all of which have 132711145 as aProgressMax and aProgress values going up to 132664432.  The events we dispatch have 132710615 as the .total value, since that's the size of the data we're actually trying to set; the remaining 530 bytes are headers.  We're storing this value in mUploadTotal.

Then we get to OnStartRequest, and we dispatch one final progress event.  This one has .total set to 132663902, which is precisely 530 bytes less than the last aProgress value we saw.  And the reason that happens is that right before doing this call:

    DispatchProgressEvent(mUpload, NS_LITERAL_STRING(LOAD_STR),
                          true, mUploadTotal, mUploadTotal);

we call MaybeDispatchProgressEvents(true), which does:

  if ((XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT) & mState) {
    if (aFinalProgress) {
      mUploadTotal = mUploadTransferred;
    }

mUploadTransferred is set based on the last aProgress value we saw in OnProgress.  So any time we don't get an OnProgress after the end of the upload but before the server response comes in we'll get this at least this situation.

The reason we set mUploadTotal here is entirely unclear to me.  This code was added in bug 687087.  Olli, do you recall what the story is here?  It seems to me like we should never clobber mUploadTotal: we're doing the uploading, so we know exactly how much data there is!

I still have no idea why the size would be different the first time, unless necko tells us it has a non-computable size or something.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Hmm, need to test chunked-* and progress events, and how .total behaves there.

Comment 6

[Boris Zbarsky [:bzbarsky]]

For _uploads_?  Why would any of that be relevant for the upload case?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Uh, right. Silly me.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Ok, this is related to the fix in Bug 689328 and mUploadProgressMax = mUploadProgress was removed in 
Bug 845631.

Patch coming. I think setting mUploadTotal is just bogus and wrong.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=086173582ed7

Per spec there needs to be at least one progress event.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8357916 [details] [diff] [review]
patch

Yes, agreed.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Somehow I managed to upload the initial patch. Not this one with tests and
forcing the progress event. This is the one I pushed to try

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8358046 [details] [diff] [review]
patch

r=me

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2614c1884c63

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2614c1884c63