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

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8764270 [details]
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.
(Assignee)

Comment 1

2 years ago
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).
Attachment #8764646 - Flags: review?(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.
Attachment #8764646 - Flags: review?(terrence) → review+
(Assignee)

Comment 3

2 years ago
(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

2 years ago
Pushed by jcoppeard@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51d7caf4b72c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.