1674770 - ThreadSanitizer: data race [@ StrongRuleNode::ensure_child]

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Aria Beingessner [:Gankra]]

Attachment: tsan-backtrace-nov-2020.txt

I am seeing this in my WIP branch to properly instrument the stdlib. This is likely a false positive, as the access is clearly guarded by a parking_lot RwLock. I am assuming this is TSan failing to instrument or otherwise understand parking_lot.

I have filed an upstream bug: https://github.com/Amanieu/parking_lot/issues/257

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

[Emily McDonough [:alaskanemily]]

Gankra: If this is caused by parking_lot code would it be more appropriate to put it under a different component?

Comment 2

[Aria Beingessner [:Gankra]]

Perhaps? Stylo is the one using parking_lot, so I don't know where else I would hang this.

Comment 3

[Aria Beingessner [:Gankra]]

An upstream fix for this has been proposed: https://github.com/Amanieu/parking_lot/pull/260

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:nordzilla, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674770 - Update parking_lot to get RwLock fix. r?decoder

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88f6cae1d63f
Update parking_lot to get RwLock fix. r=decoder

Comment 7

[Natalia Csoregi [:nataliaCs]]

Backed out for causing build bustage

backout: https://hg.mozilla.org/integration/autoland/rev/6b352a5fd0e48c0f64b36347916a51ab0a771fd6

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=88f6cae1d63fb0836a3c50ea1aa69e716c1b980d

failure log: https://treeherder.mozilla.org/logviewer?job_id=322318088&repo=autoland&lineNumber=2361

[task 2020-11-19T16:57:20.925Z] 16:57:20 INFO - /builds/worker/workspace/obj-build/_virtualenvs/init_py3/bin/python -m mozbuild.action.file_generate /builds/worker/checkouts/gecko/build/RunCbindgen.py generate_metadata config/cbindgen-metadata.json config/.deps/cbindgen-metadata.json.pp config/.deps/cbindgen-metadata.json.stub .cargo/config
[task 2020-11-19T16:57:20.925Z] 16:57:20 INFO - error: failed to select a version for the requirement parking_lot = "=0.11.1"
[task 2020-11-19T16:57:20.925Z] 16:57:20 INFO - candidate versions found which didn't match: 0.11.0
[task 2020-11-19T16:57:20.926Z] 16:57:20 INFO - location searched: directory source /builds/worker/checkouts/gecko/third_party/rust (which is replacing registry https://github.com/rust-lang/crates.io-index)
[task 2020-11-19T16:57:20.926Z] 16:57:20 INFO - required by package wgpu_bindings v0.1.0 (/builds/worker/checkouts/gecko/gfx/wgpu_bindings)
[task 2020-11-19T16:57:20.926Z] 16:57:20 INFO - perhaps a crate was updated and forgotten to be re-vendored?
[task 2020-11-19T16:57:20.926Z] 16:57:20 ERROR - make[3]: *** [backend.mk:123: config/.deps/cbindgen-metadata.json.stub] Error 101
[task 2020-11-19T16:57:20.927Z] 16:57:20 INFO - make[3]: Leaving directory '/builds/worker/workspace/obj-build'

Comment 8

[Aria Beingessner [:Gankra]]

Hmm, interesting, I'm still seeing the issue with the "fixed" parking_lot.

Perhaps the use of rayon nearby is to blame?

Still good to bump this dependency, but maybe we just need to leave the supression.

Comment 9

[Erik Nordin [:nordzilla]]

Since this bug is assigned and actively being worked on, I am going to assign it an S3 severity, which is typical for most bugs.

If you have reason to believe the severity should be different, feel free to modify it further.

Comment 10

[Aria Beingessner [:Gankra]]

Attachment: Bug 1674770 - update parking_lot to get tsan fixes. r?glandium

Although this doesn't fix the particular false positive we hit, more fixes
is good to get

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a0074e53c54
update parking_lot to get tsan fixes. r=glandium

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7a0074e53c54

Comment 13

[Aria Beingessner [:Gankra]]

unassigning myself for now -- this merits more investigation, but for the time being it should be regarded as a permanent false-positive.

Comment 14

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Alexis, what can you say about the regression below being caused by this bug?
== Change summary for alert #28053 (as of Fri, 11 Dec 2020 18:40:02 GMT) ==

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
4%	build times		linux64	plain taskcluster-m5n.4xlarge	1,090.64 -> 1,139.55

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28053

Comment 15

[Aria Beingessner [:Gankra]]

Using slightly stronger atomics making the build 4% slower is a pretty shocking (making me suspicious this PR is to blame), but the merged changes are improving our correctness so I don't think we should revert them.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

:aionescu can you comment on the confidence of the culprit revision given :Gankra's feedback?

Comment 17

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Thanks dave.
I'm not confident at all, the test is build metrics and unstable. The procedure is usually to ask build metrics team.
:mhentges, I did some retriggers on this, I also see an infra change pretty close to the alert so I retriggered additionally 59216bd222. Could you help on identigying the culprit?

Comment 18

[Mitchell Hentges [:mhentges] 🦀]

Hmm, it looks the only code (including dependencies) that should've changed that is associated with :gankra's patch is:

• the Firefox patch itself
• the changes to parking_lot
• the changes to cfg-if

There's a macro change on cfg_if!, but it should make it simpler (and faster!), not slower.
So far, I'm also not confident that this is the patch causing the build time regression.

:alexandrui, I'm not familiar with the perf alert tooling: can you point me towards the metrics for the 59216bd222 that you retriggered?

Comment 19

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Looking at the big picture looks like the is invalid.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:emilio, maybe it's time to close this bug?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Gankra, do you know what should we do with this bug given comment 13?

Comment 22

[Aria Beingessner [:Gankra]]

Let's assume invalid...