341872 - Download manager leaks helperApps

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Régis Caspar]

Download manager leaks helperApps  (from helperApps.js) when downloading a file and choosing "open with".

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060617 Minefield/3.0a1 ID:2006061704 [cairo]

Will attach details and patch

Comment 1

[Régis Caspar]

Attachment: Leak Monitor results

Comment 2

[Régis Caspar]

Attachment: patch

This nullify HelperApps when dialog (unknownContentType.xul) is unloaded.

Comment 3

[Adam Guthrie]

Why is helperApps.js even included here? Do we even use it?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/downloads/content/unknownContentType.xul&rev=1.12&mark=61#58

Comment 4

[Régis Caspar]

(In reply to comment #3)
> Why is helperApps.js even included here? Do we even use it?
That's a pretty good question and honestly I tried to understand this code (open with function of the download manager) but I don't know how this is used. It looks used (how the dialog works is a mystery for me) because unknownContentType.xul actually do something and it's the only script included (I checked with DOMi too).

Comment 5

[Adam Guthrie]

After looking into this, it appears that nsHelperAppDlg.js.in does expect the unknownContentType dialog to include that file. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in&rev=1.35&mark=821-824#817

Regis, you should probably request review from mconnor or Mano on this.

Comment 6

[Régis Caspar]

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

(In reply to comment #5)
> After looking into this, it appears that nsHelperAppDlg.js.in does expect the
> unknownContentType dialog to include that file.
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in&rev=1.35&mark=821-824#817
> 
> Regis, you should probably request review from mconnor or Mano on this.
OK, I verified that the issue is still present, that the patch is still OK and updated my review request. Thanks Adam.

Comment 7

[Adam Guthrie]

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

Comment 8

[Adam Guthrie]

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

Comment 9

[mdew]

would be nice to get this patch into Firefox 2.0 beta2, its still showing the latest branch nightlies

Comment 10

[Adam Guthrie]

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

Comment 11

[Haudy Kazemi]

(In reply to comment #9)
> would be nice to get this patch into Firefox 2.0 beta2, its still showing the
> latest branch nightlies

I agree.  I have a Leak Monitor leak report nearly identical to the submitted 'Leak Monitor results' attachment that I got from 
Mozilla/5.0 (Windows;;; en-US; rv:1.8.1b1) Gecko/20060724 BonEcho/2.0b1 ID:2006072404

Comment 12

[mdew]

such a simple one-liner patch, any reason for the delay?, can it make it into beta 2?

Comment 13

[Adam Guthrie]

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

Comment 14

[Michael Ventnor]

It looks unlikely this patch will make Fx2 unless the drivers feel very generous and mconnor reviews this ASAP.

Comment 15

[Dave Townsend [:mossop]]

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

Comment 16

[towsonu2003]

leaked again, from a site that doesn't require login: 
http://www.cse.yorku.ca/~cs251010/document.pdf

Leaks in window 0x8f818c8:
[+] [leaked object] (96831c8) = [object Object]
 [+] _inner (9682c68) = [xpconnect wrapped (nsISupports, nsIRDFDataSource, nsIRDFRemoteDataSource)]
  [ ] AddObserver (9682c30) = function AddObserver() {
    [native code]
}
  [ ] HasAssertion (9682658) = function HasAssertion() {
    [native code]
}
  [ ] GetTarget (9682610) = function GetTarget() {
    [native code]
}
  [ ] Change (96824d0) = function Change() {
    [native code]
}
  [ ] Assert (9682300) = function Assert() {
    [native code]
}
  [ ] QueryInterface (99d1930) = function QueryInterface() {
    [native code]
}
  [ ] Flush (99d1918) = function Flush() {
    [native code]
}
  [ ] URI = file:///home/mehmet/.mozilla/firefox/7uz3lf08.default/mimeTypes.rdf
  [+] GetSource (8f4f288) = function GetSource() {
    [native code]
}
   [ ] prototype (8f81930) = [object Object]
  [+] GetSources (8f4f270) = function GetSources() {
    [native code]
}
   [ ] prototype (8f81b38) = [object Object]
  [+] GetTargets (8f4f260) = function GetTargets() {
    [native code]
}
   [ ] prototype (8f81be0) = [object Object]
  [+] Unassert (8f4f210) = function Unassert() {
    [native code]
}
   [ ] prototype (8f81c80) = [object Object]
  [+] Move (8f4f1f0) = function Move() {
    [native code]
}
   [ ] prototype (8f81d38) = [object Object]
  [+] RemoveObserver (8f4f1e0) = function RemoveObserver() {
    [native code]
}
   [ ] prototype (8f81e50) = [object Object]
  [+] ArcLabelsIn (8f4f178) = function ArcLabelsIn() {
    [native code]
}
   [ ] prototype (8f81ec0) = [object Object]
  [+] ArcLabelsOut (8f4f168) = function ArcLabelsOut() {
    [native code]
}
   [ ] prototype (8f81f60) = [object Object]
  [+] GetAllResources (8f4f158) = function GetAllResources() {
    [native code]
}
   [ ] prototype (8f82070) = [object Object]
  [+] IsCommandEnabled (8f4f140) = function IsCommandEnabled() {
    [native code]
}
   [ ] prototype (8f68d20) = [object Object]
  [+] DoCommand (8f4f108) = function DoCommand() {
    [native code]
}
   [ ] prototype (8f68d40) = [object Object]
  [+] GetAllCmds (8f4f0f8) = function GetAllCmds() {
    [native code]
}
   [ ] prototype (8f68fc0) = [object Object]
  [+] hasArcIn (8f4f0e0) = function hasArcIn() {
    [native code]
}
   [ ] prototype (8f69018) = [object Object]
  [+] hasArcOut (8f4f0c8) = function hasArcOut() {
    [native code]
}
   [ ] prototype (8f69038) = [object Object]
  [+] beginUpdateBatch (8f4f0b0) = function beginUpdateBatch() {
    [native code]
}
   [ ] prototype (8f690a8) = [object Object]
  [+] endUpdateBatch (8f4f058) = function endUpdateBatch() {
    [native code]
}
   [ ] prototype (8f69af0) = [object Object]
  [ ] loaded = true
  [+] Init (8f4f010) = function Init() {
    [native code]
}
   [ ] prototype (8f69d48) = [object Object]
  [+] Refresh (8f4efd0) = function Refresh() {
    [native code]
}
   [ ] prototype (8f6a788) = [object Object]
  [+] FlushTo (8f4efc0) = function FlushTo() {
    [native code]
}
   [ ] prototype (8f6a8b8) = [object Object]
 [+] _fileTypeArc (9682bb0) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8f4f680) = function QueryInterface() {
    [native code]
}
   [ ] prototype (96208d8) = [object Object]
  [+] EqualsNode (8f4f670) = function EqualsNode() {
    [native code]
}
   [ ] prototype (9620930) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileType
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileType
  [+] Init (8f4f5e8) = function Init() {
    [native code]
}
   [ ] prototype (95ecf80) = [object Object]
  [+] EqualsString (8f4f328) = function EqualsString() {
    [native code]
}
   [ ] prototype (95ed418) = [object Object]
  [+] GetDelegate (8f4f308) = function GetDelegate() {
    [native code]
}
   [ ] prototype (95ed438) = [object Object]
  [+] ReleaseDelegate (8f4f300) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (95ed460) = [object Object]
 [+] _fileHandlerArc (9682b80) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8f3c650) = function QueryInterface() {
    [native code]
}
   [ ] prototype (9620628) = [object Object]
  [+] EqualsNode (8f3c638) = function EqualsNode() {
    [native code]
}
   [ ] prototype (9620650) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileHandler
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileHandler
  [+] Init (8f4fb18) = function Init() {
    [native code]
}
   [ ] prototype (96206a8) = [object Object]
  [+] EqualsString (8f4fb08) = function EqualsString() {
    [native code]
}
   [ ] prototype (96206c0) = [object Object]
  [+] GetDelegate (8f4fad8) = function GetDelegate() {
    [native code]
}
   [ ] prototype (9620750) = [object Object]
  [+] ReleaseDelegate (8f4fad0) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (9620788) = [object Object]
 [+] _fileIconArc (9682b38) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8d21ff0) = function QueryInterface() {
    [native code]
}
   [ ] prototype (96204c8) = [object Object]
  [+] EqualsNode (8d21fa8) = function EqualsNode() {
    [native code]
}
   [ ] prototype (96204d8) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileIcon
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileIcon
  [+] Init (8d21f90) = function Init() {
    [native code]
}
   [ ] prototype (9620508) = [object Object]
  [+] EqualsString (8d21f80) = function EqualsString() {
    [native code]
}
   [ ] prototype (9620538) = [object Object]
  [+] GetDelegate (8f3cce8) = function GetDelegate() {
    [native code]
}
   [ ] prototype (9620568) = [object Object]
  [+] ReleaseDelegate (8f3c8a0) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (9620598) = [object Object]
 [+] _fileExtensionArc (9682b10) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8d22120) = function QueryInterface() {
    [native code]
}
   [ ] prototype (96200c8) = [object Object]
  [+] EqualsNode (8d22110) = function EqualsNode() {
    [native code]
}
   [ ] prototype (96203f0) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileExtension
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileExtension
  [+] Init (8d220f0) = function Init() {
    [native code]
}
   [ ] prototype (9620438) = [object Object]
  [+] EqualsString (8d220c0) = function EqualsString() {
    [native code]
}
   [ ] prototype (9620460) = [object Object]
  [+] GetDelegate (8d220b8) = function GetDelegate() {
    [native code]
}
   [ ] prototype (9620478) = [object Object]
  [+] ReleaseDelegate (8d220a8) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (9620488) = [object Object]
 [+] _fileExtensionsArc (9682ad0) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8d22240) = function QueryInterface() {
    [native code]
}
   [ ] prototype (961f810) = [object Object]
  [+] EqualsNode (8d22238) = function EqualsNode() {
    [native code]
}
   [ ] prototype (961f9a0) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileExtensions
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileExtensions
  [+] Init (8d221d0) = function Init() {
    [native code]
}
   [ ] prototype (961fab0) = [object Object]
  [+] EqualsString (8d221c8) = function EqualsString() {
    [native code]
}
   [ ] prototype (961fac0) = [object Object]
  [+] GetDelegate (8d221b0) = function GetDelegate() {
    [native code]
}
   [ ] prototype (961fb28) = [object Object]
  [+] ReleaseDelegate (8d22158) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (961fb38) = [object Object]
 [+] _handleAutoArc (9682ab0) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8d22338) = function QueryInterface() {
    [native code]
}
   [ ] prototype (961f688) = [object Object]
  [+] EqualsNode (8d222e8) = function EqualsNode() {
    [native code]
}
   [ ] prototype (961f6c8) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#FileHandleAuto
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#FileHandleAuto
  [+] Init (8d222b8) = function Init() {
    [native code]
}
   [ ] prototype (961f700) = [object Object]
  [+] EqualsString (8d222b0) = function EqualsString() {
    [native code]
}
   [ ] prototype (961f750) = [object Object]
  [+] GetDelegate (8d222a8) = function GetDelegate() {
    [native code]
}
   [ ] prototype (961f790) = [object Object]
  [+] ReleaseDelegate (8d22298) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (961f7b0) = [object Object]
 [+] _valueArc (96829d0) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8cf6578) = function QueryInterface() {
    [native code]
}
   [ ] prototype (961f560) = [object Object]
  [+] EqualsNode (8cf6140) = function EqualsNode() {
    [native code]
}
   [ ] prototype (961f598) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#value
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#value
  [+] Init (8cf5d10) = function Init() {
    [native code]
}
   [ ] prototype (961f5b8) = [object Object]
  [+] EqualsString (8d223f8) = function EqualsString() {
    [native code]
}
   [ ] prototype (961f5c8) = [object Object]
  [+] GetDelegate (8d223f0) = function GetDelegate() {
    [native code]
}
   [ ] prototype (961f5f0) = [object Object]
  [+] ReleaseDelegate (8d223b0) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (961f600) = [object Object]
 [+] _handlerPropArc (96827b8) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8cc3be0) = function QueryInterface() {
    [native code]
}
   [ ] prototype (961f470) = [object Object]
  [+] EqualsNode (8cc3bd0) = function EqualsNode() {
    [native code]
}
   [ ] prototype (961f480) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#handlerProp
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#handlerProp
  [+] Init (8cf66a0) = function Init() {
    [native code]
}
   [ ] prototype (961f4a0) = [object Object]
  [+] EqualsString (8cf6690) = function EqualsString() {
    [native code]
}
   [ ] prototype (961f4b0) = [object Object]
  [+] GetDelegate (8cf6688) = function GetDelegate() {
    [native code]
}
   [ ] prototype (961f4c0) = [object Object]
  [+] ReleaseDelegate (8cf6678) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (961f4d0) = [object Object]
 [+] _externalAppArc (96827a8) = [xpconnect wrapped nsIRDFResource]
  [+] QueryInterface (8b5dd20) = function QueryInterface() {
    [native code]
}
   [ ] prototype (961f360) = [object Object]
  [+] EqualsNode (8b5dd00) = function EqualsNode() {
    [native code]
}
   [ ] prototype (961f390) = [object Object]
  [ ] Value = http://home.netscape.com/NC-rdf#externalApplication
  [ ] ValueUTF8 = http://home.netscape.com/NC-rdf#externalApplication
  [+] Init (8c3aec0) = function Init() {
    [native code]
}
   [ ] prototype (961f3b8) = [object Object]
  [+] EqualsString (8c3ae90) = function EqualsString() {
    [native code]
}
   [ ] prototype (961f3c8) = [object Object]
  [+] GetDelegate (8c3ae50) = function GetDelegate() {
    [native code]
}
   [ ] prototype (961f3f0) = [object Object]
  [+] ReleaseDelegate (8c3ae08) = function ReleaseDelegate() {
    [native code]
}
   [ ] prototype (961f428) = [object Object]

Pls let me know if this is a new bug that I should submit.

Comment 17

[Régis Caspar]

(In reply to comment #16)
> leaked again, from a site that doesn't require login: 
> http://www.cse.yorku.ca/~cs251010/document.pdf
> [...]
> Pls let me know if this is a new bug that I should submit.
I would say the bug you observe is this one..

Comment 18

[towsonu2003]

(In reply to comment #17)
> (In reply to comment #16)
> > leaked again, from a site that doesn't require login: 
> > http://www.cse.yorku.ca/~cs251010/document.pdf
> > [...]
> > Pls let me know if this is a new bug that I should submit.
> I would say the bug you observe is this one..
> 

Leaked again with another link (was downloading very small tar.gz package from http://www.whatwouldjesusdownload.com/christianubuntu/2006/11/ubuntu-ce-v20-edgy-downloads.html (link under "Web Content Filtering Only")). 

How can I make sure this is the same thing? -pls don't say "patch the source" eheh- Is there a place I should look at the Memory Leak Monitor output to identify this? thanks

Comment 19

[Régis Caspar]

(In reply to comment #18)
> (In reply to comment #17)
> How can I make sure this is the same thing? -pls don't say "patch the source"
> eheh- Is there a place I should look at the Memory Leak Monitor output to
> identify this? thanks

"patch the source" ;)
No, I just looked at your Leak Monitor output and compared to mine (see attachment 1 [details] [diff] [review]), function name, etc were similar. 

Comment 20

[towsonu2003]

keep getting this memory leak with many downloads... when should I expect this to be fixed with a 2.0.1 or similar release? thanks

Comment 21

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/mozapps/downloads/content/unknownContentType.xul 	1.19

Comment 22

[Régis Caspar]

Looks VERIFIED to me (tested with comment #18 link and another link to a zip file ; Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061208 Minefield/3.0a1 ID:2006120804 [cairo])

Comment 23

[Daniel Veditz [:dveditz]]

Moving blocking request, 1.8.1.1 is done.

Comment 24

[Jay Patel [:jay]]

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

Approved for 1.8 branch, a=jay for drivers.

Comment 25

[Régis Caspar]

Attachment: patch v2

Comment 26

[Régis Caspar]

I made an error: the leak is due to HelperApps, not because of HelperApps itself, but because of a never removed observer. 
So, attached is patch v2, which back out v1 changes and add a "clear()" method to HelperApps. This method is then called when HelperApps is no more needed (in nsHelperAppsDlD.js).

I would really appreciate some additional testing on the v2 patch

 -> Reopening

Comment 27

[Régis Caspar]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Patch v2 fixes the leak for me

Comment 29

[Régis Caspar]

Thanks for the testing Ryan.

Comment 30

[Jay Patel [:jay]]

Gavin:  Could you get this patch reviewed so we can land it soon?  Thanks!

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 249680 [details] [diff] [review]
patch v2

>Index: toolkit/mozapps/downloads/content/helperApps.js

> HelperApps.prototype = {
>+  clear: function ()
>+  {
>+    if (this._inner)  {
>+      this._inner.RemoveObserver(this);
>+    }
>+  },

Can't you use the existing nearly-identical destroy()? I'm not sure why the null check is needed (none of the other methods seem to null check _inner, and _inner is never nulled out after being assigned to and then used without a null check in the constructor).

Comment 32

[:Gavin Sharp [email: [e-mail]]]

I filed bug 366387 for other issues with this code.

Comment 33

[Régis Caspar]

(In reply to comment #31)
> Can't you use the existing nearly-identical destroy()? 
hum? no destroy in http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
> I'm not sure why the null check is needed (none of the other methods seem to null check _inner, and
> _inner is never nulled out after being assigned to and then used without a null
> check in the constructor).
null check? like "if (dialog) dialog.onCancel();"? If yes, that's existing code (see bug 294827).

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #33)
> (In reply to comment #31)
> > Can't you use the existing nearly-identical destroy()? 
> hum? no destroy in
> http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

There is in http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/helperApps.js#424, though, which is where you added the function I quoted.

> > I'm not sure why the null check is needed (none of the other methods seem to null check _inner, and
> > _inner is never nulled out after being assigned to and then used without a null
> > check in the constructor).
> null check? like "if (dialog) dialog.onCancel();"? If yes, that's existing code
> (see bug 294827).

No, I mean the |if  (this._inner)| null check in the clear() function you added. destroy() only differs from your clear in that it doesn't null check.

Comment 35

[Régis Caspar]

Attachment: patch v3

(In reply to comment #34)
> There is in
> http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/helperApps.js#424,
> though, which is where you added the function I quoted.
> No, I mean the |if  (this._inner)| null check in the clear() function you
> added. destroy() only differs from your clear in that it doesn't null check.
OK, I didn't see this function. I just tested using destroy() and it looks good.
Here's related patch (using destroy)

Comment 36

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/mozapps/downloads/content/unknownContentType.xul 	1.20
mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in 	1.44

Comment 37

[Régis Caspar]

Thanks for the review and cvs co Gavin

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 250932 [details] [diff] [review]
patch v3

Approved for 1.8 branch, a=dveditz for drivers

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Attachment: branch patch

mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in 	1.32.2.6

Comment 40

[Carsten Book [:Tomcat]]

verified 1.8.1.2 - tested with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070126 BonEcho/2.0.0.2pre ID:2007012603 - no mlk alert.