88669 - Race condition(!) in nsIHelperAppLauncher::GetDownloadInfo for mTimeDownloadStarted

Status: RESOLVED WORKSFORME

(Core Graveyard :: Embedding: APIs, defect)

Description

[Philip Langdale]

I'm not 100% that this is exactly an embedding api issue, but I'm embedding. :-)

Galeon overrides nsIHelperAppLauncherDialog and implements a progress dialog
that is pretty much an exact conversion of mozilla's js one to a c++ one.

However, we have options that allow galeon to assume directory and filename for
a download, so it's possible for a user to click on a link and have the file
start downloading and show the progress dialog immediately.

This has revealed a race condition in exthandler where mTimeDownloadStarted 
doesn't get set before the nsIHelperAppLauncherDialog::ShowProgressDialog
callback is called. This problem is not evident in mozilla-the-browser as you
must at least go through a file-save file picker before the progress dialog
appears. In galeon, if our "save-or-view" dialog comes up allowing the download
to begin in the background, mTimeDownloadStarted is set correctly when we get
around to showing the progress dialog. Our file picker runs in the mozilla
thread and blocks ( for various reasons ) so it doesn't allow the download
to start in the background, so it's the same for us as showing the progress
dialog straight away. I've looked at the exthandler code but I haven't seen
what the problem is there.

--phil

Comment 1

[Boris Zbarsky [:bzbarsky]]

ccing mscott.  Status -> NEW

Comment 2

[Adam Lock]

mscott do you want to take this bug?

Comment 3

[Scott MacGregor]

sure

Comment 4

[scottputterman]

Scott, if you think this is important for embedding to have soon, then let's
keep it in 0.9.7 or 0.9.8, if you don't this can go in 0.9.9 or later.

Comment 5

[Scott MacGregor]

moving to moz 1.0

Comment 6

[scottputterman]

Scott, if you think this needs to be fixed, can you put an nsbeta1+ on it?
Otherwise, can you move this out?

Comment 7

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 8

[Philip Langdale]

I think this bug may have bit rotted. :-)

The new progress dialog system has nsIExternalHelperService instantiate an
nsIProgressDialog itself rather than calling
|nsIHelperAppLauncherDialog::ShowProgressDialog|. nsIExternalHelperService then
passes time info to the progress dialog rather than relying on the dialog to
callback for the info.

So I think this bug is now N/A.

Comment 9

[bmcclendon]

This bug is still open, but I'm seeing a different manifestation in M1.2.1 on
WinXP.  I have a helper app set up for (what turns out to be) very small files.
 I don't have "save to disk" set. 

Every time I click on the url, the download progress dialog comes up, but I
immediately get another dialog complaining that it couldn't open the downloaded
file.  I believe the helper app is being execed before the download is complete
(and, even stranger, the download _never_ completes).

To reproduce, install Earthviewer (www.earthviewer.com), see the Windows file
system handler it sets up, and manually (why do I have to do this?) set up the
same thing in Mozilla.

Go to http://bbs.earthviewer.com/ and read some of the messages that have
"placemark" attachments.  The files are very small (usually less than 1K).

Comment 10

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

ok... comment 8 indicates this is not a problem anymore, and code inspection
seems to show that mTimeDownloadStarted is always initialized before the
progress dialog gets shown (via CreateProgressListener)

marking worksforme