MarkExactStackRoot was written with the original concept of Rooted in mind. It is meant to adequately mark pointers to movable GC things. It has been pressed into service against its will as a marking mechanism for stack-based structs containing GC things. People (myself included) wanted to use the Rooted<Foo> idiom, even when Foo was not a GC thing. This is bad, because MarkExactStackRoot expects it to be. The IsNullTaggedPointer() check operates on the deref of Rooted<void *>->address(). For rooteds as intended, this makes good sense. We don't want to mark a |nullptr| JSObject *, and what lives on the stack is a single word. For stack-based rooted structs, this makes no sense at all. It grabs the first word of the struct, and compares it against an arbitrary series of checks, and decides whether to trace the *whole struct* based on the results of those checks. This is an enormous and intolerable footgun.

Seems to me like efaust is right. IsNullTaggedPointer really isn't valid for anything that isn't a Cell*. Which means we shouldn't be calling it for Value, jsid, Bindings, JSPropertyDescriptor, or THING_ROOT_CUSTOM. And we should make it harder to get this wrong.

Oh, and types::Type.

(In reply to Eric Faust [:efaust] from comment #0) Nice catch, this is just wrong. We really should be using typed Rooted rather than Rooted<void*> where possible.

Comment on attachment 8434617 [details] [diff] [review] Proposed Fix Review of attachment 8434617 [details] [diff] [review]: ----------------------------------------------------------------- I would probably spell this a little differently, by making a ShouldNotMarkRooter(kind, rooter) that handles both IsNullTaggedPointer and the TaggedProto::LazyProto thing internally. But either way, it sounded like on IRC that jonco has a patch that avoids casting away the type info here, so I'm going to transfer review to him.

Here's a patch to type the marking code for Rooters. This also removes the current RootedGeneric template and reimplements it as a subclass of CustomAutoRooter, which means we can get rid of the pointer arithmetic necessary to find the vtable. I don't think this should be a problem. Try run here: https://tbpl.mozilla.org/?tree=Try&rev=fc80317acc72

Comment on attachment 8435054 [details] [diff] [review] type-root-marking Review of attachment 8435054 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/RootMarking.cpp @@ +72,5 @@ > +{ > + return IsNullTaggedPointer(*thingp) || *thingp == TaggedProto::LazyProto; > +} > + > +template <class T, void (*MarkFunc)(JSTracer *trc, T *ref, const char *name), class Source> I think you should templatize on the function, not the function pointer, for MarkFunc. @@ +109,5 @@ > + MarkExactStackRootsForType<jsid, MarkIdRoot>(trc, "exact-id"); > + MarkExactStackRootsForType<Value, MarkValueRoot>(trc, "exact-value"); > + MarkExactStackRootsForType<types::Type, MarkTypeRoot>(trc, "exact-type"); > + MarkExactStackRootsForType<Bindings, MarkBindingsRoot>(trc); > + MarkExactStackRootsForType<JSPropertyDescriptor, MarkPropertyDescriptorRoot>(trc); Why no name for these last two? ::: js/src/jscntxt.cpp @@ +265,5 @@ > } > > +#if defined(JSGC_USE_EXACT_ROOTING) && defined(DEBUG) > +void > +ContextFriendFields::checkNoGCRooters() { I'd prefer the #if goop to only show up once. Can you always define checkNoGCRooters(), and #ifdef the body?

Patch with review comments applied.

(In reply to Steve Fink [:sfink] from comment #7) > > + MarkExactStackRootsForType<Bindings, MarkBindingsRoot>(trc); > > + MarkExactStackRootsForType<JSPropertyDescriptor, MarkPropertyDescriptorRoot>(trc); > > Why no name for these last two? The existing marking mechanisms for these don't use the name (and it made the lines wrap) so I got rid of it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/846ee7c7debf sfink pointed me to the Try run for this over IRC. In the future, please include the link at the time of requesting checkin.

Backed out for bustage that sfink says has his name all over it. https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e6b3caa5a2 https://tbpl.mozilla.org/php/getParsedLog.php?id=41133777&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41134045&tree=Mozilla-Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11) > Backed out for bustage that sfink says has his name all over it. Actually, now I'm unsure of the exact problem, since it's only failing the ICS emulator build. The problem is static void foo() { ... } template <void (F)()> { ... } it doesn't want to instantiate that template because F doesn't have external linkage. If it were template <void (*F)()>, that would sort of make sense. Maybe. Well, not really. But I really don't understand why it has a problem with what's in the actual push, unless the extra parens around (F) are being interpreted as an implicit (*F) or something. Anyway, I'm looking into it. Probably ought to figure out how to do the ics emulator builds locally. I've been doing some emulator-jb builds recently, but it looks like those are succeeding. :(

Ugh. I think the external linkage thing may be a red herring. I think the problem is that gcc 4.4 doesn't allow template <typename T, void F(T&)> ... (variadic template arguments?) where one template parameter T is part of the type of another template parameter F.

Re-pushed, with template functions switched to non-static to appease gcc 4.4. http://hg.mozilla.org/integration/mozilla-inbound/rev/7ffbe6899aed

(In reply to Steve Fink [:sfink] from comment #14) Thanks for sorting that out!