Closed
Bug 417949
Opened 16 years ago
Closed 16 years ago
leak docshell after opening object to be saved in another tab and closing tab quickly
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: memory-leak)
Attachments
(8 files)
2.11 KB,
text/plain; charset=UTF-8
|
Details | |
293.16 KB,
text/plain; charset=UTF-8
|
Details | |
18.82 KB,
text/plain; charset=UTF-8
|
Details | |
11.54 KB,
text/plain; charset=UTF-8
|
Details | |
267.21 KB,
text/plain; charset=UTF-8
|
Details | |
21.30 KB,
text/plain; charset=UTF-8
|
Details | |
290.60 KB,
text/plain; charset=UTF-8
|
Details | |
2.81 KB,
patch
|
Gavin
:
review+
bzbarsky
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Comment 1•16 years ago
|
||
This shows it was leaked from an nsExternalAppHandler.
Assignee | ||
Comment 2•16 years ago
|
||
I think this shows the other reference was from a wrapped native, although I didn't check very carefully.
Assignee | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
Shows all the nsTraceRefcnt-logged objects that were leaked.
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
Assignee | ||
Comment 7•16 years ago
|
||
Assignee | ||
Comment 8•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #303763 -
Attachment mime type: application/octet-stream → text/plain; charset=UTF-8
Assignee | ||
Comment 9•16 years ago
|
||
(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.
Assignee | ||
Comment 10•16 years ago
|
||
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•16 years ago
|
||
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 13•16 years ago
|
||
What we should probably do is try to hook up this stuff to cycle collection... except docshell is involved, which makes life hard.
Assignee | ||
Comment 14•16 years ago
|
||
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.
Assignee | ||
Comment 15•16 years ago
|
||
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.)
Assignee | ||
Updated•16 years ago
|
Attachment #303794 -
Flags: superreview?(bzbarsky)
Attachment #303794 -
Flags: review?(gavin.sharp)
Comment 16•16 years ago
|
||
Comment on attachment 303794 [details] [diff] [review] patch Looks good!
Attachment #303794 -
Flags: superreview?(bzbarsky) → superreview+
Updated•16 years ago
|
Attachment #303794 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 17•16 years ago
|
||
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?
Updated•16 years ago
|
Attachment #303794 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Component: Embedding: Docshell → Download Manager
Product: Core → Firefox
QA Contact: docshell → download.manager
Updated•16 years ago
|
Assignee: nobody → dbaron
Assignee | ||
Comment 18•16 years ago
|
||
Checked in to trunk, 2008-02-20 08:30 -0800.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•