WindowStateHolder never Release()s its timeouts

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bz, Assigned: Brian Ryner (not reading))

Tracking

({memory-leak})

Trunk
mozilla1.8beta4
x86
Linux
memory-leak
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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).
Flags: blocking1.8b4?

Comment 1

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

Updated

13 years ago
Target Milestone: --- → mozilla1.8beta4
(Assignee)

Comment 3

13 years ago
Created attachment 191718 [details] [diff] [review]
patch
Assignee: general → bryner
Status: NEW → ASSIGNED
Attachment #191718 - Flags: review?(darin)

Comment 4

13 years ago
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.
Attachment #191718 - Flags: review?(darin) → review+

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+
(Assignee)

Comment 5

13 years ago
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.
Attachment #191718 - Flags: approval1.8b4?
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
Attachment #191718 - Flags: approval1.8b4? → approval1.8b4+
Comment on attachment 191718 [details] [diff] [review]
patch

sr=jst fwiw.
Attachment #191718 - Flags: superreview+
(Assignee)

Comment 8

13 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.