57207 - progress dialog sometimes doesn't go away when opening content with a helper app

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[shrirang khanzode]

I have winamp installed. Windows branch build 20001018.

Steps:
1 Go to the above url
2 Click on any of the "Tune In" buttons
3 Observe that a dialog with options "Open Using(winamp)/Save To Disk" opens up 
with the  'Save to Disk' button selected by default (I think it should be the 
other   button that should be selected by default)
4 Click on the 'Open Using' radio button and press OK
5 Observe that the winamp application is launched AND the SAVING FILE dialog 
also opens up and stays open.

EXPECTED: The SAVING FILE dialog should not open up at all ifI do not select 
that radio button. This is a bad problem.

Comment 1

[shrirang khanzode]

keyword :rtm

Comment 2

[shrirang khanzode]

correcting summary.

Comment 3

[selmer (gone)]

Scott, this seems nasty enough to look for a small safe fix.

Comment 4

[Scott MacGregor]

I don't see this behavior at all when downloading content via the helper app 
dialog. Still trying to reproduce...
(was using a debug build from yesterday)

Comment 5

[varada]

WinNT 20001023 Branch
Something similar happens when I try to launch a Windows Media Player as well.
Download Windows Media Player and then launch the link given below in Netscape 6.
http://www.soundclick.com/Playlist/bands/PlayList_265_28.asx
The Saving File Dialog comes up with open using ASFFile.
Point it to the Windows Media Player and Click OK
The "Enter Name of File to Save" native windows dialog comes up.
Cancel this.
"Saving File" dialog comes up with the saving from being the link locations
To is a white strip (blank)
status, Timeleft and Elapsed time are blank and the progress meter is at zero.

If you choose to save the file instead of point it to the Media Player- It
brings up the Saving File dialog with the
From and To locations being filled in with the appropriate values but nothing
happens.

Comment 6

[Scott MacGregor]

Oh I see it isn't the file location dialog which is what I thought everyone was
talking about. it's the standalone progress dialog which isn't going away once
we bring it up. And it seems to be just for this site. Most sites, progress
comes up, we show it and it goes away. Not sure why it isn't going away when
clicking on the TuneIn button.

But you SHOULD be seeing this dialog to show progress for the dialog. So in that
the bug title is invalid. The bug is really that it doesn't go away once we are
done downloading the content. Changing the summary

Comment 7

[Scott MacGregor]

Attachment: proposed fix

Comment 8

[Scott MacGregor]

The fix turned out to be pretty easy. There was a race condition that this site
exposed where we would start to bring up the progress dialog and while we were
doing so, we'd finish downloading the content for the helper app. By the time
the progress dialog came up, we weren't sending any more progress notifications
so the dialog never went away.

The fix is straightforward. When the progress dialog is loaded, it calls back to
the exthandler to register it's implementation of nsIWebProgressListener. We
check here to see if a the on stop request for the content was already issued.
If it was, fake a "on end document load" notification back to the dialog. This
causes the dialog to go away.

As a result of this change, I also needed to initialized a JS variable to 0 for
the time elapsed because in this scenario were I fake a on end notification,  we
were using this variable before it got initialized.

adding seth and alec for a quick r=, sr=.

Comment 9

[(not reading, please use [e-mail] instead)]

cool, looks good.  r=sspitzer.

Comment 10

[Alec Flett]

sr=alecf

Comment 11

[Scott MacGregor]

marking rtm plus now that I have a sr= and a r=. Thanks guys.

Comment 12

[selmer (gone)]

Why is the JS part of this included?  Looks like cleanup that isn't even 
necessary.  We'd like the minimal patch.

Comment 13

[Scott MacGregor]

I put a comment above that the initialization of that variable is NOW required
because I exposed a new code path by explicilitly faking a on stop call to the
JS object. This causes that variable to be used before it would have been
initialized later on. It is NOT cleanup and is a required part of the fix.

Moving back to rtm+

Comment 14

[Jim Roskind]

hmmm... we thought JS would have given this as a default initialization...
hmm... shows how non-JS-savvy I am :-(.
OK.  Critical point is that we want focused minimal fixes.  You insist it is
necessary... and you have the reviews.... and we want the fix (cause it seems
small and valuable...)
RTM double plus.
PLease land on branch asap.

Comment 15

[Alec Flett]

I can vouch for what scott says. The problem is that the variable is being
evaluated in a string context, where it has the value "undefined", then .5 is
appended, and this is is then parsed to get an integer:
so what happens is that JS tries to parse the string "undefined.5" and we get
back NaN

kudos to scott for finding this.

Comment 16

[Scott MacGregor]

Fixed on the branch and the trunk. The progress dialog will now come up and then
properly go away. 

Comment 17

[shrirang khanzode]

verified that the progress box goes away after downloading. Used today's branch 
build on windows 20001025. Adding keyword :vtrunk for verification on trunk

Comment 18

[shrirang khanzode]

verifed this on windows trunk build 10/27. Progress dialog goes away.

Comment 19

[shrirang khanzode]

removing vtrunk.