Closed Bug 1326507 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: smaug)

References

Details

Attachments

(3 files, 3 obsolete files)

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?
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....
Depends on: 1326508
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()) {
CC logging might look a bit weird though.
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);
Attached patch Trace after traverse (obsolete) — Splinter Review
hmm, try: -a doesn't seem to do anything useful anymore :/
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 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.
Attachment #8823127 - Flags: review?(continuation)
Attachment #8823128 - Flags: review?(continuation)
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.)
Attachment #8823129 - Flags: review?(continuation)
Attachment #8823128 - Flags: review?(continuation) → review+
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.
Attachment #8823129 - Flags: review?(continuation) → review+
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
Attachment #8823127 - Flags: review?(continuation) → review+
Looks like Traverse can be abstract after all, at least on linux.
Attachment #8823127 - Attachment is obsolete: true
Attachment #8823129 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
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
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee: bzbarsky → bugs
You need to log in before you can comment on or make changes to this bug.