Closed Bug 797065 Opened 8 years ago Closed 8 years ago
Leak if I quit while Scratchpad is open
1. Run a debug build of Firefox with XPCOM_MEM_LEAK_LOG=2 2. Shift+F4 (open Scratchpad) 3. Cmd+Q (quit browser) Result: Leak 9 nsDocument, 6 nsGlobalWindow, etc
I'll do some leak analysis on this...
When I run this locally, I'm seeing 1 chrome nsGlobalWindow and 6 chrome nsDocuments leak. With all-traces CC logging, I can see that nsGlobalWindow is being held alive by live JS. From the GC log, it seems to be rooted like so: via resource:///modules/devtools/scratchpad-manager.jsm : 0x1260a3060 [BackstagePass 126ffefd0] --[ScratchpadManager]-> 0x1260ac6c0 [Object <no private>] --[_scratchpads]-> 0x1260a9e20 [Array <no private>] --[element]-> 0x1260af200 [Proxy <no private>] --[private]-> 0x119af6880 [Object <no private>] [...blah blah JS stuff...] --[parent]-> 0x11c1a1060 [ChromeWindow 1195f2970] via resource:///modules/sessionstore/SessionStore.jsm : 0x125dc2060 [BackstagePass 125f33160] --[ScratchpadManager]-> 0x125de8a80 [Proxy <no private>] --[private]-> 0x1260ac6c0 [Object <no private>] --[_scratchpads]-> 0x1260a9e20 [Array <no private>] --[element]-> 0x1260af200 [Proxy <no private>] --[private]-> 0x119af6880 [Object <no private>] [...blah blah JS stuff...] --[parent]-> 0x11c1a1060 [ChromeWindow 1195f2970] So it looks like there is a field of the ScratchPadManager named _scratchpads that isn't being cleared when it should be? Is that helpful at all?
One solution is to manually clear _scratchpads on shutdown—and since SessionStore needs that property to write state on disk we'll have to run cleanup on xpcom-shutdown or something similar. However, I don't really like this solution since _scratchpads shouldn't have any links to ChromeWindow. If I'm reading your log correctly, there is a property named 'parent' that holds a reference to a ChromeWindow. What I don't understand is how it gets there. :) Since the leak happens even if I populate _scratchpads with empty objects.
Your diagnosis in comment 4 looks right: if you keep an object alive in a compartment, it will keep it's compartment's global alive (via the path you see of shape -> baseshape -> parent (which is the global)). My understanding of the broader bug ends there, of course :)
Yeah, maybe this is something to ask somebody who knows frontend stuff, as I'd imagine this is a fairly common scenario.
So thanks to Andrew, Luke and Philipp (philikon) I managed to fix the problem in a non-lame way. The leak was happening because the object returned by Scratchpad.getState was being created in a compartment and such objects get a special property that holds on to a ChromeWindow instance. Cloning the object on arrival fixed the leak.
Probably doesn't matter much either way, but I vaguely wonder how your clone() implementation compares to using JSON.stringify+parse, for the type of objects you're cloning.
Are you talking about performance? I started by using JSON but then decided against it to minimize changes (i.e. I don't have to change tests to JSON.parse stuff and all that). Wasn't based on anything real, just something I thought looks better.
Performance, and handling of edge cases (like whether any of the properties are non-primitive values, e.g. other objects). But maybe that doesn't apply in this situation?
Yeah, this specific case uses only primitive values (strings and numbers). So we don't need to deep-clone the object.
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
This might be slightly worse than first reported here. It seems like my profile where I was doing this testing (without the fix) always leaks now, with Scratchpad in there somewhere (I haven't dug in great detail yet), even though I never open Scratchpad during the session. Is that expected?
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][MemShrink]
This might be because the browser is trying to restore your Scratchpad windows causing another leak. Could you try with a fix and see if it fixes it? (it should)
There was no scratchpad window that came up which was a little weird. I'll try to remember to see what happens, next week.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][MemShrink] → [MemShrink]
Target Milestone: --- → Firefox 20
I checked just now, and applying this patch makes the shutdown leak go away that I was seeing even without explicitly opening Scratchpad.
You need to log in before you can comment on or make changes to this bug.