If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({mlk, qawanted})

Trunk
x86
All
mlk, qawanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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).
(Assignee)

Updated

13 years ago
Keywords: mlk
OS: Linux → All
(Assignee)

Updated

13 years ago
Summary: leaking DOM windows opening and closing new tab → leaking (until window close) DOM windows opening and closing new tab

Comment 1

13 years ago
Just 1 question. Is this bug related to bug 131456? (...which has twice as many
votes as comments)
(Assignee)

Comment 2

13 years ago
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.
(Assignee)

Comment 4

13 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

13 years ago
Created attachment 173239 [details] [diff] [review]
some debugging code, along with -Wshadow fixes
(Assignee)

Comment 6

13 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.)

Comment 7

13 years ago
Didn't you work around the securityUI leak in your checkin for bug 131456?
(Assignee)

Comment 8

12 years ago
This was probably fixed by bug 283129.
Depends on: 283129
Keywords: qawanted
(Assignee)

Comment 9

12 years ago
Fixed, twice, IIRC.

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