966762 - Add a chrome JS function to unlink ghost windows

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

When investigating a leak that persists until shutdown, you can use a cycle collector log to figure out what object is keeping everything alive, but figuring out what is holding alive that object is difficult.  For leaks that don't persist until shutdown, you can break on the Release() method for the leaking object to figure out what the missing reference is.

One way to turn the former into the latter may be to add a method that unlinks all ghost windows.  The hope is that the teardown for a window would break whatever cycle is holding the leaking object alive, thus causing it to be Released(), whereupon we can figure out what was holding it alive.

The first step would be to throw together a patch that can do this.  The ghost window detector keeps a list of such windows, so calling Unlink on them should be easy.  If it turns out to be useful, we could land it in the tree.

If we really want to be bold, we could try periodically purging ghost windows that are top level windows or something, though that has all sorts of drawbacks, so it would be best done in a followup.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Ghost buster, WIP

This is effective enough to tear down the ghost windows created by the test case in bug 951491.

Comment 2

[Andrew McCreight [:mccr8]]

To use, apply the patch and build.  Then, from the browser console, run |Components.utils.ghostBuster();| and it will call Unlink on all ghost windows.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Ghost buster, WIP

Now with "I ain't afraid of no ghost!" button in about:memory.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: indicate we want to refcount log the unlinked window, WIP

This patch adds a new method to TraceCountRefLogging called ThisObjectIsInteresting, and calls
it on any object we're unlinking from the GhostBuster.  What that does, is once we indicate that an object is interesting,
then we start refcount logging it.  Thus, when we GhostBust it, we can look at the refcount log to see what actually
caused the extra release.

You then run Firefox in some way like the following:
  XPCOM_MEM_REFCNT_LOG=./rclog.txt XPCOM_MEM_LOG_CLASSES=nsGlobalWindow XPCOM_MEM_LOG_OBJECTS=9999 ./mach run -P debug

./rclog.txt is the name of the file you want to save.  nsGlobalWindow is the class you want to log.  I'm not sure
if the LOG_OBJECTS is necessary, but I think it is needed to initialize some hash table we use.

Once you have the log, you want to do something like:
  python tools/rb/fix_macosx_stack.py < rclog.txt > fixed_rclog.txt

which will symbolicate the stacks, and may take a little while.  On Linux, there's a different
script in the same directory (tools/rb is a subtree of m-c) that you have to use.

When I tried using this, the resulting stack was kind of crummy, so I had to get out a debugger and breakpoint
at the right spot, but that's probably because I had an optimized debug build.

But anyways, with these two patches, I was eventually able to solve bug 951491, which was otherwise
intractable.

Comment 5

[Andrew McCreight [:mccr8]]

This doesn't depend on bug 951491 per se, but I used to to fix it.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Add chrome JS function to unlink ghost windows. r=smaug

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Add a button to about:memory for the ghost buster, for convenience. DONT LAND

Comment 8

[Andrew McCreight [:mccr8]]

I filed bug 974187 for the refcount logging portion of this, as I'm not quite sure how I want to deal with that for real.

Comment 9

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=bd00a78ee9b4

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8377913 [details] [diff] [review]
Add chrome JS function to unlink ghost windows. r=smaug

+ nsGlobalWindow* window = windowsById->Get(aIDHashKey->GetKey());
Make that nsRefPtr so that window doesn't die during Unlink.

bholley should probably f+ nsIXPCComponents_Utils change.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8377913 [details] [diff] [review]
Add chrome JS function to unlink ghost windows. r=smaug

Bobby, are these XPCComponents changes okay with you?

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8377913 [details] [diff] [review]
Add chrome JS function to unlink ghost windows. r=smaug

Review of attachment 8377913 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/idl/xpccomponents.idl
@@ +279,5 @@
>       */
>      void schedulePreciseShrinkingGC(in ScheduledGCCallback callback);
>  
> +    /*
> +     * In a debug build, unlink any ghost windows. This may cause badness.

Please be much more explicit here, and indicate that this is for leak debugging only.

::: js/xpconnect/src/XPCComponents.cpp
@@ +2856,5 @@
>      nsRefPtr<PreciseGCRunnable> event = new PreciseGCRunnable(aCallback, true);
>      return NS_DispatchToMainThread(event);
>  }
>  
> +/* void ghostBuster(); */

This comment is out of date.

Comment 13

[Andrew McCreight [:mccr8]]

> Make that nsRefPtr so that window doesn't die during Unlink.

I think this isn't actually needed, because the |windowsById->Enumerate(GetWindows, &windows);| call is supposed to keep all of the windows alive, but I added the ref ptr anyways, because raw pointers are scary.

Addressed review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1853f837d121

Comment 14

[Andrew McCreight [:mccr8]]

backed out in the hope that it will fix valgrind leaks
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f682933ea9

Comment 15

[Andrew McCreight [:mccr8]]

Valgrind problem was another patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7eeeff1a47d

Comment 16

[Carsten Book [:Tomcat]]

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