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....

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);

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

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

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.

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 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 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

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

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

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