1326507 - What should happen when two classes that inherit from each other both want to trace JS things?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Say we have two C++ classes that we CC: A and B.  A inherits from B.  Both hold on to JS things that they want to trace.

The obvious way for someone to write this is to have:

  class A {
    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(A)
  };

  class B : public A {
    NS_DECL_ISUPPORTS_INHERITED
    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(B, A)
  };

and then:

  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(A)
    // trace stuff
  NS_IMPL_CYCLE_COLLECTION_TRACE_END
  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(A)
    // drop stuff
  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(A)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(B, A)
    // trace stuff
  NS_IMPL_CYCLE_COLLECTION_TRACE_END
  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(B, A)
    // drop stuff
  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(B, A)
    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Note that this will end up tracing everything _twice_ for a B instance. Specifically, when we hit the subclass Traverse we will call the superclass traverse, which calls _our_ Trace, and then call out Trace again.  Both times our Trace is called we will call the superclass Trace.

One solution is for the subclass to not do NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS, but then what happens if the superclass stops being a script holder class?  Will we have a compile failure, or just silently not trace?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I just audited all of our NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED users.  Most of them don't NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in the subclass, some with explicit comments that the superclass does it.  But PerformanceMainThread does do it.  And some code I wrote this week ended up doing it....

Comment 2

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

From CC point of view extra use of NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS should mean as if there were more JS member variables pointing to the same js object.
White marking does still rely on
if (pi->mInternalRefs == pi->mRefCount || pi->IsGrayJS()) {

Comment 3

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

CC logging might look a bit weird though.

Comment 4

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

What if we always called Trace after calling Traverse and removed NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS. Hopefully that wouldn't slow things down too much.
Most of the objects are script_holders anyhow. 
Though, we could avoid the virtual call (if we don't want to rely on compiler to optimize it out) using similar boolean flag to canSkip stuff.

We could rename Traverse to TraverseInternal and then add a new Traverse to nsCycleCollectionParticipant and that one would call first TraverseInternal and then 
TraceCallbackFunc noteJsChild(&nsScriptObjectTracer::NoteJSChild);
Trace(aPtr, noteJsChild, &aCb);

Comment 5

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

Attachment: Trace after traverse

Comment 6

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

Attachment: remove unneeded macro

Comment 7

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

Attachment: rename Traverse to TraverseNative

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b675c72a59aea5cf6f888fc5f8fa009cd57acdd6

Comment 8

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

hmm, try: -a doesn't seem to do anything useful anymore :/

Comment 9

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

Maybe  https://treeherder.mozilla.org/#/jobs?repo=try&revision=199e6c9396cf1290712ca8860ce65dadf105c4f4
 works

Comment 10

[Andrew McCreight [:mccr8]]

Yeah, comment 4 matches what I was thinking would be a nice improvement here.

> Will we have a compile failure, or just silently not trace?

We'll just silently not trace. It'll still trace it for the GC, I think, so I don't think we'll get UAFs. I'm not sure if we'll get leaks or use after unlinks from the lack of CC traversal.

Comment 11

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

Comment on attachment 8823127 [details] [diff] [review]
Trace after traverse

The tryserver issues looked like existing issues, but I retriggered tests anyhow.

About NS_SUCCESS_INTERRUPTED_TRAVERSE, see for example FragmentOrElement.cpp part in the next patch.

Comment 12

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

Comment on attachment 8823129 [details] [diff] [review]
rename Traverse to TraverseNative

I did this mainly to ensure I had looked at all the Traverse impls, and the
naming makes TraverseNativeAndJS from the first patch a tad more understandable.
But happy leave this out too.


(I think we could simplify nsCycleCollectionParticipant handling a bit more. Merging nsCycleCollectionParticipant, nsScriptObjectTracer and nsXPCOMCycleCollectionParticipant might make sense.)

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8823129 [details] [diff] [review]
rename Traverse to TraverseNative

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

This is probably a good idea, as "Traverse" sounds like it would traverse everything. I guess TraverseButNotTrace is not a good name. :)

::: xpcom/base/CycleCollectedJSContext.cpp
@@ +303,5 @@
>      reinterpret_cast<char*>(this) - offsetof(CycleCollectedJSContext,
>                                               mGCThingCycleCollectorGlobal));
>  
>    JS::GCCellPtr cellPtr(aPtr, JS::GCThingTraceKind(aPtr));
>    runtime->TraverseGCThing(CycleCollectedJSContext::TRAVERSE_FULL, cellPtr, aCb);

Here's a minor argument for keeping the Traverse name: this traverses JS but not anything native! :) It doesn't matter much, though.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8823127 [details] [diff] [review]
Trace after traverse

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

Thanks for fixing this, and thanks to Boris for pointing out how the current setup is a little dangerous.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +113,5 @@
>  class NS_NO_VTABLE nsCycleCollectionParticipant
>  {
>  public:
> +  constexpr nsCycleCollectionParticipant()
> +  : mMightSkip(false)

I think these should be indented more. Also for the other ctor.

@@ +128,4 @@
>  
> +  NS_IMETHOD Traverse(void* aPtr, nsCycleCollectionTraversalCallback& aCb)
> +  {
> +    return NS_OK;

So this can't just be an abstract method any more? Kind of weird, but I guess the worst that could happen is somebody forgets a Traverse but that isn't too big of a deal.

@@ +135,5 @@
> +                               nsCycleCollectionTraversalCallback& aCb)
> +  {
> +    nsresult rv = Traverse(aPtr, aCb);
> +    if (mTraverseShouldTrace) {
> +      // Note, we call Trace always, even if Traverse returned

nit: I think "we always call Trace" sounds better than "we call Trace always".

@@ +146,5 @@
> +
> +    // Implemented in nsCycleCollectorTraceJSHelpers.cpp.
> +  static void NoteJSChild(JS::GCCellPtr aGCThing, const char* aName,
> +                          void* aClosure);
> +  

nit: trailing whitespace

Comment 15

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

Attachment: traverse_native_and_js_2.diff

Looks like Traverse can be abstract after all, at least on linux.

Comment 16

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

Attachment: rename_traverse_to_traverseNative_2.diff

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=287ca834925d36c676031f3d4d0cd8e9616efbf2

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc85edeec99
trace after traverse, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/910c551b4d72
remove NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ab104b7f1e
rename Traverse to TraverseNative, r=mccr8

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged on top of

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8823516 [details] [diff] [review]
Part 1 merged on top of

Bah, bzexport fail.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bc85edeec99
https://hg.mozilla.org/mozilla-central/rev/910c551b4d72
https://hg.mozilla.org/mozilla-central/rev/a4ab104b7f1e