958315 - Suspect JS things released by dying C++ objects in the next CC

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Kyle had this idea.  Basically, when a C++ object dies, maybe only when it is destroyed during GC finalization, we figure out what JS it is holding onto, and suspect those JS objects during the next CC.  This would be a more general solution to the Promises problem in bug 887687, where long weird chains of JS and C++ held alive by dead JS objects end up taking a long time to free.

Comment 1

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

Actually, couldn't we just trace right before killing snow-white objects, and the tracer would
store those JS objects in a CCable object which holds js objects and which has a reference to itself.
Put that object to purple buffer, and it will be traversed next time CC is called.

Comment 2

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

Or ClearJSHolder could do this.

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, that sounds reasonable.  One annoying aspect of this is removing JS objects that die, as presumably you don't want the JS-purple buffer to hold strong references.

Comment 4

[Andrew McCreight [:mccr8]]

You could probably deal with that by adding a call in the browser's sweep callback that scans the list and removes anything that is being finalized.

Comment 5

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

So normally we get just few gray objects when we're about to kill snow-white objects.
But, mozilla::dom::exceptions::StackDescriptionOwner  may cause lots of stuff to
be added to the JS purple buffer.
My dummy implementation uses nsTArray to store JS::Heap stuff and with the number of
exceptions we get, that just doesn't work. Need to make something closer to the normal purple buffer.

Also, ClearJSHolder can't take the holders, since it is usually called after native object has
cleared references to JS.

Comment 6

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

Attachment: wip

https://hg.mozilla.org/try/rev/f89960baf5f2

This is one approach. Collect JS stuff just before we're about to delete
snow-white objects.

(and yes, I used 'auto')

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=f89960baf5f2
which interestingly doesn't seem to compile :)

Comment 8

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

Attachment: wip2

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

Comment 9

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

(and I can't reproduce the Promise "leak" with the patch, but can without.)

Comment 10

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

Comment on attachment 8358876 [details] [diff] [review]
wip2

I've been profiling this too since I was a bit worried about 
o.mParticipant->Trace(o.mPointer, *this, nullptr); call, but
so far I haven't seen anything where that matters.
o.mParticipant->DeleteCycleCollectable(o.mPointer); takes all the time.

The reason why we need to trace before DropJSObjects call in the dtor is that
JS member variables are cleared before DropJSObjects.
Adding Trace to all the participants, but keeping = 0 version of it in the
scriptholder participant should enforce scriptholder participants to reimplement it.

For the various Trace methods I just wanted to be consistent and handle all the cases, even if
some of those aren't ever called.

In theory we could store void* to gc things and make sure we clear the list before
any kind of gc, but that feels a bit scary. Better to use the common style and use
JS::Heap.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8358876 [details] [diff] [review]
wip2

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2163,5 @@
> +{
> +};
> +
> +template<class T>
> +class SegmentedArray

There's also nsSegmentedBuffer.h, though it is old-style.  Maybe if you want this new class it could be in a new file?

@@ +2202,5 @@
> +class TraceableContainer
> +{
> +public:
> +    TraceableContainer(TraceableContainer*& aReferenceToThis)
> +      : mReferenceToThis(aReferenceToThis)

aReferenceToThis doesn't look useful to me.

You should be able to use MOZ_THIS_IN_INITIALIZER_LIST() to initialize mReferenceToThis in the initializer list.

Can't you just make mReferenceToThis into an nsRefPtr<> and then not do the manual addref/release stuff?  What is the purpose of that extra reference, to be sure that this stays alive until a CC?  Isn't this going to be kept alive by the CC anyways?

@@ +2211,5 @@
> +    }
> +
> +    ~TraceableContainer()
> +    {
> +        MOZ_ASSERT(!mValues.GetFirstSegment());

Maybe define an Empty method for SegmentedArray()?  It would be easier to read.

@@ +2228,5 @@
> +        mObjects.Clear();
> +        mStrings.Clear();
> +        mScripts.Clear();
> +        mFunctions.Clear();
> +        mozilla::DropJSObjects(this);

It would be safer to call Drop in the dtor.

@@ +2232,5 @@
> +        mozilla::DropJSObjects(this);
> +        NS_RELEASE_THIS();
> +    }
> +
> +    NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(TraceableContainer)

It seems weird to cycle collect this, as it will always be definitely alive, but I suppose this is the easiest way to keep JS alive.

Oh, right, by making this a CCed object, they are added to the CC graph, which is the whole point of this.  Right.  I was wondering where the rest of the patch is.  This class definitely needs some more documentation describing what the point of it is.  "TraceableContainer" is not a very descriptive class name.

@@ +2336,5 @@
>      {
>        return mObjects.Length() > 0;
>      }
>  
> +    virtual void Trace(JS::Heap<JS::Value>* aValue, const char* aName,

Could you split out all of these Trace methods into a new class rather than adding it to SnowWhiteKiller?  It doesn't look like it needs to be here.

@@ +2345,5 @@
> +            mCollector->GetTraceableContainer()->mValues.AppendElement(*aValue);
> +        }
> +    }
> +
> +    virtual void Trace(JS::Heap<jsid>* aId, const char* aName,

Can jsid ever be an object?

@@ +2365,5 @@
> +
> +    virtual void Trace(JS::Heap<JSString*>* aString, const char* aName,
> +                       void* aClosure) const
> +    {
> +        if (*aString && xpc_GCThingIsGrayCCThing(*aString)) {

Strings are never CC things, so this method can be empty, and you don't need mStrings in the traceable container.

@@ +2378,5 @@
> +            mCollector->GetTraceableContainer()->mScripts.AppendElement(*aScript);
> +        }
> +    }
> +
> +    virtual void Trace(JS::Heap<JSFunction*>* aFunction, const char* aName,

Like strings, I think JSFunctions never have CC kind, so they won't be added.  But they can hold alive CC kind stuff (like getters and setters, I think?), so we still want to keep them around.  This will require special handling, because we have to add everything the script holds onto that has a CC kind as a root to the CC graph.

@@ +3237,5 @@
>      if (mIncrementalPhase == IdlePhase) {
>          MOZ_ASSERT(mGraph.IsEmpty(), "Non-empty graph when idle");
>          MOZ_ASSERT(!mBuilder, "Non-null builder when idle");
> +        if (mTraceableContainer) {
> +            mTraceableContainer->Clear();

I guess this is okay, because in the chain situation, the new GC will free the next link in the chain, so we'll get some fresh things in our purple buffer.

FWIW, PrepareForGarbageCollection should get called before every GC, so the GC should never end up tracing anything in the traceable container.

Comment 12

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

(In reply to Andrew McCreight [:mccr8] from comment #11)

> There's also nsSegmentedBuffer.h, though it is old-style.  Maybe if you want
> this new class it could be in a new file?
The new class is so trivial that I didn't bother to make it really public.
It is basically just wrapping a linkedlist
And nsSegmentedBuffer is horrible.


> Can't you just make mReferenceToThis into an nsRefPtr<> and then not do the
> manual addref/release stuff?
Yeah, I could do that

>  What is the purpose of that extra reference,
> to be sure that this stays alive until a CC?  Isn't this going to be kept
> alive by the CC anyways?
Something needs to addref/release

> Maybe define an Empty method for SegmentedArray()?  It would be easier to
> read.
true

> It would be safer to call Drop in the dtor.
I want to drop before GC runs.

> It seems weird to cycle collect this, as it will always be definitely alive,
> but I suppose this is the easiest way to keep JS alive.
The whole point is the object is just plain normal cycle collectable keeping
JS stuff alive.

> Oh, right, by making this a CCed object, they are added to the CC graph,
> which is the whole point of this. 
exactly


> Right.  I was wondering where the rest of
> the patch is.  This class definitely needs some more documentation
> describing what the point of it is.  "TraceableContainer" is not a very
> descriptive class name.
Yeah, couldn't come up anything good yet.


> Could you split out all of these Trace methods into a new class rather than
> adding it to SnowWhiteKiller?  It doesn't look like it needs to be here.
It was handy to add them to SWK, since that has already the reference to cycle collector.


> > +            mCollector->GetTraceableContainer()->mValues.AppendElement(*aValue);
> > +        }
> > +    }
> > +
> > +    virtual void Trace(JS::Heap<jsid>* aId, const char* aName,
> 
> Can jsid ever be an object?
AFAIK, no. I could add an assert. But the other tracer does similar thing.


> > +    virtual void Trace(JS::Heap<JSString*>* aString, const char* aName,
> > +                       void* aClosure) const
> > +    {
> > +        if (*aString && xpc_GCThingIsGrayCCThing(*aString)) {
> 
> Strings are never CC things, so this method can be empty, and you don't need
> mStrings in the traceable container.
ditto.


> > +    virtual void Trace(JS::Heap<JSFunction*>* aFunction, const char* aName,
> 
> Like strings, I think JSFunctions never have CC kind, so they won't be
> added.  But they can hold alive CC kind stuff (like getters and setters, I
> think?), so we still want to keep them around.  This will require special
> handling, because we have to add everything the script holds onto that has a
> CC kind as a root to the CC graph.
What? 
The tracer just copies whatever it gets from traceable and stores those in the normal way.
I'd prefer keep the tracing as it is in the patch - or change other tracers.

> FWIW, PrepareForGarbageCollection should get called before every GC, so the
> GC should never end up tracing anything in the traceable container.
Yes, which is why I want to clear the container before gc, so that gc can deal with the garbage.

Comment 13

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

Though, if some Trace() isn't implemented, we just don't even try to handle those js things
as traceable in cc. I'll simplify ...

Comment 14

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

Attachment: p1

A bit simpler. Just deal with JSObjects and JS::Values.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8359909 [details] [diff] [review]
p1

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

::: xpcom/base/nsCycleCollector.cpp
@@ +2168,5 @@
> +{
> +public:
> +    ~SegmentedArray()
> +    {
> +        MOZ_ASSERT(!mSegments.getFirst());

Please change this to MOZ_ASSERT(IsEmpty())

@@ +2203,5 @@
> +private:
> +    mozilla::LinkedList<SegmentedArrayElement<T>> mSegments;
> +};
> +
> +// JSPurpleBuffer keeps references to GCThings which might affect to the

nit: the second "to" should be removed ("affect to the")

@@ +2206,5 @@
> +
> +// JSPurpleBuffer keeps references to GCThings which might affect to the
> +// next cycle collection. It is owned only by itself and during unlink its
> +// self reference is broken down and the object ends up killing itself.
> +// If gc happens before cc references to GCthings and the self reference are

nit: "GC" and "CC".  Also, a comma after CC would be nice. ie "before CC, references to"

Please document somewhere in here that mReferenceToThis is a weak reference from nsCycleCollector.

@@ +2358,5 @@
> +    {
> +    }
> +
> +    virtual void Trace(JS::Heap<JSScript*>* aScript, const char* aName,
> +                       void* aClosure) const

Scripts are CC things, but I guess we can change this if we find a problem somewhere.  They are a bit weird so I think it should be okay.

@@ +3400,5 @@
> +nsCycleCollector::GetJSPurpleBuffer()
> +{
> +  if (!mJSPurpleBuffer) {
> +    // JSPurpleBuffer keeps itself alive, but we need to create it in such way
> +    // that it ends up to the normal purple buffer. That happens when

nit: "ends up to" should be "ends up in".

@@ +3402,5 @@
> +  if (!mJSPurpleBuffer) {
> +    // JSPurpleBuffer keeps itself alive, but we need to create it in such way
> +    // that it ends up to the normal purple buffer. That happens when
> +    // nsRefPtr goes out of the scope and calls Release.
> +    nsRefPtr<JSPurpleBuffer> pb = new JSPurpleBuffer(mJSPurpleBuffer);

A less tricky way to make sure we traverse the JSPurpleBuffer would be to explicitly add mJSPurpleBuffer to the graph near the end of nsCycleCollector::BeginCollection, after we do mPurpleBuf.SelectPointers. (with a null check, of course)

Comment 16

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

> A less tricky way to make sure we traverse the JSPurpleBuffer would be to
> explicitly add mJSPurpleBuffer to the graph near the end of
> nsCycleCollector::BeginCollection, after we do mPurpleBuf.SelectPointers.
> (with a null check, of course)
Well, that would make JSPurpleBuffer more magical and less like normal CCable object

Comment 17

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

Attachment: +nits

Comment 18

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

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

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be73d80464d

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3be73d80464d