152224 - Leak in nsExternalAppHandler

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P3)

Description

[Mark Liffiton]

In nsExternalHelperAppService.cpp:

mWebProgressListener is set in SetWebProgressListener() when a new
nsIDownload/nsIWebProgressListener is created, but it is only released in
CloseProgressWindow(), which doesn't seem to be called anywhere.

In my specific case (in a Gecko-embedding environment), this means that download
progress dialogs (which implement nsIDownload and nsIWebProgressListener) are
never released, causing a significant memory leak.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Well... mWebProgressListener is a member variable on a nsExternalAppHandler
object. When said object gets destroyed, the progresslistener is automatically
released.  Is this object not getting destroyed?

CloseProgressWindow() is called by the OnUnload() method in
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/progressDlg/nsProgressDlg.js;
it does not seem to be called by the download manager anywhere, however...

Comment 2

[Mark Liffiton]

FYI, I orginally noticed the problem because a breakpoint in my progress
dialog's destructor never triggered.  It just sits around w/ mRefCnt equal to 1.
 I assume this is due to the mWebProgressListener reference, though I may be wrong.

Furthermore, I don't know that I have access to an instance of
nsIHelperAppLauncher to call CloseProgressListener() like in nsProgressDlg.js. 
We used to get it as a parameter to
nsHelperAppLauncherDialog::ShowProgressDialog(), but that method is apparently
obsolete and nothing has replaced it(?).

(Side note:  Is there any documentation for the download API anywhere?  I can
only glean so much by reading the source...)

Comment 3

[Mark Liffiton]

FYI #2:  I've worked around this, though not in a particularly elegant way.  My
class is passed an instance of nsExternalHelperAppService as an nsIObserver in
SetObserver().  I queryinterface this nsIObserver for an nsIHelperAppLauncher
when I need to call CloseProgressWindow() on it.

It would be nice if there were a more proper way to deal with this...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mark, are you ever seeing the nsExternalAppHandler called in your app?  Both
with and without the workaround you mention in comment 3?

Sorry it's taken me so long to get back to you....

I'm starting to suspect that we may have a reference cycle between the
nsExternalAppHandler and its mWebProgressListener...  that would be bad. ;)

Comment 5

[amutch]

Mark's going to be unavailable for 3 months. I'll see if one of our other 
coders can answer this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

confirming for now and assigning to myself to keep this on my radar.  I will
likely be working on cleaning up these apis some and welcome any feedback or
ideas (in personal email, not in this bug).

Comment 7

[Boris Zbarsky [:bzbarsky]]

[e-mail]?  Mark?  How do things stand here?

Comment 8

[amutch]

Let me check on this and see where we are at.

Comment 9

[Boris Zbarsky [:bzbarsky]]

[e-mail]?  Mark?  Any luck?

Marking Future for now due to lack of response....

Comment 10

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

taking. this is a reference cycle:
nsExternalAppHandler has a reference to its listener (must have). as comment 0
mentioned, this is nowhere released (CloseProgressWindow has no caller, not in
seamonkey at least).

listener has a reference to the nsExternalAppHandler, as the nsIObserver to
cancel the download. also nowhere released - again, at least not in seamonkey.

Comment 11

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

hm. actually, it looks like there are still 3 references after that... one will
be uriloader, still have to find out what the other two are.

Comment 12

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

second problem: cycle with mDialog

Comment 13

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

Attachment: patch

Comment 14

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

Comment on attachment 137492 [details] [diff] [review]
patch

so, this does two other things as well:
o Do not use prettyprinting of boolean values, per darin's special request
o send an OnStateChange with STATE_START as well, not only STATE_STOP (required
for download resuming. I can put that into another bug's patch if you prefer)

Comment 15

[Alec Flett]

Comment on attachment 137492 [details] [diff] [review]
patch

in just perusing the patch, I like what I see, but do you want to document
mRequest a little more - i.e. why your'e holding a strong ref to it, and so
forth?

Comment 16

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

Comment on attachment 137492 [details] [diff] [review]
patch

hm, good point. a weak ref should suffice, given that I don't need it anymore
after OnStopRequest.

Comment 17

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

Attachment: patch v2 part 1

make mRequest a weak reference, clear the reference to mDialog in
CreateProgressListener (because we might need it after OnStopRequest, in
PromptForSaveToFile), also clear it in Cancel.

now... this causes download manager to crash, because it doesn't keep a
reference to the nsDownload objects it puts into its hashtable
(mCurrDownloads). I'll attach a patch for that.

Comment 18

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

Attachment: patch v2 part 2 (checked in)

make download manager keep references to the things in its hashtable

Comment 19

[Jan Varga [:janv]]

Comment on attachment 137580 [details] [diff] [review]
patch v2 part 2 (checked in)

looks good
r=varga

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137579 [details] [diff] [review]
patch v2 part 1

I won't be able to review this until probably Tuesday.

Comment 21

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

that's ok, this isn't urgent

Comment 22

[Darin Fisher]

Comment on attachment 137580 [details] [diff] [review]
patch v2 part 2 (checked in)

isn't there a hash table that you can use that'll do the addref & release
automagically?

sr=darin

Comment 23

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

darin: there is, but it's a bit hard to use, at least until bug 228794 is fixed.
I filed Bug 228825

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137579 [details] [diff] [review]
patch v2 part 1

Add some comments at the spots where the reference cycles are set up and torn
down clearly saying that that's what's going on (and in particular make it very
very clear that the progress listener unhooking stuff is absolutely required to
not leak).

r=bzbarsky, assuming you've tested both download manager and progress dialog,
and tested both codepaths (select action before/after download finishes).

Comment 25

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

Attachment: part 1 v3

made the changes bz requested.

all 4 codepaths tested successfully.

Comment 26

[Darin Fisher]

Comment on attachment 138077 [details] [diff] [review]
part 1 v3

sr=darin

Comment 27

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

thanks, checked in.