158066 - Potential leak of window object because of self-cycle with opener

Status: RESOLVED FIXED

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

Description

[Harshal Pradhan]

Here is another one of my "strange case" leak reports.

The testcase that I am about to to attach will cause mozilla to leak a
GlobalWindow object.

What is happening here is that the GlobalWindow holds on to a strong ref to its
opener   

nsCOMPtr<nsIDOMWindowInternal> mOpener;

But the opener can potentially be itself. So it end up having a ref to itself
and leaking. Everything that it owns goes with it.

I dunno how you are going to fix this, or if it is even worth fixing  :-/

Comment 1

[Harshal Pradhan]

err .. sorry ... typo in summary.

Comment 2

[Harshal Pradhan]

Attachment: ugly test case that should suffice to explain what I am talking about

Comment 3

[Boris Zbarsky [:bzbarsky]]

nice....

Would it be possible to use something similar to create a loop with more than
one window in it?  Or do we just need to deal with the foo.opener == foo case?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Just did some testing with that testcase.  It doesn't leak any webshells, which
makes it leaking a window seem unlikely....

Comment 5

[Harshal Pradhan]

Yeah. Its not as bad as I initially thought.

We end up realeasing most (probably all) of the stuff that the window holds
since SetDocShell(null) does get called. But we do leak the GlobalWindow object
itself.

Comment 6

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 7

[Jesse Ruderman]

I see a nsGlobalWindow leak with the testcase, using Mac trunk debug.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

The comment that was there is:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&rev=1.899&mark=679-684,735,736#672
Need to test if there still are some problems...

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 292303 [details] [diff] [review]
possible patch

I run mochitest with many windows open and there weren't any problems.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 292303 [details] [diff] [review]
possible patch

Does it fix this bug?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

sure :)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Checked in