303839 - Leaking webshell/domWindow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mike Pinkerton (not reading bugmail)]

We're leaking a dom window every time you close the browser window. Not sure
when this regressed.

cc'ing jst in case it's more fallout from the split-window bug.

Comment 1

[Simon Fraser [no longer active]]

Firefox is leaking 2 dom windows for each main window that you create and then
close.

Comment 2

[Mike Pinkerton (not reading bugmail)]

well then!

Comment 3

[Mike Schroepfer]

JST - Split window related?

Comment 4

[Johnny Stenback (:jst)]

Hmm. Seems like Mac only, or anyone see this on non-Mac platforms?

Comment 5

[Johnny Stenback (:jst)]

Ok, I actually do see this on win32 and linux as well. This is a leak that crept
in *after* the splitwindow landing, when that change landed, we didn't leak a
single window object after starting firefox and opening another window, and then
closing firefox. 

Comment 6

[Johnny Stenback (:jst)]

I see us leaking one document object, the URL loaded into the leaked document is
"chrome://browser/content/feedview/feedview.xsl". There's no JS reference to
that document, so it seems like a leak through C++ code somewhere... digging
deeper...

Comment 7

[Caleb]

Might be related to bug 303043.

Also see bug 303043 comment 8.

Comment 8

[Simon Fraser [no longer active]]

Would we get this feedview stuff in Camino?

Comment 9

[Johnny Stenback (:jst)]

No idea...

Comment 10

[Johnny Stenback (:jst)]

The feedview leaks have been fixed on the trunk and branches, does this leak
still  exist in Camino?

Comment 11

[Simon Fraser [no longer active]]

I still see one nsGlobalWindow leaked per main window in Camino when loading a
simple page.

After heavy browsing sessions, I've seen 50-60 windows leaked.

Comment 12

[Johnny Stenback (:jst)]

Someone needs to enable GC_MARK_DEBUG to start with to see if we're leaking due
to JS references not being torn down or if it's a C++ leak somewhere. That'd be
where I'd start. Feel free to take this bug, I won't be able to get to a place
where I can debug this any time soon. I can help whoever takes on the task of
finding where this leak is tho...

Comment 13

[Mike Schroepfer]

Simon - is it possible for you to take a swag at this?

Comment 14

[Simon Fraser [no longer active]]

We're leaking webshells, which entrain dom windows.

I think it may be attributable to this change:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/embedding/browser/webBrowser&command=DIFF_FRAMESET&file=nsWebBrowser.cpp&rev2=1.152&rev1=1.151

which bryner made.

Comment 15

[Simon Fraser [no longer active]]

It looks like we have a ref cycle between nsSecureBrowserUIImpl and docShell,
which I don't see being broken anywhere.

Comment 16

[Simon Fraser [no longer active]]

Attachment: Patch to break ref cycle

This patch breaks the ref cycle between the secureBrowserUI/docShell/domWindow.
I'm not sure if this is the best solution.

Comment 17

[Simon Fraser [no longer active]]

Attachment: Better patch

Comment 18

[Johnny Stenback (:jst)]

Seems like this had nothing to do with the split window changes. Updating
dependencies...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193614 [details] [diff] [review]
Better patch

Thanks, Simon!

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 193614 [details] [diff] [review]
Better patch

This needs to be fixed on the branch too.

Comment 21

[Simon Fraser [no longer active]]

Checked into the trunk and branch.