797065 - Leak if I quit while Scratchpad is open

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Jesse Ruderman]

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]]

I'll do some leak analysis on this...

Comment 2

[Andrew McCreight [:mccr8]]

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)]

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]]

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]]

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]]

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)]

Attachment: 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: [e-mail]]]

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)]

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: [e-mail]]]

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)]

Yeah, this specific case uses only primitive values (strings and numbers). So we don't need to deep-clone the object.

Comment 12

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/77d729ffd925

Comment 13

[Andrew McCreight [:mccr8]]

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)]

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]]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/77d729ffd925

Comment 17

[Andrew McCreight [:mccr8]]

I checked just now, and applying this patch makes the shutdown leak go away that I was seeing even without explicitly opening Scratchpad.