1670982 - Apply MOZ_ATOMIC_BITFIELDS to remove the cluster of "benign" bitfield TSan supressions

Status: RESOLVED FIXED

(Core :: Sanitizers, task)

Description

[Aria Beingessner [:Gankra]]

Didn't fit under any particular bug, so making a new one to hang the patches off of.

Comment 1

[Aria Beingessner [:Gankra]]

Attachment: Bug 1670982 - Make the bitflags in CacheStorageService actually atomic. r?decoder

TSan found races on these flags that are ostensibly benign but this way there's no UB.

This code is a bit weird. These bitfields are seemingly pointless as
they're squeezed between a uint64_t and an Atomic<bool>. There's plenty
of space for 2 more bools there.

Also the Atomic<bool> could theoretically be merged into the flags. For
now, here's the version of this patch that preserves the semantics of
this code as closely as possible, for review by the code owners.

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1670982 - Make the bitfields in nsHostResolver atomic. r?decoder

TSan found races between mResolveAgain and mGetTtl. This makes them non-UB.

The code is a bit weird. Although the values are typed as uint16_t's, they're
used as bools (even assigned true/false). In addition, the atomic bool mTRRUsed
could be folded into these fields, as there is a spare bit. I decided not to
change these things, as network code can have weird representation requirements
that I'd prefer the owners of the code chime in on first.

Depends on D93416

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: Bug 1670982 - make the bitfields in nsHttpChannel atomic. r?decoder

Weird things about this code I didn't fix:

All the fields are uint32_t, even though most of them are bools.
Some of the bools are set with true/false, others are set with 0/1,
seemingly randomly. The consistent use of uint32_t suggests that it
was expected that all of these values would use 32 bits, but 33
bits are in fact being used (necessitating a uint64_t backing store).

These details were left the same, pending review from the code owners.

A minor correctness caveat to this change:

CacheEntriesToWaitFor modifications aren't modified fully transactionally. We
load it, AND it, and then store it. The load and store are atomic, but the
compare_exchange that performs the store doesn't use the old value from our load.
This means that in theory two racing threads that both try to AND in a value
could still clobber eachother's changes. This was already true, but now it won't
show up in TSan, and won't formally be a datarace. Just a "safe" race condition.

This change assumes the conflicting modifications are to seperate bitfields,
that happen to share storage, and not races on the literal same bitfield. If
that's the case, then the above problem should be fine. (In Bug 1614697 it is
stated that all writes happen on a single thread, so indeed this should be fine).

Depends on D93417

Comment 4

[Aria Beingessner [:Gankra]]

It looks like in the interim month the supressions have allowed new bitfield races to sneak in, now involving nsHTTPBaseChannel. So I think I'll defer the nsHTTPChannel patch to a separate bug while I land the others.

Comment 5

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:Gankra, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7211a44d968
Make the bitfields in nsHostResolver atomic. r=decoder,necko-reviewers,dragana,valentin
https://hg.mozilla.org/integration/autoland/rev/3ce1e8c65cbc
Make the bitflags in CacheStorageService actually atomic. r=decoder,necko-reviewers,dragana,valentin

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (bug 1670982) for tsan xpc shell failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=60d51bd9e518ea6fdd64d5e24cf3554b3d263760&searchStr=linux%2C18.04%2Cx64%2Ctsan%2Copt%2Cxpcshell%2Ctests%2Ctest-linux1804-64-tsan%2Fopt-xpcshell-e10s%2Cx4&selectedTaskRun=NKCF7uRJRQqFxvox9pnP4Q.0&tochange=e00c29720b9f5a4313575a9bfd41cfa802f6e746

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

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b72db9242786
Make the bitfields in nsHostResolver atomic. r=decoder,necko-reviewers,dragana,valentin
https://hg.mozilla.org/integration/autoland/rev/83d921a84082
Make the bitflags in CacheStorageService actually atomic. r=decoder,necko-reviewers,dragana,valentin

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b72db9242786
https://hg.mozilla.org/mozilla-central/rev/83d921a84082