395134 - When resuming a download, strange things happen in the UI

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

Currently, pausing and resuming uses resumable downloads, when possible.  However, this results in a crazy jump in the download speed, as well as no state when the download is paused.

This *really* needs to be fixed before we ship.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Just incase my machine runs out of power..

Comment 2

[Ed Lee :Mardak]

Comment on attachment 279861 [details] [diff] [review]
v1

From my 1 download test, it seems to be working. Sleeping the laptop.. 3% :P

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/show_test.cgi?id=4577 and http://litmus.mozilla.org/show_test.cgi?id=4630 (the latter is a better stress test).

in-litmus+

Comment 4

[Shawn Wilsher :sdwilsh]

This helps a lot, but if I pause it a second time, I get no status still.

Comment 5

[Ed Lee :Mardak]

Comment on attachment 279861 [details] [diff] [review]
v1

<sdwilsh> Mardak: r=me, but talk to mconnor if he wants it in

mconnor: This patch only fixes the insanely huge GB/sec issue, but the patch for bug 394263 should fix the blank status issue.

This could land to avoid some future bugs people will file when they try out the new download resume and see the obviously wrong download speeds.

Comment 6

[Michael Ventnor]

Edward, you probably should request approval-1.9 on the patch so mconnor can mark his decision.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 279861 [details] [diff] [review]
v1

naw, this isn't really right either.

Something is funky...

Comment 8

[Ed Lee :Mardak]

What's funky about it?

Comment 9

[Ed Lee :Mardak]

My patch for bug 385734 will fix probably fix the blank status issue.

Comment 10

[Ed Lee :Mardak]

Attachment: v2

Apparently the ProgressChange is getting a ton of updates when resuming - basically it's starting from 0 and counting up really quickly to where it was before.

biesi: I might have been wrong thinking ProgressChange was starting at 0 because WBP was saying the resumed position is the new 0. But this makes me wonder if the download is actually resuming at the correct spot or just happens to be downloading really quickly from cache.... partial cache? Or more likely, it /is/ resuming, but how ProgressChange works, it'll start from 0 and send progress updates until it reaches it current file size? ???


This plus patch from bug 385734 has download speed and progress displaying fine in the UI without over 100 percentages and status isn't blank when pausing.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

is that with or without the patch in bug 395205?

Comment 12

[Ed Lee :Mardak]

Comment on attachment 280039 [details] [diff] [review]
v2

Oh, didn't know about bug 395205. Seems like with your patch, it /does/ restart from 0 but without massive jumps.

My patch won't work then, but the original one will likely work.. with the addition of a s/aCurTotalProgress/mCurBytes/

Comment 13

[Ed Lee :Mardak]

Attachment: v3

Soo... apparently aMaxTotalProgress is also based on the new request's starting position....... for HTTP requests.

biesi: Which one is right? HTTP's Max starts at <real>-<resumedPos> while FTP's Max is always <real> before and after resuming.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 280071 [details] [diff] [review]
v3

>@@ -2110,6 +2106,11 @@ nsDownload::PauseResume(PRBool aPause)
>     rv = resumableChannel->ResumeAt(fileSize, mEntityID);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    // Track where we resumed because progress notifications restart at 0
>+    mResumedAt = fileSize;
>+    mCurrBytes = 0;
>+    mMaxBytes = LL_MAXUINT;
>+
You know, I think it might be good to have an nsDownload::ResumeAt method that abstracts it out (this will help with cross session resumable downloads too so there is less logic duplication ;))

Comment 15

[Ed Lee :Mardak]

(In reply to comment #14)
> >+    // Track where we resumed because progress notifications restart at 0
> >+    mResumedAt = fileSize;
> >+    mCurrBytes = 0;
> >+    mMaxBytes = LL_MAXUINT;
> You know, I think it might be good to have an nsDownload::ResumeAt method that
> abstracts it out (this will help with cross session resumable downloads too so
> there is less logic duplication ;))
I would have thought the cross session resume would just reuse this whole function -- instead of canceling downloads at application quit, we pause them (we would only prompt the user if we have a download that we can't resume).

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

The current behavior of cancelling, which is to delete the intermediate file, clearly isn't going to work.  So either we need to make cancel keep the intermediate and force the user to delete by hand through the OS, or pause on quit instead.

Which of these we do might depend on whether we want to automatically continue downloads on restart, or prompt.

Comment 17

[Ed Lee :Mardak]

FTP uses the SIZE request to report the max (*full* size), but progress starts at 0 when resuming.
http://mxr.mozilla.org/mozilla/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1909
// Ignore the progressMax value from the socket.  We know the true size of
// the file based on the response from our SIZE request.
mChannel->OnTransportStatus(nsnull, status, progress, mFileSize);

HTTP uses the response headers to report the max (*partial* size), and progress starts at 0 when resuming.
http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4474
// mResponseHead may reference new or cached headers, but either way it
// holds our best estimate of the total content length.  Even in the case
// of a byte range request, the content length stored in the cached
// response headers is what we want to use here.
nsUint64 progressMax(PRUint64(mResponseHead->ContentLength()));

Could probably hack around this in the download manager if necessary... check if MaxTotalProgress changed.. if so, we know to add ResumedAt to both curr|maxBytes.. if not, only add to currBytes.



About quitting with downloads, we can count the number of downloads that we can't resume (hopefully none these days) and let the user know how many of those there are. For the resumable ones, we just pause them and set a flag saying they were autopaused. On resume, we can resume the autopaused downloads.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

please file a bug on ftp; it should do what HTTP does.

Comment 19

[Ed Lee :Mardak]

Comment on attachment 280071 [details] [diff] [review]
v3

With the patch from bug 395539, ftp transfers display correctly.

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 280071 [details] [diff] [review]
v3

r=sdwilsh

Comment 21

[Ed Lee :Mardak]

Attachment: unbitrot

Bug 395207 fixed up the WBP flag.

Comment 22

[Ed Lee :Mardak]

Btw, interdiff lies ;) I'm not changing it back to pre bug 395207 -- I'm just not changing it at all.

This blocks bug 396448 in terms of patch landing order.

Comment 23

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.115; previous revision: 1.114
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.40; previous revision: 1.39

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007092705 Minefield/3.0a9pre