1674773 - ThreadSanitizer: data race [@ moz_task::TaskRunnable::Release] vs. [@ nsThreadPool::Run]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace-nov-2020.txt

I am seeing this in my WIP branch to properly instrument the rust stdlib.

The race involves this task and this runnable. It's possible this is just a non-atomic refcounting issue somewhere, but I'm not confident. Things are a bit too virtual/macro-y to have a clear picture.

Marked as security since this is ostensibly a potential Use-After-Free involving a lot of virtual methods.

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

[Nika Layzell [:nika] (ni? for response)]

I remember us discussing a while ago that tsan doesn't handle atomic fences super well. I wonder if the cause of this issue is actually the Acquire fence in the xpcom atomic refcounting code here: https://searchfox.org/mozilla-central/rev/16d30bafd4e5276d6d3c632fb52a6c71e739cc44/xpcom/rust/xpcom/src/refptr.rs#301-302

Our C++ implementation of atomic release actually has config options to perform a less efficient operation because tsan doesn't understand the fence operation: https://searchfox.org/mozilla-central/rev/16d30bafd4e5276d6d3c632fb52a6c71e739cc44/xpcom/base/nsISupportsImpl.h#363-374. As does the rust standard library: https://github.com/rust-lang/rust/blob/338f939a8d77061896cd0a1ca87a2c6d1f4ec359/library/alloc/src/sync.rs#L41-L56.

I think the fix to this issue might just be to perform a similar check within refptr.rs.

Comment 2

[Christian Holler (:decoder)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #1)

I think the fix to this issue might just be to perform a similar check within refptr.rs.

Thanks a lot Nika, I believe this is exactly what we have been looking for!

Alexis, do you feel comfortable for attempting a fix here?

Comment 3

[Aria Beingessner [:Gankra]]

Yeah should be fine,

Comment 4

[Andrew McCreight [:mccr8]]

This is TSan not understanding our synchronization, apparently, so I'm unhiding.

Comment 5

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674773 - Use the dummy-load-as-a-fence trick for refptr.rs. r?nika

Comment 6

[Aria Beingessner [:Gankra]]

Oh boy, the cfg(sanitize="thread") check is behind an unstable feature(cfg_sanitize), time to do some messy nonsense.

Comment 7

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674773 - Add an --enable-rust-thread-sanitizer flag. r?rstewart

This creates a more principled way for us to set all the things our rust
code needs for tsan. In the future, this will also be used to set -Zbuild-std
and the infra it needs. In this patch set, it's needed to set a feature
flag on the xpcom crate.

Comment 8

[Aria Beingessner [:Gankra]]

Two green (for this issue) try builds, one with just these changes, and one on top of my rust-vendor branch:

just these changes: https://treeherder.mozilla.org/jobs?repo=try&revision=df2ceb83144c273a50f532aa8ab978b1a173a5e4&selectedTaskRun=flIdSdthQ5C3GjrwH2SjXA.0

with instrumented std: https://treeherder.mozilla.org/jobs?repo=try&revision=aa595b5b0e89ce0c6ab3c6049aa58268264e88ac&selectedTaskRun=TD3h7w79RcqqVniM-C6zFA.0

Comment 9

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674773 - make tsan builds use consistent flags. r?rstewart

This makes --enable-thread-sanitizer turn on Rust tsan (-Zsanitizer=thread).
This requires changing SpiderMonkey tsan to use the tsan rust nightly.

In future changes, more Rust tsan integration will key off of MOZ_TSAN.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89babec1b885
make tsan builds use consistent flags. r=rstewart
https://hg.mozilla.org/integration/autoland/rev/acd441508a66
Use the dummy-load-as-a-fence trick for refptr.rs. r=nika

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65bcbd8c456a
Backed out 2 changesets for turning Bug 1646925 into almost permafail.

Comment 12

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&revision=acd441508a66de3244ed3a2f6cde09f6a0a95f33&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedTaskRun=VW9bAqkWRASq52DD1aYjew.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=321277563&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/65bcbd8c456a4b7e8ca78bd06cf22a6023c8cd23

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5942060b993b
Backed out 2 changesets for turning Bug 1646925 into almost permafail.

Comment 14

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674773 - Disable a few more tests under tsan. r?decoder

These already-flakey tests seem to get pushed over the edge to "almost always
busted" with more instrumented Rust code. Disabling seems like the right approach
to get more coverage elsewhere (and the other similarly-flakey tests suggest
we're probably still hitting the root cause).

Depends on D95950

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/378d2e32026b
make tsan builds use consistent flags. r=rstewart
https://hg.mozilla.org/integration/autoland/rev/342247a735ac
Use the dummy-load-as-a-fence trick for refptr.rs. r=nika
https://hg.mozilla.org/integration/autoland/rev/0644208f1414
Disable a few more tests under tsan. r=decoder

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/378d2e32026b
https://hg.mozilla.org/mozilla-central/rev/342247a735ac
https://hg.mozilla.org/mozilla-central/rev/0644208f1414