944040 - Do not fire post-barriers during moving GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: no_barriered_on_stack-v0.diff

The general problem here is when we have a barrier hidden behind a generic type and we use the generic type directly in the store buffer. When we do store buffer marking the generic type passes through the stack and inserts more things into the store buffer. There are two common failure modes here: (1) accidentally messing up the buffer iteration pointer and sending StoreBuffer::mark into an infinite loop and (2) accidentally upcasting Relocatable to Encapsulated and losing an unput.

This has bitten us enough times, both with new code and with changed code triggering the wrong behavior, that we should really just fix the underlying problem. There were only a handful of places that ended up needing attention after removing the generic HashTableWriteBarrierPost interface.

1) Debugger::liveScopes -> no barriers, so just a trivial generic barrier.
2) Debugger::proxiedScopes -> we know the type, so it is trivial to cast to unbarriered.
3) WeakMapObject -> again, we know the type so we can just cast out of it.
4) WeakMap -> we don't know the type here, so I added a template Unbarriered to automatically strip the HeapPtr.
5) MapObject/SetObject -> again we know the types; however, in this case we already have a totally custom generic buffer entry. This is quite a bit of code, but I don't think it is substantially more confusing that what is already there.

Note: the assertions enforcing this change are in bug 902174 and should land soon after this.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8339471 [details] [diff] [review]
no_barriered_on_stack-v0.diff

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

This looks good!

It would have been nice to somehow do this automatically, but like you said, I think it would have actually been more code.

::: js/src/vm/ScopeObject.cpp
@@ +1506,5 @@
> +     * This will automatically ensure that barriers do not fire during GC.
> +     */
> +    typedef WeakMap<JSObject *, JSObject *> UnbarrieredMap;
> +    typedef gc::HashKeyRef<UnbarrieredMap, JSObject *> Ref;
> +    if (key && !IsInsideNursery(rt, key))

I think this should be |IsInsideNursery(rt, key))| rather than |!IsInsideNursery(rt, key))|.

Comment 2

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8339471 [details] [diff] [review]
> ::: js/src/vm/ScopeObject.cpp
> @@ +1506,5 @@
> > +     * This will automatically ensure that barriers do not fire during GC.
> > +     */
> > +    typedef WeakMap<JSObject *, JSObject *> UnbarrieredMap;
> > +    typedef gc::HashKeyRef<UnbarrieredMap, JSObject *> Ref;
> > +    if (key && !IsInsideNursery(rt, key))
> 
> I think this should be |IsInsideNursery(rt, key))| rather than
> |!IsInsideNursery(rt, key))|.

O_O  Good catch! I'm surprised that didn't bite during testing; jit-tests --tbpl is still green after the change though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab7ece2fd805

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab7ece2fd805