Closed Bug 1136004 Opened 5 years ago Closed 5 years ago

clang -Wthread-safety-analysis warnings in webrtc: writing variable 'red_enabled_' requires locking 'acm_crit_sect_' exclusively

Categories

(Core :: WebRTC: Audio/Video, defect, P4)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: cpeterson, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

These warnings are from webrtc code that writes to an enum member variable outside a critical section:

> media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:1443:3 [-Wthread-safety-analysis] writing variable 'red_enabled_' requires locking 'acm_crit_sect_' exclusively
> media/webrtc/trunk/webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc:1446:16 [-Wthread-safety-analysis] reading variable 'red_enabled_' requires locking 'acm_crit_sect_'
> media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc:698:5 [-Wthread-safety-analysis] writing variable 'loadstate_' requires locking 'crit_sect_' exclusively

These warnings might fairly be treated as false positives because they point to Lock() and Unlock() member functions that wrap _critSect. However, the Lock() and Unlock() member functions are never actually called, so they can be removed.

> media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:186:5 [-Wthread-safety-analysis] mutex '_critSect' is still locked at the end of function
> media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:190:9 [-Wthread-safety-analysis] unlocking '_critSect' that was not locked
Attachment #8568379 - Flags: review?(cpeterson)
Comment on attachment 8568379 [details] [diff] [review]
fix TSAN in webrtc warning when RED isn't enabled

Review of attachment 8568379 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM for the warnings in audio_coding_module_impl.cc.
Attachment #8568379 - Flags: review?(cpeterson) → review+
btw, I thought all code under media/webrtc/trunk/ was a snapshot of the upstream webrtc code that shouldn't be changed in mozilla-central. Is that still true? Are you making the same fix upstream?
Flags: needinfo?(rjesup)
Keywords: leave-open
It is.  pkerr was intended to help manage getting things upstreamed, but (partly due to loop/hello, which started the same time he did) that didn't happen.

We haven't been great at linking things to bug 864513; however part of what we'll do eventually is view the diffs against upstream (which is easy because of how I land updates), then walk back through the history to find where our change landed, and then use that to craft an upstream issue/patch.  Or in some cases, just roll up some set of changes into a new bug.
Flags: needinfo?(rjesup)
Keywords: leave-open
Flags: firefox-backlog+
Priority: -- → P4
https://hg.mozilla.org/mozilla-central/rev/60ac868f9918
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
This patch fixes the remaining -Wthread-safety-analysis warnings reported in comment 0. These are the last three -Wthread-safety-analysis compiler warnings (not TSan run-time warnings) in mozilla-central.

AudioDeviceMac's private member functions Lock(), Unlock(), and Id() seem to be unused, so I removed them.

> media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc:698:5 [-Wthread-safety-analysis] writing variable 'loadstate_' requires locking 'crit_sect_' exclusively
> media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:186:5 [-Wthread-safety-analysis] mutex '_critSect' is still locked at the end of function
> media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h:190:9 [-Wthread-safety-analysis] unlocking '_critSect' that was not locked
Attachment #8570843 - Flags: review?(rjesup)
Comment on attachment 8570843 [details] [diff] [review]
more-Wthread-safety-analysis-warnings.patch

Review of attachment 8570843 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for media_optimization change
r- of the audio_device_mac.h change unless it causes a problem - we avoid making changes like that just because we don't use it.  If it's really annoying, then put it in an "#ifndef WEBRTC_MOZILLA_BUILD". If it's truly kruft that upstream should have removed *and* is annoying, then ok, remove it.
(In reply to Randell Jesup [:jesup] from comment #8)
> r- of the audio_device_mac.h change unless it causes a problem - we avoid
> making changes like that just because we don't use it.  If it's really
> annoying, then put it in an "#ifndef WEBRTC_MOZILLA_BUILD". If it's truly
> kruft that upstream should have removed *and* is annoying, then ok, remove
> it.

OK. I will submit a patch upstream to remove these functions instead of making more work for you when you next merge upstream. :)

These are the only clang -Wthread-safety-analysis warnings in all C++ code in mozilla-central, so it would be nice to fix or suppress them so we have zero -Wthread-safety-analysis warnings.

Upstream recently went out of their way to suppress these clang warnings with clang-specific annotations, but they didn't see that these member functions are never actually used:

https://code.google.com/p/webrtc/source/detail?r=7961&path=/trunk/webrtc/modules/audio_device/mac/audio_device_mac.h#
I filed an upstream bug about audio_device_mac.h's -Wthread-safety-analysis warnings:

https://code.google.com/p/webrtc/issues/detail?id=4362
Comment on attachment 8570843 [details] [diff] [review]
more-Wthread-safety-analysis-warnings.patch

Review of attachment 8570843 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on media_optimization per previous review
Attachment #8570843 - Flags: review?(rjesup) → review+
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.