Closed
Bug 280818
Opened 20 years ago
Closed 19 years ago
leaking (until window close) DOM windows opening and closing new tab
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: memory-leak, qawanted)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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).
OS: Linux → All
Assignee | ||
Updated•20 years ago
|
Summary: leaking DOM windows opening and closing new tab → leaking (until window close) DOM windows opening and closing new tab
Comment 1•20 years ago
|
||
Just 1 question. Is this bug related to bug 131456? (...which has twice as many votes as comments)
Comment 3•20 years ago
|
||
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.
Assignee | ||
Comment 4•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
Assignee | ||
Comment 6•20 years ago
|
||
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.)
Keywords: qawanted
Assignee | ||
Comment 9•19 years ago
|
||
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.
Description
•