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).
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?
Fixed, twice, IIRC. Once bug 320192 lands it will be easier to verify.