1652133 - ThreadSanitizer: data race [@ servo_arc::Arc drop_slow] vs [@ nsStyleUI::CalcDifference]

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Byron Campen [:bwc]]

Attachment: tsan output

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This (and a bunch of other similar reports that have been filed recently) are wrong. There's synchronization here: https://searchfox.org/mozilla-central/rev/622dbd3409610ad3f71b56c9a6a92da905dab0aa/servo/components/servo_arc/lib.rs#542

Is Rust code properly instrumented? Is this a TSAN bug?

Comment 2

[Byron Campen [:bwc]]

This may be a known limitation of TSan (it does not support fences). decoder?

Comment 3

[Christian Holler (:decoder)]

This is a known problem as ThreadSanitizer does not support memory fences (which Arc uses).

We already have these suppressions in place:

         // Bug 1590423 - permanent
         "race:sync..Arc\n"
         "race:alloc::sync::Arc\n"

and probably need to add servo_arc::Arc there to suppress these false positives. I will make a patch for this.

There has been some discussion in Rust (can't find the issue# right now) to replace the memory fence in Arc with regular atomic synchronization only when building with TSan, which would solve this problem properly.

Byron, can you dupe all bugs involving servo_arc::Arc to this one? Thanks!

Comment 4

[Christian Holler (:decoder)]

Hm, I overlooked that servo_arc is actually part of our Servo implementation.

Emilio, since this seems to be a fork of std::sync::Arc, does this servo_arc implementation also use memory fences for synchronization? I see the use of atomics in the code, but that should be supported.

If there are only atomics used in that implementation and no fences, then either the code is not instrumented for some reason (which would be a bug) or this is a real race.

Comment 5

[Christian Holler (:decoder)]

Also, Byron, you mentioned that this is with your own builds, did you ever see any of these races with our builds?

If not, can you please provide your exact mozconfig and rust compiler version? It might well be, that your build is missing instrumentation for Rust code, which would explain this.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/layout/style/ServoStyleConstsInlines.h#166-173 can use fences, but not when MOZ_TSAN is enabled.

Otherwise there's only atomics. We use fences in some other places like this but I don't think that is involved in this report.

Comment 7

[Byron Campen [:bwc]]

Attachment: .mozconfig

Mozconfig is based on the documentation here:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Thread_Sanitizer

rustc version is:

rustc 1.44.1 (c7087fe00 2020-06-17)

clang version is:

clang version 10.0.0 (Fedora 10.0.0-2.fc32)

Comment 8

[Christian Holler (:decoder)]

(In reply to Byron Campen [:bwc] from comment #7)

Created attachment 9163220 [details]
.mozconfig

Mozconfig is based on the documentation here:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Thread_Sanitizer

This documentation is not up-to-date (and I am planning to update it when moving it firefox-source-docs).

If you want to build locally, you need to add the appropriate flags for building the Rust code with TSan as well, e.g. found in https://searchfox.org/mozilla-central/source/browser/config/mozconfigs/linux64/tsan

# Ensure Rust also gets the necessary instrumentation
export RUSTFLAGS="-Zsanitizer=thread"

# rustfmt is currently missing in Rust nightly
unset RUSTFMT

# Current Rust Nightly has warnings
ac_add_options --disable-warnings-as-errors

A Rust Nightly version is necessary for this.