Closed Bug 280818 Opened 19 years ago Closed 19 years ago

leaking (until window close) DOM windows opening and closing new tab

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: memory-leak, qawanted)

Attachments

(1 file)

In the suite, we leak DOM windows when opening and closing new tabs, but they go
away eventually.  They're not entrained in the GC, but they are destroyed during
GC, with the stack:


nsGlobalWindow::Release()
(/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:337)
nsCOMPtr<nsIDOMWindow>::~nsCOMPtr() (../../../../dist/include/xpcom/nsCOMPtr.h:583)
nsSecureBrowserUIImpl::~nsSecureBrowserUIImpl()
(/builds/trunk/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:163)
nsSecureBrowserUIImpl::Release()
(/builds/trunk/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:90)
XPCJSRuntime::GCCallback(JSContext*, JSGCStatus)
(/builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:556)
...

These nsSecureBrowserUI objects seem to be per-tab, and have an
nsCOMPtr<nsIDOMWindow> member.  They seem to be entrained in the GC by the path:

0a49ac80 object 0xa4b47f8 XPCWrappedNative_NoHelper via
XPCWrappedNative::mFlatJSObject(XULElement).securityUI(XPCWrappedNative_NoHelper).
Summary: leaking DOM windows opening and closing new tab → leaking (until window close) DOM windows opening and closing new tab
Just 1 question. Is this bug related to bug 131456? (...which has twice as many
votes as comments)
I think this is a recent regression, so no.
Hmm... So the Firefox browser.xml code has this destroy method with comment: 
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/browser.xml#565

Suite just nulls out securityUI in the destructor.

Are we failing to call the browser binding destructor when we close a tab?

This is the only place I see a securityUI member on a binding, so this has got
to be where the problem is.
So, the reason that stuff doesn't go away is the following, I think:

Any code that sets properties on a DOM node that aren't IDL setters (i.e.,
things that go through normal JS property setting, which includes XBL fields)
causes nsDocument::AddReference to be called.  This AddRefs the element's
wrapper, which causes it to become a root (which XPCWrappedNative objects do
when they have a reference count of two or more).

The stuff in the toolkit XBL code is probably a good workaround for 1.8

However, I think the basic problem here is something we should fix.  Content
removed from a document should be GCed if it is no longer reachable.  I think a
good solution would involve changing XPConnect to support special Mark ops
(which I believe the JS engine supports) and then making content nodes use them
to ensure that (1) if any node in a document tree is marked, all nodes reachable
from that node via .parentNode, .childNodes, .nextSibling, etc., are marked and
(2) that we always mark the stuff reachable from the document itself.  In other
words, we'd maintain a hashtable like AddReference does, except we wouldn't
AddRef the wrappers.  Instead, we'd maintain one such hashtable for each tree of
nodes.  (To deal with nodes being removed and added, we'd need to set bits all
the way up the tree for every node that was in such a hashtable.)  Then the
custom mark op would find and enumerate the correct hashtable (in a way that
avoids reentry as cheaply as possible).  This code could also keep the root of
the tree alive, which is probably a good thing.  We don't want people to be
caught by GC-timing-dependent bugs if they hold a subtree disconnected from the
document by something other than the root node.

I think I've talked about something like this in another bug, although I don't
think I was aware of AddReference when I wrote it.
I think this has been fixed, although I need to check.

(But there was a separate regression from what fixed it, or what would have
fixed it if it hadn't already been fixed.)
Didn't you work around the securityUI leak in your checkin for bug 131456?
This was probably fixed by bug 283129.
Depends on: 283129
Fixed, twice, IIRC.

Once bug 320192 lands it will be easier to verify.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: