Closed Bug 1207232 Opened 9 years ago Closed 9 years ago

TSan: debug only race conditions related to locks owners

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

Attachments

(2 files)

Attached file report
On inbound, debug build of the JS shell under TSan shows a lot of race conditions, which all seem to be expected.

- the debug-only lockOwner variable in the GlobalHelperThreadState is read and written in parallel, to assert a thread has ownership of a given lock.
- there's a lockOwner in GCRuntime.h as well, which seems to serve the same purpose
- in the shell only inline OOMThreadCheck function, there's also a benign data race: only one thread can be the target of OOMs, iirc, so it's fine to have one thread which writes the value (probably the main thread?) and several who read it later.
Attached patch lockOwner.patchSplinter Review
This fixes the two lockOwner races. I took advantage of this bug for making one of the lockOwner a DebugOnly class. Unfortunately, compilers can't seem to infer that DebugOnly<Atomic<T>> can be coerced into a T, so we have to explicitly extract the value from the DebugOnly struct. Moreover, the "value" field is set only in debug builds, which implies more #ifdef DEBUG code. Bleh.

The default memory ordering (consistent and sequential) seems to be the right one here (and the safest), as reading and writing needs to be consistent with locking.

The last race condition is more subtle and needs proper fixing as Waldo said on IRC, so letting it as is, at the moment. I was mistaken in comment 0 and thought this race was due to the target thread uint32_t, but it's actually about OOM_Counter (which makes more sense). For what it's worth, just making OOM_Counter an Atomic had TSan not complain about it anymore, which is nice already.
Attachment #8664351 - Flags: review?(terrence)
Comment on attachment 8664351 [details] [diff] [review]
lockOwner.patch

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

Thanks for fixing this!

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Created attachment 8664351 [details] [diff] [review]
> lockOwner.patch
>
> The default memory ordering (consistent and sequential) seems to be the
> right one here (and the safest), as reading and writing needs to be
> consistent with locking.

I concur.

> The last race condition is more subtle and needs proper fixing as Waldo said
> on IRC, so letting it as is, at the moment. I was mistaken in comment 0 and
> thought this race was due to the target thread uint32_t, but it's actually
> about OOM_Counter (which makes more sense). For what it's worth, just making
> OOM_Counter an Atomic had TSan not complain about it anymore, which is nice
> already.

Great! If you have a patch, I'd be happy to review.
Attachment #8664351 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
> > The last race condition is more subtle and needs proper fixing as Waldo said
> > on IRC, so letting it as is, at the moment. I was mistaken in comment 0 and
> > thought this race was due to the target thread uint32_t, but it's actually
> > about OOM_Counter (which makes more sense). For what it's worth, just making
> > OOM_Counter an Atomic had TSan not complain about it anymore, which is nice
> > already.
> 
> Great! If you have a patch, I'd be happy to review.

Thanks for the review! I won't submit the patch for the other race condition about OOM_Counter, quoting irc:

19:18 	<bbouvier>	Waldo: so using an Atomic around that value actually make TSan not care about it, does it make sense?
19:19 	<jonco>	bbouvier, Waldo: my plan with this is actually to stop it being accessed from more than one thread
19:19 	<Waldo>	bbouvier: that might make TSan happy, but it wouldn't fix the logic problems of depending on the value being semi-consistent
Even better then!
https://hg.mozilla.org/mozilla-central/rev/36d363bb1d73
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: