Last Comment Bug 797065 - Leak if I quit while Scratchpad is open
: Leak if I quit while Scratchpad is open
Status: RESOLVED FIXED
[MemShrink]
: mlk
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: Trunk
: x86_64 Mac OS X
: P2 normal (vote)
: Firefox 20
Assigned To: Anton Kovalyov (:anton)
:
: Julian Descottes [:jdescottes]
Mentors:
Depends on:
Blocks: fuzz-keys
  Show dependency treegraph
 
Reported: 2012-10-02 12:02 PDT by Jesse Ruderman
Modified: 2014-01-26 11:28 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clone the object returned from Scratchpad.getState (1.84 KB, patch)
2012-12-19 13:39 PST, Anton Kovalyov (:anton)
dcamp: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-10-02 12:02:12 PDT
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
Comment 1 Andrew McCreight [:mccr8] 2012-12-18 16:37:33 PST
I'll do some leak analysis on this...
Comment 2 Andrew McCreight [:mccr8] 2012-12-19 12:00:49 PST
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?
Comment 3 Anton Kovalyov (:anton) 2012-12-19 12:06:28 PST
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.
Comment 4 Andrew McCreight [:mccr8] 2012-12-19 12:12:56 PST
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?
Comment 5 Luke Wagner [:luke] 2012-12-19 12:23:00 PST
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 :)
Comment 6 Andrew McCreight [:mccr8] 2012-12-19 12:55:35 PST
Yeah, maybe this is something to ask somebody who knows frontend stuff, as I'd imagine this is a fairly common scenario.
Comment 7 Anton Kovalyov (:anton) 2012-12-19 13:39:47 PST
Created attachment 694047 [details] [diff] [review]
Clone the object returned from Scratchpad.getState

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.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-19 13:47:19 PST
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.
Comment 9 Anton Kovalyov (:anton) 2012-12-19 13:50:59 PST
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.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-19 15:20:44 PST
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?
Comment 11 Anton Kovalyov (:anton) 2012-12-19 15:24:51 PST
Yeah, this specific case uses only primitive values (strings and numbers). So we don't need to deep-clone the object.
Comment 13 Andrew McCreight [:mccr8] 2012-12-20 07:58:21 PST
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?
Comment 14 Anton Kovalyov (:anton) 2012-12-20 13:39:04 PST
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)
Comment 15 Andrew McCreight [:mccr8] 2012-12-20 19:39:24 PST
There was no scratchpad window that came up which was a little weird.

I'll try to remember to see what happens, next week.
Comment 16 Panos Astithas [:past] 2012-12-21 00:37:05 PST
https://hg.mozilla.org/mozilla-central/rev/77d729ffd925
Comment 17 Andrew McCreight [:mccr8] 2012-12-27 06:42:46 PST
I checked just now, and applying this patch makes the shutdown leak go away that I was seeing even without explicitly opening Scratchpad.

Note You need to log in before you can comment on or make changes to this bug.