417949 - leak docshell after opening object to be saved in another tab and closing tab quickly

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I stumbled across a rather timing-sensitive docshell leak that I've been able to reproduce by running the process niced with and starting something else CPU-intensive during the timing-sensitive part.

Steps to reproduce:
 1. NSPR_LOG_FILE=nspr.log NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5,NodeInfoManagerLeak:5 nice -n 15 ./firefox
 2. Tools -> Clear Private Data (to clear cache; not really sure if it matters)
 3. load https://build.mozilla.org/tryserver-builds/2008-02-15_17:[e-mail]-leakmon-0.4.2-try2/
 4. start something CPU-intensive (like md5sum /dev/zero)
 5. middle click on the .dmg file to open it in a new tab
 6. a few seconds after the tab appears (but *before* things have gone on long enough that the Save/Cancel dialog would pop up), close the tab by clicking the [X] on it
 7. stop the CPU-intensive thing
 8. close the browser
 9. run leak-gauge on your NSPR log

Actual results (some of the time):
Leaked docshell at address a262868.
 ... which loaded URI "about:blank".
 ... which loaded URI "https://build.mozilla.org/tryserver-builds/2008-02-15_17:[e-mail]-leakmon-0.4.2-try2/[e-mail]".
Summary:
Leaked 0 out of 11 DOM Windows
Leaked 0 out of 44 documents
Leaked 1 out of 5 docshells
Leaked content nodes within 0 out of 53 documents


Further, nsTraceRefcnt logging shows that nsDocShell serial number 5 is leaked (2 references; 1 from COMPtrs), and nsExternalAppHandler serial number 1 is leaked (1 references; 0 from COMPtrs).  I'll attach a few more details of those leaks.

I may have a chance to debug this further a little later...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: find-comptr-leakers on nsDocShell #5

This shows it was leaked from an nsExternalAppHandler.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: make-tree --ignore-balance --comptr on nsDocShell #5

I think this shows the other reference was from a wrapped native, although I didn't check very carefully.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: make-tree --ignore-balance --comptr on nsExternalAppHandler #1

This shows it was leaked from a wrapped native; I confirmed this one by looking at the version without --ignore-balance, and it looked like everything ignored by --ignore-balance was the ignoring of balanced nsCOMPtrs in the comptr log.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: XPCOM_MEM_LEAK_LOG

Shows all the nsTraceRefcnt-logged objects that were leaked.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh -- and in the cases where it leaks, my network activity monitor shows a lot of activity, suggesting that we're probably downloading the file.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: XPC_SHUTDOWN_HEAP_DUMP

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: make-tree --ignore-balance --comptr on nsXPCWrappedJS #263

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: concatenation of xpclog:5 log and XPC_SHUTDOWN_HEAP_DUMP

I stuck in a JS_GC(ccx) right before the XPC_SHUTDOWN_HEAP_DUMP dump (which is, in turn, just before the xpclog:5 output).

This is the concatenation of both of them for the same run.  Note the following:

b3a9a980 is the mFlatJSObject of a wrapped native wrapping nsIHelperAppLauncher

The following 2 traces are in the XPC_SHUTDOWN_HEAP_DUMP:

0xb3a9a880 Object b3a9a980        via nsXPCWrappedJS[nsIHelperAppLauncherDialog,0x8ba8048].mJSObj

0xb3a9a980 XPCWrappedNative_NoHelper 8bdcf58 via nsXPCWrappedJS[nsIHelperAppLauncherDialog,0x8ba8048].mJSObj(0xb3a9a880 Object).mLauncher

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> 0xb3a9a880 Object b3a9a980        via
> nsXPCWrappedJS[nsIHelperAppLauncherDialog,0x8ba8048].mJSObj

Ignore this one -- it's just confusion due to bug 417972.  The other trace is the important one.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I found that we are NOT hitting this failure case:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp&rev=1.362&mark=1463,1465#1450

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.... show() doesn't actually do anything other than setting up a timer.  If the timer then fires and notify() throws, the helper app dialog never notifies the core code to break the cycle.  Are we getting an exception thrown from the openWindow call in reallyShow (at http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#63)?

Comment 12

[Boris Zbarsky [:bzbarsky]]

If I'm right, this is a regression from bug 361909.

Comment 13

[Boris Zbarsky [:bzbarsky]]

What we should probably do is try to hook up this stuff to cycle collection... except docshell is involved, which makes life hard.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, reallyShow thows:

[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: file:///builds/trunk/obj/firefox-debugopt/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 64"  data: no]

... although it also throws that in some cases where I don't see the leak (because I closed the tab too quickly).


While I'm here, I'll point out that the QueryInterface method should claim to implement nsITimerCallback.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This seems to fix te leak for me.  (I could repro about 50% of the time without this; I think I couldn't about 8 times in a row with it.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Looks good!

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Leak fix to break cycles (and stop the download) if we're unable to bring up the helper app dialog due to the parent window disappearing in the interim.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk, 2008-02-20 08:30 -0800.