Closed
Bug 302290
Opened 19 years ago
Closed 19 years ago
WindowStateHolder never Release()s its timeouts
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: bzbarsky, Assigned: bryner)
References
Details
(Keywords: memory-leak)
Attachments
(1 file)
847 bytes,
patch
|
darin.moz
:
review+
jst
:
superreview+
brendan
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.8b4?
Comment 1•19 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
Reporter | ||
Comment 2•19 years ago
|
||
> 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•19 years ago
|
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Comment 3•19 years ago
|
||
Comment 4•19 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•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Assignee | ||
Comment 5•19 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 6•19 years ago
|
||
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 7•19 years ago
|
||
Comment on attachment 191718 [details] [diff] [review] patch sr=jst fwiw.
Attachment #191718 -
Flags: superreview+
Assignee | ||
Comment 8•19 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•