1471989 - Ghost windows on Facebook from JS stack from held alive by ConsoleAPIStorage.js

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Andrew McCreight [:mccr8]]

I've been seeing ghost windows on Facebook for maybe the last week. I'm inclined to blame BinAST but I have no evidence for that yet. :)

Comment 1

[Andrew McCreight [:mccr8]]

The ghost window went away this time when I clicked on minimize memory usage. I'll have to grab a log before I do that next time I see this.

Comment 2

[Eric Rahm [:erahm]]

Andrew can you follow up on this with your existing session.

Comment 3

[Andrew McCreight [:mccr8]]

I did see this when we looked at this bug, but I haven't seen it since then. and I guess I forgot to get a log. I'll keep an eye out for it.

Comment 4

[Andrew McCreight [:mccr8]]

I'm not seeing this too frequently, but I do see it occasionally. I got a decent log recently. In this log, two Facebook inner windows are leaking. The path keeping them alive in the CC log looks like this:

0x122f06430 [JS Object (XPCWrappedNative_NoHelper)]
    --[js::GetObjectPrivate(obj)]--> 0x1192b0100 [XPCWrappedNative]
    --[mIdentity]--> 0x1099d4000 [JSStackFrame]
    --[mStack]--> 0x152805ee0 [JS Object (SavedFrame)]
    --[group_global]--> 0x155df9100 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x150a51c00 [nsGlobalWindowInner # 4294968146 inner https://www.facebook.com/]

    Root 0x122f06430 is a marked GC object.

This looks like the JS representation of a stack.

when I switch to the GC log, this is how the XPCWN is staying alive:

0x1214182e0 [NonSyntacticVariablesObject <no private>]
    --[_consoleStorage]--> 0x123dae940 [Map 0x1219bb380]
    --[value]--> 0x1214c3100 [Array <no private>]
    --[objectElements[0]]--> 0x12231ddc0 [Object <no private>]
    --[shape]--> 0x184293d98 [shape]
    --[getter]--> 0x155d1edd0 [Function stacktrace]
    --[nativeReserved[0]]--> 0x122f06430 [XPCWrappedNative_NoHelper 0x1192b0100]

It looks like _consoleStorage is a global data structure defined in ConsoleAPIStorage.js.

Comment 5

[Andrew McCreight [:mccr8]]

From that file:
 * ConsoleAPI messages are stored as they come from the ConsoleAPI code, with
 * all their properties. They are kept around until the inner window object that
 * created the messages is destroyed. Messages are indexed by the inner window
 * ID.

The message in this case seems to keep alive a JS call stack thing, which leaks the window.

Comment 6

[Andrew McCreight [:mccr8]]

At a conceptual level, this is exactly the kind of chrome JS -> content JS leak that the hueyfix should protect us from. However, rather than passing from chrome to content via a proxy (which is what the hueyfix relies on), this is actually chrome JS -> C++ (specifically JSStackFrame) -> content JS.

Comment 7

[Andrew McCreight [:mccr8]]

Boris, does it sound reasonable to keep a per window set of JSStackFrame objects so that we can somehow stop them from leaking windows at the same time we hueyfix the window CCWs? I don't know how many of them there are. I guess JSStackFrames could be kept in a linked list or something, so the overhead would only really be one pointer per frame.

Comment 8

[Boris Zbarsky [:bzbarsky]]

This is similar to the issue I ran into in bug 1438553.   See bug 1438553 comment 8...

That said, shouldn't navigating away from a page or otherwise unloading it trigger  nsGlobalWindowInner::FreeInnerObjects which will then dispatch the "inner-window-destroyed" notification, which should make ConsoleAPIStorage.js drop the corresponding entries?  Is that not happening for some reason?

But yes, we could stick "root" JSStackFrames on the window and have them all drop their stacks when the window is being hueyfixed.

Comment 9

[Andrew McCreight [:mccr8]]

Good point. I wasn't sure when that happened, but it looks like that notification happens at the same time as the nuking does. I'll look over the ConsoleAPIStorage.js code to try to figure out what might be going wrong.

Comment 10

[Andrew McCreight [:mccr8]]

I still see a ghost window on FB on my current session, so I should look at this.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Make ConsoleAPIStorage leaky and use that to make a test leak. DO NOT LAND.

After a fair amount of trouble, I managed to get a similar leak to trigger. First, I modified ConsoleAPIStorage to leak everything that is passed into it. Second, I added a call to console.trace() to an existing XPConnect browser chrome mochitest that I think just opens and closes a page. The trace causes JSStackFrame object to be passed to the ConsoleAPIStorage service, and it holds alive the content page. This needs some more work to be a landable test but at least I have something I can test with locally.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: 'hueyfix' for JSStackFrames. WIP

JSStackFrames are C++ objects that are exposed to chrome JS and keep alive content JS. This means that if chrome JS leaks a stack frame then a window can be leaked.

To prevent this, this patch modifies the compartment private to keep a list of all live JSStackFrames that have been created with objects in that compartment. When we nuke that compartment, we also clear out all of the JS pointers from the registered stack frames on that compartment.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Bug 1471989 - Clear JSStackFrame's JS object pointer when the window goes away.

JSStackFrames are C++ objects that are exposed to chrome JS and keep
alive content JS. This means that if chrome JS leaks a stack frame
then a window can be leaked.

The basic idea of this patch is to think of JSStackFrames as
cross-compartment wrappers, and do a "hueyfix" on them by dropping the
content JS reference when the associated content window is closed.

To do that, this patch modifies the compartment private to keep a list
of all live JSStackFrames that have been created with objects in that
compartment. When we nuke that compartment, we also clear out all of
the JS pointers from the registered stack frames on that compartment.

This adds a hash table lookup to the JSStackFrame ctor and dtor, which
is hopefully not too much overhead.

The test works by intentionally leaking a JSStackFrame from chrome JS
and making sure that the window still goes away.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1fe3750580d
Clear JSStackFrame's JS object pointer when the window goes away. r=bzbarsky

Comment 15

[Andreea Pavel [:apavel]]

Backed out for build bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b1fe3750580d45af6abdd858529658f0282dc6ae

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217944060&repo=autoland&lineNumber=14657

Backout: https://hg.mozilla.org/integration/autoland/rev/ef9d941b10a8ef56fd81ba86f43b2ac60ffbf6ac

Comment 16

[Andrew McCreight [:mccr8]]

I guess one of my changes to address review comments broke this, and I built on my Linux box which has warnings disabled because that's needed to build with icecc.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa90df2db90a
Clear JSStackFrame's JS object pointer when the window goes away. r=bzbarsky

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/fa90df2db90a