392793 - invalid (negative) progress percentage displayed in the window title for certain downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Nickolay_Ponomarev]

When downloading https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&product=Core&target_milestone=mozilla1.9+M8&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&resolution=FIXED&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=
(linked from http://www.mozilla.org/projects/firefox/3.0a8pre/releasenotes/ - A full _Gecko 1.9 bug fix list_), the file size cannot be determined and the download manager shows progress values like "-337000% of 1 file" in the window heading.

Firefox 2 deals fine with unknown file size (no percentage progress is displayed).

Comment 1

[Jo Hermans]

Works for me.
in the latest trunk with the new download manager, I see 'unknown time left'.

Comment 2

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

Ditto here, with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007081905 Minefield/3.0a8pre (I see what Jo sees); this WFM.

Nickolay, is this with a corrupted downloads.sqlite file?

Comment 3

[Nickolay_Ponomarev]

Stephen: No, this is with a clean profile.

'Unknown time left' is indeed shown next to the specific item, but the window title has this wrong progress value. I just tested with 2007081905, this is WinXP.

Comment 4

[Nickolay_Ponomarev]

So this code uses dl.size no matter what:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/downloads/content/downloads.js&rev=1.81#373

..and nsIDownload.size returns "LL_MAXUINT" (which gets mapped to -1 in JS for some reason) for downloads of unknown size:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/public/nsIDownload.idl&rev=1.20

The check of size > 0 was removed in the DM rewrite:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/toolkit/mozapps/downloads/content&command=DIFF_FRAMESET&file=downloads.js&rev2=1.69&rev1=1.68

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Try v1.0

Try v1.0

Is this what we need?

Comment 6

[Nickolay_Ponomarev]

It would also be worth checking if the MAX_INT -> -1 mapping for PRUint64 return type is intended...

Comment 7

[Shawn Wilsher :sdwilsh]

Brian, is it the intended behavior of the JS engine to map LL_MAXUINT to -1?

Comment 8

[Brian Crowder]

There is no -1 in unsigned-land.  So, likely yes.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 277401 [details] [diff] [review]
Try v1.0

r=sdwilsh

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

Because this is front end and has a r+ does that mean I can put up a checkin request on it or does it need a second review or whatnot?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Brian, is it the intended behavior of the JS engine to map LL_MAXUINT to -1?

It's XPConnect, not the JS engine. See

http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcconvert.cpp#177

/be

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Because this is front end and has a r+ does that mean I can put up a checkin
> request on it or does it need a second review or whatnot?
My review should be sufficient here.  If I ever feel it isn't, I'll let you know.  Since this is a Firefox UI thing, it also does not need approval yet.

Comment 13

[Nickolay_Ponomarev]

mozilla/toolkit/mozapps/downloads/content/downloads.js  1.84

Now showing "100% of 1 file" in this case.. certainly better than -12345%!

Comment 14

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

http://litmus.mozilla.org/show_test.cgi?id=4596

in-litmus+

If you'd be so kind as to review the testcase when you get a chance, I'd appreciate it!

Comment 15

[Nickolay_Ponomarev]

The test case looks fine, except for me the link cannot be saved as neither on 2.0, nor on a self-built trunk... Don't have the time to track down whether it's due to something in my profile or because of a Firefox bug.

Comment 16

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

(In reply to comment #15)
> The test case looks fine, except for me the link cannot be saved as neither on
> 2.0, nor on a self-built trunk... Don't have the time to track down whether
> it's due to something in my profile or because of a Firefox bug.

I couldn't save it either, which actually means it's probably a good testcase :-) 

Comment 17

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

(In reply to comment #16)
> (In reply to comment #15)
> > The test case looks fine, except for me the link cannot be saved as neither on
> > 2.0, nor on a self-built trunk... Don't have the time to track down whether
> > it's due to something in my profile or because of a Firefox bug.
> 
> I couldn't save it either, which actually means it's probably a good testcase
> :-) 

Looks like this is a regression; bug 393895.

Comment 18

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

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > The test case looks fine, except for me the link cannot be saved as neither on
> > > 2.0, nor on a self-built trunk... Don't have the time to track down whether
> > > it's due to something in my profile or because of a Firefox bug.
> > 
> > I couldn't save it either, which actually means it's probably a good testcase
> > :-) 
> 
> Looks like this is a regression; bug 393895.

I don't really think it's the same bug, actually, so I spun off bug 394278.

Comment 19

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

Verified FIXED with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007082923 Minefield/3.0a8pre; although it goes fast.  Bug 394278's testcase would be the best testcase, but I want to get this verified so we'll know in the future if and when it does regress.

I see it quickly jump from 70% or so to "100% of 1 file", even though it's still downloading, but I don't see the negative percentage in comment 0 and comment 3.