TSan: Thread data race in js::gc::ArenaLists::allocateFromArena(JSCompartment*, js::gc::AllocKind) vs. js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::ArenaHeader*, bool)

RESOLVED FIXED

Status

()

Core
General
--
critical
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: posidron, Assigned: decoder)

Tracking

(Blocks: 1 bug, {leave-open, sec-want})

Trunk
x86_64
Linux
leave-open, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tsan][tsan-test-blocker])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 717801 [details]
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
(Reporter)

Updated

5 years ago
Summary: TSan: Thread data race in js::gc::ArenaLists::allocateFromArena(JSCompartment*, js::gc::AllocKind) → TSan: Thread data race in js::gc::ArenaLists::allocateFromArena(JSCompartment*, js::gc::AllocKind) vs. js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::ArenaHeader*, bool)
(Assignee)

Comment 1

5 years ago
Ccing bhackett and billm for some help here :)
This is probably ArenaLists::backgroundFinalizeState, which is accessed with the GC lock held in backgroundFinalize and without in allocateFromArena - it's volatile and care is taken to ensure the main thread see changes from the background thread if necessary.  So I think this is ok.
(Assignee)

Comment 3

5 years ago
Same as for bug 844755 (comment 6), will put these on ignore when bug 847350 is through.
Assignee: nobody → choller
Depends on: 847350
Created attachment 823252 [details] [diff] [review]
Bug844759-backgroundFinalizeState

Here's a patch for this one to use atomics rather than volatile for data shared between threads.
Decoder found that there is still a race reported with this patch applied.  Edited IRC transcript:

11:38	decoder	there is also one more, with maybegc on the stack, and it shows a load vs. a store from Atomics. i am not sure yet whats going on, because our atomics are compiler builtins recognized by tsan. maybe in this particular case something phishy i going on
11:38	decoder	ill try to figure it out
11:41	decoder	jonco: https://pastebin.mozilla.org/3366630
11:43	decoder	jonco: thats with your patches applied btw
11:44	decoder	jsgc.cpp line 2187 is: ArenaLists::backgroundFinalize(&fop, arenas, onBackgroundThread);
11:44	decoder	and 1546 is: void *thing = cx->allocator()->arenas.allocateFromArenaInline(zone, thingKind);
11:45	jonco	decoder: so it looks like the patch for 844759 didn't work
11:45	jonco	decoder: I think it's that issue
11:46	decoder	jonco: you mean the signature changed from that patch?
11:46	decoder	i can try to confirm by backing that one out locally
11:59	decoder	jonco: okay. without the patch im seeing two signatures there. the one you fixed with the patch and one load/store: https://pastebin.mozilla.org/3366716
12:00	decoder	your patch certainly fixed one of these
12:00	decoder	but the other one was still there or morphed into something else
12:00	decoder	there could be two problems here, or the fix is incomplete
(Assignee)

Updated

4 years ago
Blocks: 929478
Created attachment 8421032 [details] [diff] [review]
bug844759-atomic-bfs

Even if this doesn't exactly fix the TASN errors, use of atomics is preferable to volatile.
Attachment #823252 - Attachment is obsolete: true
Attachment #8421032 - Flags: review?(terrence)

Updated

4 years ago
Keywords: leave-open
Comment on attachment 8421032 [details] [diff] [review]
bug844759-atomic-bfs

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

r=me. Sadly, |volatile| is more a signal to the programmer that the memory is being used for synchronization than it is to the compiler.
Attachment #8421032 - Flags: review?(terrence) → review+
decoder, are you still seeing TSAN warnings relating to this?
Flags: needinfo?(choller)
(Assignee)

Comment 11

2 years ago
Clearing old needinfo as I'm not working on TSan anymore.

Nathan, since you've been running TSan in the past, do you still encounter this signature/issue?
Flags: needinfo?(choller) → needinfo?(nfroyd)
(In reply to Christian Holler (:decoder) from comment #11)
> Clearing old needinfo as I'm not working on TSan anymore.
> 
> Nathan, since you've been running TSan in the past, do you still encounter
> this signature/issue?

I haven't seen anything like this.  I think this has been open long enough that a new bug is probably more appropriate in any event.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(nfroyd)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.