302290 - WindowStateHolder never Release()s its timeouts

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I don't see any code in WindowStateHolder that calls Release() on the timeouts
saved in it.  Since the timeout function objects are GC roots and generally
reference the window (via the parent chain, if nothing else), this means that if
the WindowStateHolder goes away without being restored into a window first we
will leak the window.

In other words, if fastback is enabled, leaving a page while a timeout or
interval is pending will effectively leak the nsGlobalWindow for that page.

To test, enable popups and run:

  firefox resource:///res/bloatcycle.html

wait for mozilla.org to start loading in the popup window, and load about:blank
(or other URI of choice) in the bloatcycle.html window.  Then close both
windows.  This leaks an nsGlobalWindow and various things it holds on to, via
the following GC mark path:

0838e908 object 0x8662190 Window via timeout.mFunObj(Function @
0x083bc8a0).__proto__(Function @ 0x083bc310).__proto__(Object @
0x083bc350).__parent__(Window @ 0x0838e908).

Comment 1

[Mike Schroepfer]

BZ,

Can you help us understand the following:

1) How big/severe is the mem leak?
2) Any thoughts on how frequent it will occur? 
3) Opinions on blocker or not for 1.8B4

Thanks!

Mike

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 1) How big/severe is the mem leak?

Not quite sure.  At least several hundred bytes when we leak; with splitwindow
in, possibly more.

> 2) Any thoughts on how frequent it will occur? 

Any time a user leaves a page while a timeout or interval is pending.  So any
time a page with a ticker is left, say.  I think such pages are more common for
the typical user than for my browsing patterns, but I can't put numbers to this
claim.

> 3) Opinions on blocker or not for 1.8B4

Block, imo.  This shouldn't be a complicated fix, and it will make the leak
testing I'm planning to do post-branch much simpler...

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

Comment 4

[Darin Fisher]

Comment on attachment 191718 [details] [diff] [review]
patch

It looks like nsTimeout::Release cleans up mTimer for you, so I think you can
eliminate the code here that mucks with mTimer.

r=darin with that code removed.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 191718 [details] [diff] [review]
patch

This code will probably all go away with mrbkap's patch to save the inner
window object instead of copying these properties and timeouts, but I'd like to
get this in in the meantime.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 191718 [details] [diff] [review]
patch

a=me, so we can have all fixes to the copy approach in before it goes away.

/be

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 191718 [details] [diff] [review]
patch

sr=jst fwiw.

Comment 8

[Brian Ryner (not reading)]

checked in