XHR2 uploads: progress event doesn't update event.loaded

VERIFIED FIXED in Firefox 32

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 months ago

People

(Reporter: me, Assigned: axel.viala)

Tracking

({regression})

32 Branch
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox32+ verified, firefox33+ verified, firefox34+ verified)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
STR:

1) Open this fiddle: http://jsfiddle.net/jmxQ5/2/
2) Select a file, hit "go"
3) In Firefox 32, event.loaded doesn't get updated after the initial value

I'd say this is a pretty critical bug, and a lot of progress bars will freak out if this hits release.
Reporter

Updated

5 years ago
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: DOM: Events → DOM
(possibly a necko issue if it doesn't send the right notifications to XHR impl - just guessing.)
Last good revision: e017c15325ae (2014-05-28)
First bad revision: b85b57f05fda (2014-05-29)
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e017c15325ae&tochange=b85b57f05fda

Not sure if those are correct:
Last good revision: 809f9bcbd7f2
First bad revision: c1512bf25a70
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=809f9bcbd7f2&tochange=c1512bf25a70
Necko is reporting the right numbers.  XHR is then screwing them up.  Specifically, this code in nsXMLHttpRequest::OnProgress:

    mUploadTransferred = aProgress;
    if (lengthComputable) {
      mUploadTransferred = aProgressMax - mUploadTotal;
    }

Note that when lengthComputable it clobbers mUploadTransferred with a value that doesn't depend on aProgress at all.

This is a regression from bug 1016399.  We should obviously have had tests for this...

Sadly, bug 1016399 doesn't actually say what it's trying to fix.  What _was_ it trying to fix?
Flags: needinfo?(ehsan)
Flags: needinfo?(axel.viala)
Maybe it was trying to get rid of the written-but-not-read "total" variable?

But then why totally change the computation of mUploadTransferred?
I imagine it was trying to remove the unused total and engaged in some overzealous (and wrong) cleanup.
Assignee

Updated

5 years ago
Flags: needinfo?(axel.viala) → needinfo?(sledru)
Flags: needinfo?(sledru)
(In reply to axel.viala from comment #6)
> Maybe this warning was a false positive and posed more problem by solving it.

You didn't just remove total though, you incorrectly changed the calculation of mUploadTransferred.
Assignee

Comment 10

5 years ago
Exactly, I decided on myself to clean up this file after picking up a warning generated with scan build(http://clang-analyzer.llvm.org/scan-build.html).

But as I read now I introduced a bug with my patch.

Here what I submitted with extra comment for explaining the bug introduced.

> --- a/content/base/src/nsXMLHttpRequest.cpp	
> +++ a/content/base/src/nsXMLHttpRequest.cpp	
> @@ -3578,25 +3578,21 @@ nsXMLHttpRequest::OnProgress(nsIRequest 
>  {
>    // We're uploading if our state is XML_HTTP_REQUEST_OPENED or
>    // XML_HTTP_REQUEST_SENT
>    bool upload = !!((XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT) & mState);
>   // When uploading, OnProgress reports also headers in aProgress and aProgressMax.
>   // So, try to remove the headers, if possible.
>  bool lengthComputable = (aProgressMax != UINT64_MAX);
>   if (upload) {
> -    uint64_t loaded = aProgress;   // Here I introduced the error by removing this variable. 
> -    uint64_t total = aProgressMax; // total is not used.
> +    mUploadTransferred = aProgress; 
>      if (lengthComputable) {
> -      uint64_t headerSize = aProgressMax - mUploadTotal; // Here and the following line the problem.
> -      loaded -= headerSize;
> -      total -= headerSize; // It is ok to remove total because it's used nowhere else.
> +      mUploadTransferred = aProgressMax - mUploadTotal;
>      }
>      mUploadLengthComputable = lengthComputable;
> -    mUploadTransferred = loaded; // This line is totally right.
>     mProgressSinceLastProgressEvent = true;
> 
>     MaybeDispatchProgressEvents(false);
>   } else {
>     mLoadLengthComputable = lengthComputable;
>     mLoadTotal = lengthComputable ? aProgressMax : 0;
>     
>     // Don't dispatch progress events here. OnDataAvailable will take care\

The good patch would have been:

> --- a/content/base/src/nsXMLHttpRequest.cpp
> +++ a/content/base/src/nsXMLHttpRequest.cpp
> @@ -3578,25 +3578,21 @@ nsXMLHttpRequest::OnProgress(nsIRequest 
> {
>   // We're uploading if our state is XML_HTTP_REQUEST_OPENED or
>   // XML_HTTP_REQUEST_SENT
>   bool upload = !!((XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT) & mState);
>   // When uploading, OnProgress reports also headers in aProgress and aProgressMax.
>   // So, try to remove the headers, if possible.
>   bool lengthComputable = (aProgressMax != UINT64_MAX);
>   if (upload) {
>     uint64_t loaded = aProgress; 
> -    uint64_t total = aProgressMax; // Here total is not used.
>     if (lengthComputable) {
>       uint64_t headerSize = aProgressMax - mUploadTotal;
>       loaded -= headerSize;
> -      total -= headerSize; // Here that was ok to remove total because it's used nowhere else.
>     }
>     mUploadLengthComputable = lengthComputable;
>     mUploadTransferred = loaded;
>     mProgressSinceLastProgressEvent = true;
> 
>     MaybeDispatchProgressEvents(false);
>   } else {
>     mLoadLengthComputable = lengthComputable;
>     mLoadTotal = lengthComputable ? aProgressMax : 0;
>     
>     // Don't dispatch progress events here. OnDataAvailable will take care

So by the fact I added this bug by making a useless refactore to a clean-up.

I am worried to introduce a bug.
What is the best solution?

Submit a patch, a test and review it?
Or backouting it.
The best solution is to fix the code to work correctly again.  ;)  Whether you do that by restoring it to the state it used to be in or by making it clearer but still correct is totally up to you.  And we need to add a test, of course.

If you have the time to do it this week, that would be great.  If not, please let me know ASAP.
Flags: needinfo?(axel.viala)
Assignee

Comment 12

5 years ago
Posted patch Patch for this bug (obsolete) — Splinter Review
Here the patch for correcting the bug.

Now to move on I can write a test but that's my first time for Firefox.

In this case I must make something like here: http://jsfiddle.net/jmxQ5/2/ ?
How I can do it?
Attachment #8463446 - Flags: review?(bzbarsky)
Flags: needinfo?(axel.viala)
Comment on attachment 8463446 [details] [diff] [review]
Patch for this bug

Please fix the indentation and r=me.

For the test, see https://developer.mozilla.org/en-US/docs/Mochitest and you'll just need to make sure the response comes in two packets.  See the sjs bits in there for something that might help.
Attachment #8463446 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(ehsan)
Assignee

Comment 14

5 years ago
Thanks!

Here the patch with indentation fixed.

I will read the documentation that you give me for test.
Attachment #8463446 - Attachment is obsolete: true
Attachment #8464179 - Flags: review?(bzbarsky)
Comment on attachment 8464179 [details] [diff] [review]
fix_1044584.patch

r=me.  For future reference, no real need to ask for review again if I already said "do this simple thing and r+".  ;)
Attachment #8464179 - Flags: review?(bzbarsky) → review+
Adding the checkin-needed for Axel (he does not have the editbugs permission yet)
Keywords: checkin-needed
Assignee: nobody → axel.viala
Sorry to be a pain, but this needs to run through Try before landing.
Keywords: checkin-needed
Assignee

Comment 19

5 years ago
Hello, I am trying to write a test with Mochitest actually.

I understand that I must write a ServerSide JavaScript, for testing if the response comes in two separate request and on the client testing if loaded is superior to a old_load value or if loaded is equal to the total.

I edited Silverwind script: http://jsfiddle.net/jmxQ5/5/

Here I test that on the client side we increase the loaded.

Thanks.
Reporter

Comment 20

5 years ago
Here's a fiddle with an binary pass/fail result, based on having at least 3 unique values for event.loaded:

http://jsfiddle.net/silverwind/nY5vn/

I'd imagine the sjs would be pretty simple, it most likely just needs to accept the request. I could probably write the Mochitest if you like.
silverwind, axel is out without an internet connection for a few weeks.  Are you willing to write the mochitest?
Flags: needinfo?(silv3rwind)
Duplicate of this bug: 1047978
Oh, I just realized that the test just needs to make sure we have two progress events for the _upload_.  That does not need an SJS at all.  Just upload a long enough string and have the test fail if it does not see multiple progress events.  You can use window.location.href as the URL to upload to, I would assume.

I'll try to just write this up on Monday if silverwind doesn't get to it first.
Flags: needinfo?(bzbarsky)
Reporter

Comment 24

5 years ago
@bz: As this would be my first mochitest, I'd not be sure if I get it right the first try. With the criticality of this bug, I'd prefer you adding this test.

As for the test, I'd say it needs to have at least three progress events. The first and last event are still correct in the failing case.

Also I updated above fiddle with simpler string generation. If time is not an issue for the test, I'd suggest creating at least 1Mb of data for the upload.
Flags: needinfo?(silv3rwind)
I would probably modify add non-optional upload progress events to
http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug435425.html?force=1#346 and then modify http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug435425.html?force=1#28 a bit so that progress on .loaded is checked too.
This is not testing what we want, because we only see one progress event... I can get it to 2 events if I make "large" a lot longer, but we really want 3 of them here.

If no one gets to this before, I'll take a another look when I get back.
Given the recent comments, it doesn't seem like this is ready for landing yet?
Keywords: checkin-needed
I think that recent comments are talking about the test. The patch is fine, tests will come later (but I might be wrong)
Reporter

Comment 29

5 years ago
Correct. The patch is good to go. bz and Axel are both absent right now, but I'm sure bz will follow up with the test.

Please get this landed all the way down to beta.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/901b7d6dcf4b

I attempted to give the patch a useful commit message and cleaned up the committer information on the patch. For the future, please make sure you have Hg configured to generate patches with the appropriate information to make life easier for those landing on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Comment on attachment 8464179 [details] [diff] [review]
fix_1044584.patch

BZ & Axel are both in holidays. Writing the uplift approval for once...

Approval Request Comment
[Feature/regressing bug #]: Bug 1016399
[User impact if declined]: XHR2 uploads are broken
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=3f65f2089f5a
A test should arrive soon
[Risks and why]: Low risk. The previous code was broken. This should fix it
[String/UUID change made/needed]: None
Attachment #8464179 - Flags: approval-mozilla-beta?
Attachment #8464179 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Duplicate of this bug: 1051825
https://hg.mozilla.org/mozilla-central/rev/901b7d6dcf4b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

5 years ago
Duplicate of this bug: 1047430
Comment on attachment 8464179 [details] [diff] [review]
fix_1044584.patch

beta+
Attachment #8464179 - Flags: approval-mozilla-beta?
Attachment #8464179 - Flags: approval-mozilla-beta+
Attachment #8464179 - Flags: approval-mozilla-aurora?
Attachment #8464179 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Reproduced the initial issue on old Nightly build (07.07.2014), verified that the issue is fixed using Firefox 32 beta 8, latest Nightly and latest Aurora on Windows 7 64bit, Mac OS X 10.9.4 and Ubuntu 14.04 32bit.
Olli and I spent some time trying to get the test running in the test harness to give us more than 2 progress events, and failed.  :(

If we figure out a way to do that, we can add a test.
Flags: needinfo?(bzbarsky)

Comment 39

5 years ago
Progress bar stops near end of downloads and then stalls. However, the download is completed. A bit scary. At least the progress bar moves to near the end, before it did not move at all. Next step, to the end and soon I hope.
Joseph, I think you are looking for bug 1057764, not this one.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.