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).
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...
Created attachment 191718 [details] [diff] [review] patch
Assignee: general → bryner
Status: NEW → ASSIGNED
Attachment #191718 - Flags: review?(darin)
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+
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+
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.