1281472 - TSan: data race in js::CheckTracedThing<js::Shape> during compacting GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: TSAN complaints

I'm filing a separate bug for this TSAN race because the stack frame is not the same as any existing bug.  That said, it's generally related to compacting GC and hence bug 1261083.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: fix-debug-build-race

There are a couple of changes necessary to CheckTracedThing.  Calling MaybeForwarded will call MakeAccessibleAfterMovingGC may update the contents of the thing, which is not required here.

Also, the IsThingPoisoned check will read the thing's contents which may race with updates on other threads.  I've just taken that check out when we are compacting.  (TBH I still don't completely understand the logic of this check).

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8764646 [details] [diff] [review]
fix-debug-build-race

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

(In reply to Jon Coppeard (:jonco) from comment #1)
> Created attachment 8764646 [details] [diff] [review]
> fix-debug-build-race
> 
> There are a couple of changes necessary to CheckTracedThing.  Calling
> MaybeForwarded will call MakeAccessibleAfterMovingGC may update the contents
> of the thing, which is not required here.
> 
> Also, the IsThingPoisoned check will read the thing's contents which may
> race with updates on other threads.  I've just taken that check out when we
> are compacting.  (TBH I still don't completely understand the logic of this
> check).

It would be more readable if it was just MOZ_ASSERT(!InFreeList): the idea being that if we're tracing the thing then it's allocated. But that's too slow even in debug, so instead Bill implemented as MOZ_ASSERT(!IsPoisoned). But that doesn't always work because strings don't initialize their inline data unless they're inline strings. So we fall back to MOZ_ASSERT(!IsPoisoned || !InFreeList), or rather the equivalent logic of MOZ_ASSERT_IF(IsPoisoned, !InFreeList). Of course then incremental came along and we fire barriers on half initialized objects and GGC came along and made the poison check not work, so that all gets workarounds in various inscrutable forms.

At this point maybe we should try to see if !InFreeList is still too slow? If it is, we could limit it to only check on small heaps or something. Or maybe just drop the check. I don't think it has ever caught a single legitimate problem and we've wasted a ton of time fixing it every time we've changed the GC.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #2)

Thanks for the explanation!  It was the phrasing of MOZ_ASSERT_IF(IsPoisoned, !InFreeList) to mean MOZ_ASSERT(!IsPoisoned || !InFreeList) that confused me.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d7caf4b72c
Fix data races in marking assertions when called as part of compacting GC r=terrence

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51d7caf4b72c