1601286 - ThreadSanitizer: data race [@ flagsField] vs. [@ setFlagBit]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The attached crash information was detected while running CI tests with ThreadSanitizer on mozilla-central revision 66531295716a.

Filing because this is happening quite frequently and inside the JS engine where we currently expect no races to happen. Also seeing a variation of this with lengthField instead of flagsField involved.

General information about TSan reports

Why fix races?

Data races are undefined behavior and can cause crashes as well as correctness issues. Compiler optimizations can cause racy code to have unpredictable and hard-to-reproduce behavior.

Rating

If you think this race can cause crashes or correctness issues, it would be great to rate the bug appropriately as P1/P2 and/or indicating this in the bug. This makes it a lot easier for us to assess the actual impact that these reports make and if they are helpful to you.

False Positives / Benign Races

Typically, races reported by TSan are not false positives [1], but it is possible that the race is benign. Even in this case it would be nice to come up with a fix if it is easily doable and does not regress performance. Every race that we cannot fix will have to remain on the suppression list and slows down the overall TSan performance. Also note that seemingly benign races can possibly be harmful (also depending on the compiler, optimizations and the architecture) [2][3].

[1] One major exception is the involvement of uninstrumented code from third-party libraries.
[2] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[3] How to miscompile programs with "benign" data races: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Suppressing unfixable races

If the bug cannot be fixed, then a runtime suppression needs to be added in mozglue/build/TsanOptions.cpp. The suppressions match on the full stack, so it should be picked such that it is unique to this particular race. The bug number of this bug should also be included so we have some documentation on why this suppression was added.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Jon Coppeard (:jonco)]

This is not GC related. We're racing on atom flags between pinning on the main thread and XDR decode on a helper thread.

Comment 3

[Jason Orendorff [:jorendorff]]

Ted, I'm guessing this is known, it's mostly harmless, and Stencil will fix it. Would you please make this depend on something in the bug 1601332 tree?

Comment 4

[Christian Holler (:decoder)]

Side note: This is hiding at least one other (similarly looking) race, that does not involve XDR decoding, but is suppressed by the same rules. I can try to remove some of the suppressions if you think that this type of race ([@ flagField] vs. [@ setFlagBit]) could have less harmless variants.

Comment 5

[Aria Beingessner [:Gankra]]

Attachment: Bug 1601286 - Remove supression for seemingly fixed issue. r?decoder

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/481bd6d0b255
Remove supression for seemingly fixed issue. r=decoder

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/481bd6d0b255