844766 - TSan: Thread data race in js::MaybeGC(JSContext*) vs. js::gc::Chunk::releaseArena

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: trace

During Firefox start-up with ThreadSanitizer (LLVM version), we get a data race reported as described in the attached log. Trace was created on mozilla-central with changeset 122820:c233837cce08.

According to the TSan devs, most of the reported traces should be real data races, even though they can be "benign". We need to determine if the race can/should be fixed, or put on the ignore list. Even for benign races, TSan devs suggest to fix them (second priority), as they can also cause problems [1].

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 1

[Christian Holler (:decoder)]

Ccing bhackett and billm for some help here :)

Comment 2

[Andrew McCreight [:mccr8]]

In this bug, bug 844768, and bug 844755 sweeping is involved, so maybe JonCo has some idea what is going on.

Comment 3

[Jon Coppeard (:jonco)]

It looks like Zone::gcBytes which is being updated by the background thread and read by the main thread to determine whether to do a GC.  So a race can change exactly when a GC happens, but I don't think this is something we care about.

Comment 4

[Christian Holler (:decoder)]

(In reply to Jon Coppeard (:jonco) from comment #3)
> It looks like Zone::gcBytes which is being updated by the background thread
> and read by the main thread to determine whether to do a GC.  So a race can
> change exactly when a GC happens, but I don't think this is something we
> care about.

If the race decides when a GC happens, then fixing the race would make test cases possibly more deterministic which could help fuzzing and debugging. I don't know though if fixing the race is possible without any side effects, but if it is, then it would be really nice to have.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Christian Holler (:decoder) from comment #4)

Even if we changed this, the timing of the GC would still depend on thread scheduling of main vs. background thread, so I don't think this would actually make it deterministic.

Comment 6

[Christian Holler (:decoder)]

Thanks, that makes sense :) I'll work on a patch to ignore this function then when bug 847350 is through (see also bug 844755 comment 6).

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug844766-gcBytes

Here's a patch to make gcBytes use atomic operations, rather than volatile.

Can you check to see whether this fixes the TSAN warning?

Comment 8

[Christian Holler (:decoder)]

I finally was able to reproduce the original issue. And I can confirm that the patch fixes it :)

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 821581 [details] [diff] [review]
Bug844766-gcBytes

This is a patch to replace use of volatile with atomic operations for the gcBytes member in JSRuntime and Zone, which is accessed from multiple threads.

The main motivation for this is to stop TSAN generate spurious warnings, but it's also nicer because this makes it explicit that this data is accessed from different threads.

I chose the ReleseAcquire memory ordering because that will lead to more deterministic operation without any real penalty.  In practice, for gcBytes I think Relaxed ordering would work as well.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 821581 [details] [diff] [review]
Bug844766-gcBytes

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

We really need to document these Atomics.h better.

Comment 11

[Jon Coppeard (:jonco)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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