Closed Bug 289724 Opened 19 years ago Closed 19 years ago

Missing progress bar animation in download window

Categories

(Core :: XPConnect, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: krmathis, Assigned: mark)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
*** Bug 289754 has been marked as a duplicate of this bug. ***
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.
Attached image Downloads
Comment on attachment 180252 [details]
Downloads

Screenshot to show what it looks like.
Attachment #180252 - Attachment description: Screenshot to show what it looks like. → Downloads
Confirmed on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2)
Gecko/20050412 Firefox/1.0+
Status: UNCONFIRMED → NEW
Ever confirmed: true
kwd->regression
Keywords: regression
As noted in duped bug 289754, I noticed the problem first with the 20050407
official nightly build.
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 ?
Flags: blocking-aviary1.1?
It started with the 20050406 nightly build (working fine in the 20050405 build).
*** Bug 290138 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1+
regression from bug 283489 ?
hmm. any messages in JS Console? also, how are you saving, "save link target as"
or by just left clicking the file?
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 ?!
(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.

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.
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.
We need to find out exactly what checkin caused this. If I have time I will look
into the regression. (Find out what regressed)
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>
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.
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.
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.
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.
Attachment #181103 - Flags: review?(shaver)
Attachment #181103 - Flags: approval-aviary1.1a?
Attached patch Updated patchSplinter Review
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.
Attachment #181103 - Attachment is obsolete: true
Attachment #181112 - Flags: review?(shaver)
Attachment #181112 - Flags: approval-aviary1.1a?
great, glad to hear this regression isn't my fault ;-)
Assignee: bugs → mark
Component: Download Manager → XPConnect
Flags: review?(shaver)
Product: Firefox → Core
QA Contact: aebrahim-bmo → pschwartau
Version: unspecified → Trunk
Status: NEW → ASSIGNED
Attachment #181112 - Flags: review?(shaver) → review?(bryner)
Attachment #181103 - Flags: approval-aviary1.1a?
Flags: blocking1.8b2?
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.
Attachment #181112 - Flags: review?(bryner) → review+
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).
Attachment #181112 - Flags: superreview?(brendan)
Attachment #181112 - Flags: approval1.8b2?
Comment on attachment 181112 [details] [diff] [review]
Updated patch

rs=me, and a= for 1.8b2.

/be
Attachment #181112 - Flags: superreview?(brendan)
Attachment #181112 - Flags: superreview+
Attachment #181112 - Flags: approval1.8b2?
Attachment #181112 - Flags: approval1.8b2+
checked in 2005-04-19 18:01
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 181112 [details] [diff] [review]
Updated patch

a=asa
Attachment #181112 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
Flags: blocking1.8b2?
*** Bug 290797 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: