310552 - Download Manager shows no progress unless download is cancelled.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Michael Graubart]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050915 SeaMonkey/1.1a
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050915 SeaMonkey/1.1a

If a download is started from any site that has downloadable files (e.g. the
SeaMonkey nightly builds), the Download Manager opens but shows no progress. In
fact, however, the download is proceeding normally. If 'Cancel' is clicked, the
Download Manager shows the progress that had been achieved before cancellation.
I have observed this misbehaviour in SeaMonkey builds 20050928 and 20050930
(trunk, Mac), but build 20050915 does not show this malfunction.

This seems to be a regression. I have found bug reports from 2002 and 2003
showing similar behaviour, e.g. Bugs nos. 215894 and 295244 (to which I added
comments before noticing how old they were). I apologize if this is therefore a
duplicate, but I have not found a recent report of this bug.

Reproducible: Always

Steps to Reproduce:
1. In any site providing downloadable files, start the download.
2. Either wait for a considerable time,
3. or click "Cancel'.

Actual Results:  
Under 2., the Download Manager shows no progress, but the download will have
been satisfactorily completed.

Under 3., the download may have been completed or cancelled; either way, the
Download Manager now displays the progress up to completion or the moment of
cancellation, as the case may be.

Expected Results:  
In every case, the Download Manager should show progress as soon as downloading
begins, and at the conclusion should show that it has been completed.

eMac G4, OS X 10.9.3, Classic theme.

Comment 1

[zug_treno]

Confirming, WFM with SeaMonkey/20050927 but reproducible with
SeaMonkey/20050928, -29 and -30 (-> All/All). This seems to be a very recent
regression so I assume this bug isn't a duplicate of those older bugs. The
progress dialog that opens when you click the properties button still works.

Comment 2

[Bruno 'Aqualon' Escherl]

It's also in the branch builds, confirming with Mozilla/5.0 (Windows; U; Windows
NT 5.1; en-US; rv:1.8b5) Gecko/20050929 SeaMonkey/1.0b

Comment 3

[Hermann Schwab]

(In reply to comment #1)
> Confirming, WFM with SeaMonkey/20050927 but reproducible with
> SeaMonkey/20050928, -29 and -30 (-> All/All). 

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-27+00%3A00&maxdate=2005-09-28+07%3A00&cvsroot=%2Fcvsroot

Maybe regressed from checkin for 
Bug 281709 - Buffer view visibility changes like resizes. r/sr=roc

Bug 281709 Under at least Windows XP, shadows are not drawn under menus on first
appearance

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Bug 281709 - Buffer view visibility changes like resizes. r/sr=roc

This was backed out.  Is this still a problem in today's builds?

Comment 5

[Boris Zbarsky [:bzbarsky]]

More likely, this is a duplicate of bug 310417

Comment 6

[Michael Graubart]

Re Comment #4: In Build 2005100209 (trunk, Mac), I find that this bug is still
present.

(eMac G4, OS X 10.9.3, Classic theme)

Comment 7

[Michael Graubart]

Sorry! Meant OS X 10.3.9!

Comment 8

[Adam Guthrie]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, the freeze-thaw we do in bug 305032 for some reason causes this (tested via
local backout).  I don't know why yet, and I'm not going to be able to look
until late tomorrow afternoon.  But we probably need to figure out exactly
what's broken here for 1.8b5...

Comment 10

[Asa Dotzler [:asa]]

I'm unable to reproduce this with Firefox branch builds on Mac, Windows, or
Linux. Is this Seamonkey only?

Comment 11

[Asa Dotzler [:asa]]

This appears to be seamonkey only and we're locking things down tonight so not
going to block on this.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Renominating -- this is a core DOM bug that'll bite any C++ caller who wants to
use windows.  The problem is that resolving a prop on a window in JS
instantiates an inner, but QI from C++ doesn't do anything like that.  The C++
caller in question does:

  win = window.open();
  win.addEventListener();

This throws because there is no inner.  It should probably force inner creation
instead (and we should think about other FORWARD_TO_INNER methods to see whether
they should be doing the same).

Asa, just because the steps to reproduce only happen in one app doesn't mean the
bug is app-specific, dammit.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So to make it clear, there is a simple and I believe safe fix here. I won't be
able to post it until tomorrow afternoon, however.

Comment 14

[Scott MacGregor]

jst, bz isn't going to be back until tomorrow when it's too late. Any chance you
can help out here?

Comment 15

[Asa Dotzler [:asa]]

bz, we have tens of thousands of bugs. unless you can show me how this impacts
our Firefox and Thunderbird users, it's not going to block this release.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is a major platform regression -- various DOM methods that should be
available on windows suddenly are not if you access the window after opening it
from C++.  It affects any user who happens to want to install any extension that
has a C++ part to it and that tries to do this to windows.  The only reason we
didn't catch it right away after splitwindow landed was that it was being hidden
by the fact that any window that had arguments worked, for bogus reasons.  But
changing the arguments behavior makes this a problem.

So what it comes down to is that while you apparently only care about Firefox
and Thunderbird, I care about Gecko and our platform story.  Since we're talking
about a Gecko milestone (1.8b5 is that, last I checked), and since after this
milestone I don't expect drivers to take changes to this code based on past
communications, if we're going to fix this platform regression at all, it needs
to be fixed for 1.8b5.

I'm not going to bother rerequesting approval on this, because I'm rapidly
approaching just writing 1.8 off completely and moving on with life.

Comment 17

[Scott MacGregor]

If jst doesn't have cycles, maybe you can help look at this too blake (I see you
are already cc'ed)

Comment 18

[Johnny Stenback (:jst)]

Attachment: Wrong file...

This makes us create a new inner window if AddEventListener(),
AddGroupedEventListener(), or GetListenerManager() is called. Those are the
only ones of the methods that forward to the inner window where it makes sense
to create a new inner window if one doesn't already exists. The other methods
that forward to the inner are HandleDOMEvent(), SetScriptsEnabled() (which
doesn't actually enable or disable scripts, just runs timeouts if necessary),
DispatchEvent(), RemoveGroupedEventListener(), RemoveEventListenerByIID(),
GetObjectProperty(), SetTimeoutOrInterval(), ClearTimeoutOrInterval(),
SuspendTimeouts(), and ResumeTimeouts(), none of which should IMO create an
inner. I also made HandleDOMEvent() and DispatchEvent() return NS_OK in stead
of an error if there is no inner window.

Comment 19

[Johnny Stenback (:jst)]

Attachment: Create inner windows when necessery

Right diff this time.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198382 [details] [diff] [review]
Create inner windows when necessery

r+sr=bzbarsky.	Thanks for dealing with this, Johnny.  I owe you one.

Requesting approval; this should be reasonably safe; only changes the behavior
of methods that mess with event listeners on newly-opened windows, and fixes
them to work instead of throwing.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198382 [details] [diff] [review]
Create inner windows when necessery

>Index: dom/src/base/nsGlobalWindow.cpp
>+      nsresult fwdic_nr = GetDocument(getter_AddRefs(doc));                   \

This could really just be |nsresult rv| since you're in a new scope and don't
have to worry about conflicting with the rest of the function. Please remember
to test to make sure that this doesn't break focus on 2nd and 3rd windows as
well.

r=mrbkap

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 198382 [details] [diff] [review]
Create inner windows when necessery

Thanks for the quick turnaound on this Johnny.

Comment 23

[Michael Graubart]

Just downloaded Build 2005100410, and Download Manager works again.

Comment 24

[Hermann Schwab]

Bug was fixed:
2005-10-03 18:08	jst%mozilla.jstenback.com 	mozilla/ dom/ src/ base/
nsGlobalWindow.cpp 	1.790 	26/8  	Fixing bug 310552. Fix window.addEventListener() 

Worksforme is the resolution for Bugs where it is not known how they got fixed.
Reopening

Comment 25

[Hermann Schwab]

marking fixed based on Reporter's comment 23

Comment 26

[Boris Zbarsky [:bzbarsky]]

jst landed this on branch too.

In general, for future reference, it's up to the person fixing the bug to decide
when to resolve it...