1664536 - ThreadSanitizer: data race [@ memcpy] vs. [@ process] ([@ webrtc::DesktopCaptureImpl::StartCapture] vs. [@ webrtc::DesktopCaptureImpl::Run])

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Christian Holler (:decoder)]

The attached crash information was detected while running CI tests with ThreadSanitizer on try (parent revision is mozilla-central dc90a7a18c07)..

For detailed crash information, see attachment.

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 Bauman [:jbauman:]]

This seems WebRTC-related

Comment 3

[Michael Froman [:mjf]]

Dan, any thoughts on who should look at this?

Comment 4

[Dan Minor [:dminor]]

I'll have a look.

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1664536 - Simplify locking in DesktopCaptureImpl; r=mjf!

We have both _apiCs and _callBackCs in DesktopCaptureImpl, presumably for
historical reasons, since one is protected and the other private. Since there
are no subclasses of DesktopCaptureImpl, one private CriticalSection is
sufficient here.

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1664536 - Add lock to StartCapture; r=mjf

Depends on D90865

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1664536 - Add _maxFPSNeeded member; r=mjf!

If we make _maxFPSNeeded an atomic member variable, then we can avoid taking
a lock in DesktopCaptureImple::process().

Depends on D90866

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1664536 - Call SetPriority after capturer thread started; r=mjf!

Depends on D90868

Comment 9

[Dan Minor [:dminor]]

Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=355e3cab1b03ae32f74f093089db1835f0ebcd4e, also manually tested on Linux and Windows.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/510009497361
Simplify locking in DesktopCaptureImpl; r=mjf
https://hg.mozilla.org/integration/autoland/rev/8d7b14467d80
Add lock to StartCapture; r=mjf
https://hg.mozilla.org/integration/autoland/rev/a2654ba127db
Add _maxFPSNeeded member; r=mjf
https://hg.mozilla.org/integration/autoland/rev/edc31e2eb1ef
Call SetPriority after capturer thread started; r=mjf

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/510009497361
https://hg.mozilla.org/mozilla-central/rev/8d7b14467d80
https://hg.mozilla.org/mozilla-central/rev/a2654ba127db
https://hg.mozilla.org/mozilla-central/rev/edc31e2eb1ef

Comment 12

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

The patch landed in nightly and beta is affected.
:dminor, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Dan Minor [:dminor]]

I'm a bit hesitant to request uplift because the threading priority changes in the last patch in the series could have hard to predict side effects and the rest of the series does not fix a recent regression.