Closed Bug 1471989 Opened 2 years ago Closed Last year

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 file, 2 obsolete files)

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. :)
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.
Priority: -- → P2
Andrew can you follow up on this with your existing session.
Flags: needinfo?(continuation)
Whiteboard: [MemShrink] → [MemShrink:P1]
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.
Flags: needinfo?(continuation)
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.
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.
Summary: Ghost windows on Facebook → Ghost windows on Facebook from JS stack from held alive by ConsoleAPIStorage.js
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
See Also: → 1438553
I still see a ghost window on FB on my current session, so I should look at this.
Flags: needinfo?(continuation)
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.
Attached patch 'hueyfix' for JSStackFrames. WIP (obsolete) — Splinter Review
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.
Flags: needinfo?(continuation)
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.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1fe3750580d
Clear JSStackFrame's JS object pointer when the window goes away. r=bzbarsky
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.
Flags: needinfo?(continuation)
Attachment #9021353 - Attachment is obsolete: true
Attachment #9021344 - Attachment is obsolete: true
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa90df2db90a
Clear JSStackFrame's JS object pointer when the window goes away. r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/fa90df2db90a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.