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

RESOLVED FIXED in mozilla1.9beta4

Status

()

Toolkit
Downloads API
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
mozilla1.9beta4
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Assignee)

Description

10 years ago
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

10 years ago
Created attachment 303743 [details]
find-comptr-leakers on nsDocShell #5

This shows it was leaked from an nsExternalAppHandler.
(Assignee)

Comment 2

10 years ago
Created attachment 303744 [details]
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.
(Assignee)

Comment 3

10 years ago
Created attachment 303745 [details]
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.
(Assignee)

Comment 4

10 years ago
Created attachment 303747 [details]
XPCOM_MEM_LEAK_LOG

Shows all the nsTraceRefcnt-logged objects that were leaked.
(Assignee)

Comment 5

10 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

10 years ago
Created attachment 303749 [details]
XPC_SHUTDOWN_HEAP_DUMP
(Assignee)

Comment 7

10 years ago
Created attachment 303751 [details]
make-tree --ignore-balance --comptr on nsXPCWrappedJS #263
(Assignee)

Comment 8

10 years ago
Created attachment 303763 [details]
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
(Assignee)

Updated

10 years ago
Attachment #303763 - Attachment mime type: application/octet-stream → text/plain; charset=UTF-8
(Assignee)

Comment 9

10 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.
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: 361909
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

10 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

10 years ago
Created attachment 303794 [details] [diff] [review]
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.)
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 17

10 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

10 years ago
Attachment #303794 - Flags: approval1.9? → approval1.9+
Component: Embedding: Docshell → Download Manager
Product: Core → Firefox
QA Contact: docshell → download.manager
Assignee: nobody → dbaron
(Assignee)

Comment 18

10 years ago
Checked in to trunk, 2008-02-20 08:30 -0800.
Status: NEW → RESOLVED
Last Resolved: 10 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.