1313098 - Ensure thread safe access to runtime's heap state

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Currently this has accessors that may be called off the main thread.  We should assert that this is only ever accessed in a safe way and fix the problems that show up.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1313098-assert-heap-state-accesses

Patch to ensure thread safe access to the runtime's heap state.

This means only written to on the main thread and only read on the main thread or from a thread performing GC on behalf of the main thread (with the main thread running collector code at this point and not running the mutator).

I broadened the scope of our per-thread state that previously told us whether a thread was sweeping to be whether the thread is running any GC code.  I could have used more fine-grained state to differentiate this but I didn't think it was worth it.

There were a couple of places where we did access the heap state from the wrong thread:
 - barrier code where we're looking at e.g. a permanent atom belonging to another runtime

I merged ArenaCellIterUnderFinalize and ArenaCellIterUnderGC and added ArenaCellIter.  These assert they're called correctly.

I removed a heap state check from write barriers in a couple of places.  This is not needed if we check the needsIncrementalBarrier flag on the zone, which will always be false if we're on the main thread and currently inside a GC.

Next up I guess we need to check that we have access to the zone.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1313098-dont-expose-in-finalizer

Patch to stop triggering a barrier in finalisation.  This us allows us to assert that we're not collecting in IsIncrementalBarrierNeededOnTenuredGCThing.

Comment 3

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

Comment on attachment 8804792 [details] [diff] [review]
bug1313098-assert-heap-state-accesses

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

Wow, nice! It's making more sense now.

::: js/public/HeapAPI.h
@@ +380,5 @@
>      MOZ_ASSERT(thing);
>      MOZ_ASSERT(!js::gc::IsInsideNursery(thing.asCell()));
> +
> +    // TODO: I'd like to assert !isHeapBusy() here but this gets called
> +    // while we are tracing the heap, e.g. during memory reporting.

Can you put a bug number here?

::: js/src/gc/Allocator.cpp
@@ +293,5 @@
>      MOZ_ASSERT(!cx->runtime()->isHeapBusy(), "allocating while under GC");
>  
>      AutoMaybeStartBackgroundAllocation maybeStartBGAlloc;
>  
> +    return arenas->allocateFromArena(zone, thingKind, true, maybeStartBGAlloc);

Hm. The bool parameter is kind of mysterious here. Please either (1) change it to a class enum (CheckThresholds, NoCheckThresholds), or (2) move it to the end of the parameter list and default it to true, then don't pass it here and the other place where it's true, but comment the case where you pass in false.

@@ +429,5 @@
>      Arena* arena = chunk->allocateArena(rt, zone, thingKind, lock);
>      zone->usage.addGCArena();
>  
>      // Trigger an incremental slice if needed.
> +    if (checkThresholds)

It is nice to not repeat the whole !rt->isHeapMinorCollecting() && !isHeapCompacting()

Comment 4

[Jon Coppeard (:jonco)]

These patches are causing assertion failures on try because JitcodeGlobalTable::lookupForSamplerInfallible gets called on a random thread and asserts based on heap state.

I also had to add the per-thread sweeping state back in because I realised it wasn't subsumed by the collecting state: the background sweep task operates in parallel with the mutator.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1313098-dont-expose-in-move-hook

We need to manually avoid that barrier in one more place.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8807546 [details] [diff] [review]
bug1313098-dont-expose-in-move-hook

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +1742,5 @@
>    JS::AutoSuppressGCAnalysis nogc;
>  
>    auto entry =
>      static_cast<NPObjWrapperHashEntry*>(sNPObjWrappers->Search(npobj));
>    MOZ_ASSERT(entry && entry->mJSObj);

I guess a boolean check already doesn't trigger the barrier?

@@ +1743,5 @@
>  
>    auto entry =
>      static_cast<NPObjWrapperHashEntry*>(sNPObjWrappers->Search(npobj));
>    MOZ_ASSERT(entry && entry->mJSObj);
> +  MOZ_ASSERT(entry->mJSObj.unbarrieredGetPtr() == old);

Is there no overload for operator== to handle this without a barrier? I thought some cases were covered.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I guess a boolean check already doesn't trigger the barrier?

We have operator bool() that doesn't trigger the read barrier.

> Is there no overload for operator== to handle this without a barrier? I
> thought some cases were covered.

Sadly not.  We're working on this in bug 1308236.  Partly this is because |old| is |const JSObject*| here.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f246bb6108c
Assert we only access heap state on the main thread r=sfink r=mccr8

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f246bb6108c