Closed Bug 417949 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Downloads API, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: memory-leak)

Attachments

(8 files)

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:26-dbaron@mozilla.com-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:26-dbaron@mozilla.com-leakmon-0.4.2-try2/dbaron@mozilla.com-leakmon-0.4.2-try2-firefox-try-mac.dmg".
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...
This shows it was leaked from an nsExternalAppHandler.
I think this shows the other reference was from a wrapped native, although I didn't check very carefully.
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.
Attached file XPCOM_MEM_LEAK_LOG
Shows all the nsTraceRefcnt-logged objects that were leaked.
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.
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
Attachment #303763 - Attachment mime type: application/octet-stream → text/plain; charset=UTF-8
(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.
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)?
If I'm right, this is a regression from bug 361909.
Blocks: flock8499
What we should probably do is try to hook up this stuff to cycle collection... except docshell is involved, which makes life hard.
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.
Attached patch patchSplinter Review
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.)
Attachment #303794 - Flags: superreview?(bzbarsky)
Attachment #303794 - Flags: review?(gavin.sharp)
Comment on attachment 303794 [details] [diff] [review]
patch

Looks good!
Attachment #303794 - Flags: superreview?(bzbarsky) → superreview+
Attachment #303794 - Flags: review?(gavin.sharp) → review+
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.
Attachment #303794 - Attachment description: possible patch → patch
Attachment #303794 - Flags: approval1.9?
Attachment #303794 - Flags: approval1.9? → approval1.9+
Component: Embedding: Docshell → Download Manager
Product: Core → Firefox
QA Contact: docshell → download.manager
Assignee: nobody → dbaron
Checked in to trunk, 2008-02-20 08:30 -0800.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.