289724 - Missing progress bar animation in download window

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050405 Firefox/1.0+ (PowerBook)
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050409 Firefox/1.0

The latest Trunk build no longer display a progress bar animation when you save
a file to disk.
The progress bar is there, but its static. Its not animating...

Reproducible: Always

Steps to Reproduce:
1. Click Save Link As.. 
2. Open the Download window
3. Watch the progress bar

Actual Results:  
No progress bar animation

Expected Results:  
Display progress bar animation

Comment 1

[riscky]

*** Bug 289754 has been marked as a duplicate of this bug. ***

Comment 2

[[e-mail]]

I'll add that it dont display the text for the remaining time, download speed or
amount of data either.
The window title say "Download" instead of "20% of 1 file - Downloads" as well.

Comment 3

[[e-mail]]

Attachment: Downloads

Comment 4

[[e-mail]]

Comment on attachment 180252 [details]
Downloads

Screenshot to show what it looks like.

Comment 5

[Arthur Wiebe]

Confirmed on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2)
Gecko/20050412 Firefox/1.0+

Comment 6

[Peter van der Woude [:Peter6]]

kwd->regression

Comment 7

[philippe (part-time)]

As noted in duped bug 289754, I noticed the problem first with the 20050407
official nightly build.

Comment 8

[Frederic Bezies]

I am seeing it too. Pretty bad, because you don't know how much you've downloaded :(

Could it be fixed for gecko 1.8b2, please ?

Comment 9

[[e-mail]]

It started with the 20050406 nightly build (working fine in the 20050405 build).

Comment 10

[Steve England [:stevee]]

*** Bug 290138 has been marked as a duplicate of this bug. ***

Comment 11

[Peter van der Woude [:Peter6]]

regression from bug 283489 ?

Comment 12

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

hmm. any messages in JS Console? also, how are you saving, "save link target as"
or by just left clicking the file?

Comment 13

[Frederic Bezies]

Erh ;o)

JS console is empty when clicking on a download link - and I have to open it first (15 hours old 
homemade build). Also, I cannot get download window in front if JS console window is opened.

Save link as ?

Just name of file and a "starting" line under it. Cancel line is available in both cases.

JS console ? 

Empty.

Should I have to do a debug build ?!

Comment 14

[philippe (part-time)]

(In reply to comment #12)
> hmm. any messages in JS Console? also, how are you saving, "save link target as"
> or by just left clicking the file?

Downloading the latest nightly from
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/
Click on the link, download manager comes up, ask what to do with the file.
click OK, 'downloads' window appears.

Same happens with 'Save Page as'

JS Console is empty in all cases.

javascript.options.showInConsole pref to true, or false, doesn't matter.

Comment 15

[Michael Graubart]

It might be relevant that (at any rate in Build 2005041309), if the 'Progress
Dialog' is selected in Preferences, this does work, even though the 'Download
Manager' does not.

Comment 16

[Michael Graubart]

One more observation that might conceivably help someone to fix this: if you
have the Download Manager open while downloading something, and you click
'Cancel', the percentage and number of KB downloaded appear.

Comment 17

[Arthur Wiebe]

We need to find out exactly what checkin caused this. If I have time I will look
into the regression. (Find out what regressed)

Comment 18

[Arthur Wiebe]

Just one more comment :)

According to the info Kai gave, the problem checkin should be one of these:

<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD
&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date
&hours=2&date=explicit&mindate=2004-04-05+10%3A51%3A00&maxdate=2004-04-06+15%3A17%3A00
&cvsroot=%2Fcvsroot>

Comment 19

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

(as unwrapped url:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-04-05+10%3A51%3A00&maxdate=2004-04-06+15%3A17%3A00&cvsroot=%2Fcvsroot
)

Comment 20

[Mark Mentovai]

That Bonsai query is from last year.  Try this:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-04-05+10%3A51%3A00&maxdate=2004-04-06+15%3A17%3A00&cvsroot=%2Fcvsroot

Looks like bug 228968.  That brought 64-bitness to the download progress
dispalay, to handle proper size reporting for files larger than 2GB.

(click click click)

Yup.  There's your problem.  228968.  Once things went 64-bit, some corruption
was introduced into the data path.

The numbers are fine in
toolkit/components/downloads/src/nsDownloadManager.cpp:OnProgressChange64. 
Something's getting mucked up by the time it's on the far side of XPIDL, though.
 Once it hits
toolkit/mozapps/downloads/content/DownloadProgressListener.js:onProgressChange,
aCurTotalProgress and aMaxTotalProgress are screwed up, and aDownload is
unusable.  aCurTotalProgress and aMaxTotalProgress show up shifted 32 bits left
(or low/high32 swapped, same thing for |values| under 2G).  Curiously,
aCurSelfProgress and aMaxSelfProgress both are fine all the way through.

With aDownload unusable, onProgressChange kicks out at the first unchecked
referenece, right below the comment that says "Update download rate."  The rest
of that method is responsible for making changes to the download manager window,
and it never gets a chance to run.

Restoring toolkit/components/downloads/public/nsIDownloadProgressListener.idl so
that the relevant fields are 32 bits covers up the problem.  Obviously, this
isn't a solution, but it points to something not being 64-bit-clean.  JS? 
XPIDL-to-JS marshalling?  Are they not 64-bit-clean on the Mac?  Does this bug
not affect other platforms?

I'll investigate more when I get another chance.

Comment 21

[Mark Mentovai]

Pasted the wrong Bonsai query (again) - one more try.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-04-05+10%3A51%3A00&maxdate=2005-04-06+15%3A17%3A00&cvsroot=%2Fcvsroot

Comment 22

[Arthur Wiebe]

Oops, no wonder I couldn't find anything using that query :)

Well this is just a guess but the fix for bug 228968 may have caused this. If I
have time I'll try it.

Comment 23

[Mark Mentovai]

Diagnosed and have a fix.  It works, but I want to test it a little bit longer.
 Feel free to reassign to me.  Stay tuned.

Comment 24

[Mark Mentovai]

Attachment: XPCOM/xptcall ABI patch for Mac OS X PPC

When I picked this one up, I didn't expect that I'd have to get so close to the
metal to make it happen.  I guess that's why this it's so satisfying, and why
I'm particularly proud of this one.

Here's what happened.  The fix for bug 228968 turned the numbers used for
download progress tracking into 64-bit integers.  See comment 20.  The xptcall
port to Mac OS X/PowerPC was not friendly with 64 bits, though.  It contained
an implementation that didn't match the Mac OS X/PowerPC ABI.  Specifically, it
enforced natural alignment of 64-bit integers, where the actual ABI has no such
requirement and allows (requires) 64-bit types to be word-aligned in the
parameter section of a stack.  Mozilla was using an implementation of SOME ABI,
but it wasn't the RIGHT ABI.

Here's a good guide to the ABI:
http://developer.apple.com/documentation/DeveloperTools/Conceptual/MachORuntime/PowerPCConventions/chapter_3_section_1.html


As for the strange behavior I saw in comment 20 with some 64-bit numbers making
it through xptcall without problems, that's because the first pair of 64-bit
ints were passed in registers and saved separately; the old code didn't have
trouble pulling them but did have trouble pulling the other numbers from the
stack.	(An interesting bug in the implementation is that for any array of
64-bit ints, depending on the alignment, EITHER the values would be pulled from
registers properly OR they'd be pulled from the stack properly, but not both.)

Nobody noticed this bug up until now, most likely, because 64-bit ints are used
so sparingly, and because if the alignment was right, everything would pass
muster anyway.	(That's alignment of ints, not planets, although I'm sure that
those were a crucial factor too.)

The attached patch contains a revised (fixed, largely rewritten) implementation
of the Mac OS X/PPC ABI for xptcall.  In addition to doing a better job of
tracking the ABI, it fixes a couple of latent bugs.  For example, the 11th,
12th, and 13th floating-point arguments would be trash if anyone ever did
anything as foolish as passing more than 11.  I hope it doesn't introduce any
new ones.  It tests well in Firefox/trunk.  The patch also significantly cleans
up the old code, both in appearance and in function.

Comment 25

[Mark Mentovai]

Attachment: Updated patch

Corrects a problem with float (not double) arguments not in FPRs (after the
13th).

@@ -519 +519 @@
-+	      dp->val.f = (float) theParam;
++	      dp->val.f = *(float *) &argsStack[argIndex];

That's better.

Comment 26

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

great, glad to hear this regression isn't my fault ;-)

Comment 27

[Brian Ryner (not reading)]

Comment on attachment 181112 [details] [diff] [review]
Updated patch

Looks good to me.  Assuming you've tested it thoroughly (it sounds like you
have), r=bryner.

Comment 28

[Mark Mentovai]

Comment on attachment 181112 [details] [diff] [review]
Updated patch

Requesting sr=brendan.

(And someone with cvs access to check in.)

bryner: It's well-tested.  It fixes this bug for Firefox and bug 290797 for
Seamonkey.  Camino works with it, but never exhibited a bug (it's still
truncating to 32 bit ints and it doesn't push download stats through xptcall).

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 181112 [details] [diff] [review]
Updated patch

rs=me, and a= for 1.8b2.

/be

Comment 30

[Asaf Romano (gone)]

checked in 2005-04-19 18:01

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 181112 [details] [diff] [review]
Updated patch

a=asa

Comment 32

[Asaf Romano (gone)]

*** Bug 290797 has been marked as a duplicate of this bug. ***