1463343 - AddToCCKind is ambiguous

Status: RESOLVED DUPLICATE of bug 1522061

(Core :: XPCOM, defect)

Description

[Steve Fink [:sfink] [:s:]]

I was just looking at a bug with arai, and ran into some confusing terminology.

CycleCollectedJSRuntime.h has a function AddToCCKind that returns true for various JS TraceKinds.

TraceKind.h has a per-TraceKind field named AddToCCKind.

The meanings of these two things is not at all the same.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Reduce "CC kind" confusion

I think this captures what is going on better. Any additional terminology improvements would be welcome!

Also, I wonder if AddToCCKind() should document what it means for it to be false? Or perhaps it should describe what both mean in combination. As in, my understanding is that when AddToCCKind() returns false, the GC graph will still be traversed as long as IsCCParticipant is true, and all JS Cells that are both reachable through that cell as well as in AddToCCKind will be added to the graph. Ugh, that sounded clunky. "The cycle collector graph includes all gray-reachable AddToCCKind Cells, with edges between them representing whether they are reachable through gray IsCCParticipant Cells."?

Or maybe I should have s/IsCCParticipant/TraverseForCC/?

Comment 2

[Steve Fink [:sfink] [:s:]]

Oh, right. The original reason for this patch was documenting that anything used as a weakmap key needs to return true for AddToCCKind().

Comment 3

[Steve Fink [:sfink] [:s:]]

Also, I ran into this in looking at something arai did in bug 1434305. Arai's version of the comment is:

// Returns true if the JS::TraceKind is one the cycle collector cares about.
// Everything used as WeakMap key should be listed here, to represent the key
// in cycle collector's graph, otherwise the key is considered to be pointed
// from somewhere unknown, and results in leaking the subgraph which contains
// the key.
	
// See the comments in NoteWeakMapsTracer::trace for more details.

So I still don't entirely understand what was going on there. This leak (the one in bug 1434305) makes sense to me if there is a cycle through the LazyScript WeakMap key back to C++, and perhaps that is what is happening? But it seems weird. I don't really understand why this would leak if there isn't such a cycle -- I would think that if LazyScript were not in AddToCCKind then the CC would mark all of this stuff grey, but that marking would stop too early when it hit the LazyScript. But if there isn't a cycle, then the C++ stuff that's pointing into these objects would have a refcount of zero anyway, so it wouldn't need to traverse through here. So it only makes sense to me for this to matter if we actually have a path through this edge in the graph that ends up back at a C++ object. Is that expected/likely?

More specifically, we have

  Window -> JSFunction -> JSScript -> LazyScript -> JSFunction
  LazyScript -> (via lazyScripts WeakMap) Debugger.Script (a JSObject) -> LazyScript
  enclosing Scope -> JSFunction

and I suppose

  JSFunction -> JSObject global -> Window

So I don't see why the LazyScript is relevant; if you have a cycle Window -> ... -> JSFunction -> ... -> Window, then you can already see a cycle. The only thing that LazyScript -> Debugger.Script adds in is the participation of a different compartment. Maybe that's what is relevant here?

Anyway, all that should really be a comment on bug 1434305, but I only really care from the perspective of getting that comment to make sense to me.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8979432 [details] [diff] [review]
Reduce "CC kind" confusion

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

Oops, I forgot about this review. Sorry.

::: js/public/TraceKind.h
@@ +85,5 @@
>  
>  // When this header is used outside SpiderMonkey, the class definitions are not
>  // available, so the following table containing all public GC types is used.
>  #define JS_FOR_EACH_TRACEKIND(D) \
> + /* PrettyName       TypeName           IsCCParticipant */ \

Maybe document what IsCCParticipant means? eg it could be a part of a cycle?

::: xpcom/base/CycleCollectedJSRuntime.h
@@ +410,5 @@
>  
>  void TraceScriptHolder(nsISupports* aHolder, JSTracer* aTracer);
>  
> +// Returns true if the JS::TraceKind is one the cycle collector should create a
> +// node in the cycle collection graph. Note that this must be true for any

Maybe reword the first sentence to "Returns true if the cycle collector should create a node in the cycle collector graph for a GC thing with this JS::TraceKind"?

Comment 5

[Steve Fink [:sfink] [:s:]]

Yeah, that's a lot better than my mess of a sentence.

While we're here, what do you think of arai's replacement for the second sentence (from bug 1434305):

> +// Everything used as WeakMap key should be listed here, to represent the key
> +// in cycle collector's graph, otherwise the key is considered to be pointed
> +// from somewhere unknown, and results in leaking the subgraph which contains
> +// the key.
> +// See the comments in NoteWeakMapsTracer::trace for more details.

I may let him land first, to avoid bitrotting his higher priority patch, but I can adjust the final wording according to what we decide here.

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #5)
> Yeah, that's a lot better than my mess of a sentence.
> 
> While we're here, what do you think of arai's replacement for the second
> sentence (from bug 1434305):

That seems okay, though maybe it can cause problems like UAFs, too. I haven't thought about it in a while, but not tracing grey JS can be bad besides just causing leaks.

Comment 7

[Jon Coppeard (:jonco)]

Steve, can this land now?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sfink, could you have a look please?

Comment 10

[Jens Stutte [:jstutte]]

Bug 1522061 has brought more clarity here, it seems. Marking duplicate, though the other bug changed some more things, too.