Closed Bug 797065 Opened 12 years ago Closed 11 years ago

Leak if I quit while Scratchpad is open

Categories

(DevTools Graveyard :: Scratchpad, defect, P2)

x86_64
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: jruderman, Assigned: anton)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, Whiteboard: [MemShrink])

Attachments

(1 file)

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
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee: nobody → anton
I'll do some leak analysis on this...
Flags: needinfo?(continuation)
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[1]]-> 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[1]]-> 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?
Flags: needinfo?(continuation)
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.
The part of the path I deleted looks like this:

    --[element[1]]-> 0x1260af200 [Proxy <no private>]
    --[private]-> 0x119af6880 [Object <no private>]
    --[type]-> 0x119d28070 [type_object]
    --[type_proto]-> 0x11d7fb020 [Object <no private>]
    --[shape]-> 0x119a9d628 [shape]
    --[parent]-> 0x11bf8c6f0 [shape]
    --[parent]-> 0x11bf8c718 [shape]
    --[parent]-> 0x11bf8c740 [shape]
    --[parent]-> 0x11bf8c768 [shape]
    --[parent]-> 0x11bf8c790 [shape]
    --[parent]-> 0x11bf8c7b8 [shape]
    --[base]-> 0x11bf8d190 [base_shape]
    --[parent]-> 0x11c1a1060 [ChromeWindow 1195f2970]

There's an element of _scratchpads that is a proxy (cross compartment wrapper maybe?) for an object 0x119af6880. All of the type, type_proto, shape and parent things are internal JS stuff. I think the object 0x119af6880 is maybe created in the compartment of the ChromeWindow, so it keeps the ChromeWindow alive or something?

That object is like this:

0x119af6880 B Object <no private>
> 0x119d28070 B type
> 0x11bf8c100 B shape
> 0x126083020 B text

The field "text" is the string "/*\n * This is a JavaScript Scratchpad.\n *\n * Enter some JavaScript, then Right Click or choose from the Execute Menu:\n * 1. Run to evaluate the selected text (Cmd-R),\n * 2. Inspect to bring up an Object Inspector on the result (Cmd-I), or,\n * 3. Display to insert the result in a comment after the selection. (Cmd-L)\n */\n\n".

Maybe Luke has some suggestions?
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.
Attachment #694047 - Flags: review?(fayearthur)
Attachment #694047 - Flags: review?(dcamp)
Attachment #694047 - Flags: review?(dcamp) → review+
Whiteboard: [land-in-fx-team]
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.
https://hg.mozilla.org/integration/fx-team/rev/77d729ffd925
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.
https://hg.mozilla.org/mozilla-central/rev/77d729ffd925
Status: ASSIGNED → RESOLVED
Closed: 11 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.
Blocks: fuzz-keys
Attachment #694047 - Flags: review?(fayearthur)
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: