New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 2999 link

Starred by 14 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Aug 24
Cc:
Components:
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Sign in to add a comment

Fix lock-order inversions reported by thread-sanitizer

Project Member Reported by pbos@webrtc.org, Mar 5 2014

Issue description

Thread-sanitizer has an experimental deadlock detector, use it and fix reported lock inversions to avoid deadlocks: https://code.google.com/p/thread-sanitizer/wiki/DeadlockDetector
 
Project Member

Comment 1 by pbos@webrtc.org, Mar 5 2014

Blockedon: webrtc:3001
Project Member

Comment 2 by pbos@webrtc.org, Mar 5 2014

Blockedon: webrtc:3002
Project Member

Comment 3 by pbos@webrtc.org, Mar 5 2014

Blockedon: webrtc:3003
Project Member

Comment 4 by pbos@webrtc.org, Mar 25 2014

Cc: glider@chromium.org dvyukov@chromium.org
Project Member

Comment 5 by pbos@webrtc.org, Mar 25 2014

Cc: kcc@chromium.org
FYI I'll start pinging the bugs without replies after the M35 cut on Friday.
Project Member

Comment 6 by pbos@webrtc.org, May 19 2014

Blockedon: webrtc:3230
Blocking: chromium:372754
Project Member

Comment 8 by glider@chromium.org, May 19 2014

I'm enabling detect_deadlocks in TSan via the default options (base/debug/sanitizer_options.cc, https://codereview.chromium.org/272763005)
This shouldn't directly affect WebRTC, but please LMK if it does.
Project Member

Comment 9 by pbos@webrtc.org, Jun 22 2014

Blocking: webrtc:3509
Project Member

Comment 10 by pbos@webrtc.org, Jul 2 2014

Cc: pbos@webrtc.org
 Issue 3509  has been merged into this issue.
Project Member

Comment 11 by mflodman@webrtc.org, Sep 30 2014

Labels: Area-Video
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 7 2014

The following revision refers to this bug:
  http://code.google.com/p/webrtc/source/detail?r=7386

------------------------------------------------------------------
r7386 | pbos@webrtc.org | 2014-10-07T14:27:27.932795Z

Changed paths:
   M http://code.google.com/p/webrtc/source/diff?path=/trunk/talk/media/webrtc/webrtcvideoengine2.cc&spec=svn7386&r_previous=7385&r=7386&format=side
   M http://code.google.com/p/webrtc/source/diff?path=/trunk/webrtc/build/tsan_suppressions_webrtc.cc&spec=svn7386&r_previous=7385&r=7386&format=side

Remove potential deadlock in WebRtcVideoEngine2.

Fixes lock-order inversions between capturer's SignalVideoFrame and
WebRtcVideoSendStream. Additionally also removes all deadlock
suppressions for WebRtcVideoEngine2.

R=stefan@webrtc.org
TBR=kjellander@webrtc.org
BUG= 1788 , 2999 

Review URL: https://webrtc-codereview.appspot.com/26729004
-----------------------------------------------------------------
Project Member

Comment 13 by pbos@webrtc.org, Dec 1 2014

Cc: sprang@webrtc.org mflodman@webrtc.org
 Issue 2959  has been merged into this issue.
Project Member

Comment 14 by mflodman@webrtc.org, Dec 10 2014

Labels: EngTriaged Mstone-42
Peter,
What is the status of this bug?

Setting M42, same as for 3001 and 3002 .
Project Member

Comment 15 by mflodman@webrtc.org, Dec 10 2014

And ignore my question about status...
Project Member

Comment 16 by pbos@webrtc.org, Feb 19 2015

Labels: -Mstone-42 Mstone-43
Can hope for 43, it's not impossible, but don't hold your breath. :)
Project Member

Comment 17 by vikasmarwaha@webrtc.org, Mar 31 2015

Did this make for M43?
Project Member

Comment 18 by mflodman@webrtc.org, Apr 1 2015

I have one more I'm trying hard to fix before the cut.

Peter,
Your status?
Project Member

Comment 19 by pbos@webrtc.org, Apr 1 2015

Labels: -Mstone-43 Mstone-45
We've quite a few supressions still, removing them should be easier with the new API, I don't think we'll make M43. Let's try for M45 for at least the webrtc:: ones.

Current state of suppressions (I don't know whether all trigger or if some are resolved already):

"deadlock:rtc::MessageQueueManager::Clear\n"
"deadlock:rtc::MessageQueueManager::ClearInternal\n"
"deadlock:webrtc::RTCPReceiver::SetSsrcs\n"
"deadlock:webrtc::test::UdpSocketManagerPosixImpl::RemoveSocket\n"
"deadlock:webrtc::vcm::VideoReceiver::RegisterPacketRequestCallback\n"
"deadlock:webrtc::ViECaptureImpl::ConnectCaptureDevice\n"
"deadlock:webrtc::ViEChannel::StartSend\n"
"deadlock:webrtc::ViECodecImpl::GetSendSideDelay\n"
"deadlock:webrtc::ViEEncoder::OnLocalSsrcChanged\n"
"deadlock:webrtc::ViESender::RegisterSendTransport\n"

Project Member

Comment 20 by pbos@webrtc.org, Apr 10 2015

Blockedon: webrtc:4534
Project Member

Comment 21 by pbos@webrtc.org, Apr 10 2015

Blockedon: webrtc:4535
Project Member

Comment 22 by pbos@webrtc.org, Apr 15 2015

Blockedon: webrtc:4552
Project Member

Comment 23 by pbos@webrtc.org, Apr 16 2015

Blocking: webrtc:4542
Project Member

Comment 24 by pbos@webrtc.org, Apr 16 2015

Blocking: -webrtc:4542
Project Member

Comment 25 by pbos@webrtc.org, Apr 16 2015

Blockedon: webrtc:4542
Project Member

Comment 26 by bugdroid1@chromium.org, May 28 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/2251d6e17438e1a085ff4f88ad19de513214bec1

commit 2251d6e17438e1a085ff4f88ad19de513214bec1
Author: Peter Boström <pbos@webrtc.org>
Date: Thu May 28 12:10:39 2015

Remove ViESender.

Registers transport on construction removing the need for ViESender as a
hop and removing a potential deadlock by removing RegisterSendTransport.

BUG= 1695 ,  2999 
R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/57449004

Cr-Commit-Position: refs/heads/master@{#9309}

[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/tools/valgrind-webrtc/memcheck/suppressions.txt
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video/call.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video/video_receive_stream.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video/video_send_stream.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/BUILD.gn
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/video_engine_core.gypi
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/vie_channel.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/vie_channel.h
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/vie_channel_group.cc
[modify] http://crrev.com/2251d6e17438e1a085ff4f88ad19de513214bec1/webrtc/video_engine/vie_channel_group.h
[delete] http://crrev.com/259bd2034c3d3ee7f2dc4d481e9bf896a3a4d6ef/webrtc/video_engine/vie_sender.cc
[delete] http://crrev.com/259bd2034c3d3ee7f2dc4d481e9bf896a3a4d6ef/webrtc/video_engine/vie_sender.h

Project Member

Comment 28 by bugdroid1@chromium.org, Jun 26 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/4b91bd08979fcfb191cdae27ad24936beefce735

commit 4b91bd08979fcfb191cdae27ad24936beefce735
Author: Peter Boström <pbos@webrtc.org>
Date: Fri Jun 26 04:58:16 2015

Move frame input (ViECapturer) to webrtc/video/.

Renames ViECapturer to VideoCaptureInput and initializes several
parameters on construction instead of setters.

Also removes an old deadlock suppression.

BUG= 1695 ,  2999 
R=asapersson@webrtc.org, mflodman@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/53559004.

Cr-Commit-Position: refs/heads/master@{#9508}

[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/talk/media/webrtc/fakewebrtccall.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/talk/media/webrtc/fakewebrtccall.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/talk/media/webrtc/fakewebrtcvideoengine.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/tools/valgrind-webrtc/memcheck/suppressions.txt
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/frame_generator_capturer.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/frame_generator_capturer.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/vcm_capturer.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/vcm_capturer.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/video_capturer.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/test/video_capturer.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/BUILD.gn
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/full_stack.cc
[rename] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/video_capture_input.cc
[rename] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/video_capture_input.h
[rename] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/video_capture_input_unittest.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/video_send_stream.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/video_send_stream.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video/webrtc_video.gypi
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/overuse_frame_detector.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/overuse_frame_detector.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/overuse_frame_detector_unittest.cc
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/video_engine_core_unittests.gyp
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/vie_defines.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_engine/vie_encoder.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/video_send_stream.h
[modify] http://crrev.com/4b91bd08979fcfb191cdae27ad24936beefce735/webrtc/webrtc_tests.gypi

Project Member

Comment 29 by pbos@webrtc.org, Jul 3 2015

Blockedon: webrtc:4456
Project Member

Comment 30 by pbos@webrtc.org, Sep 4 2015

Blockedon: webrtc:4973
Project Member

Comment 31 by bugdroid1@chromium.org, Sep 23 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/7cf04452622e8622151d8ab13a43daa437bb8ee1

commit 7cf04452622e8622151d8ab13a43daa437bb8ee1
Author: Peter Boström <pbos@webrtc.org>
Date: Wed Sep 23 15:04:00 2015

Remove ViEChannel::StartSend deadlock suppression.

No longer lock-order inverting since RTP/RTCP modules are instantiated
on construction and no longer guarded by a separate lock.

BUG= webrtc:2999 
R=stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1347283004 .

Cr-Commit-Position: refs/heads/master@{#10034}

[modify] http://crrev.com/7cf04452622e8622151d8ab13a43daa437bb8ee1/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc

Project Member

Comment 32 by mflodman@webrtc.org, Dec 10 2015

Blocking: -chromium:372754
What's left in this bug Peter?

I'm not allowed to keep chromium:372754 in the blocking field for some reason.
Project Member

Comment 33 by kjellander@webrtc.org, Dec 17 2015

Re #32: I think cross-project blocked on/blocking references are not possible until Chromium has migrated to Monorail, unfortunately. 
Project Member

Comment 34 by pbos@webrtc.org, Dec 17 2015

Five-six suppressions:

$ git grep "deadlock:"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:62:"deadlock:rtc::MessageQueueManager::Clear\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:63:"deadlock:rtc::MessageQueueManager::ClearInternal\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:80:"deadlock:webrtc::RTCPReceiver::SetSsrcs\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:81:"deadlock:webrtc::test::UdpSocketManagerPosixImpl::RemoveSocket\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:82:"deadlock:webrtc::vcm::VideoReceiver::RegisterPacketRequestCallback\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:83:"deadlock:webrtc::ViEEncoder::OnLocalSsrcChanged\n"
Project Member

Comment 35 by pbos@webrtc.org, Jan 27 2016

Labels: -Mstone-45 Mstone-51
Project Member

Comment 36 by bugdroid1@chromium.org, Apr 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/d486dc1bd7f1e0eff7118f14b22768067dcd9cef

commit d486dc1bd7f1e0eff7118f14b22768067dcd9cef
Author: Peter Boström <pbos@webrtc.org>
Date: Tue Apr 26 11:23:12 2016

Remove some TSan deadlock suppressions.

Looks like these are no longer actively triggering, so they can be removed.

BUG= webrtc:2999 
R=kjellander@webrtc.org

Review URL: https://codereview.webrtc.org/1916923002 .

Cr-Commit-Position: refs/heads/master@{#12502}

[modify] https://crrev.com/d486dc1bd7f1e0eff7118f14b22768067dcd9cef/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc

Project Member

Comment 37 by pbos@webrtc.org, Apr 26 2016

Owner: ----
Status: Available (was: Assigned)
Remaining ones are, for anyone interested in taking over:

pbos@deimos:~/webrtc/src (master)$ git grep deadlock:
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:61:"deadlock:rtc::MessageQueueManager::Clear\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:62:"deadlock:rtc::MessageQueueManager::ClearInternal\n"
webrtc/build/sanitizers/tsan_suppressions_webrtc.cc:79:"deadlock:webrtc::test::UdpSocketManagerPosixImpl::RemoveSocket\n"


The MessageQueueManager ones aren't super obvious, and the latter is test-only. Not intending to do anything to that one right now, so marking as available.
Project Member

Comment 38 by kjellander@webrtc.org, Oct 5 2016

Labels: M-51
Project Member

Comment 39 by anatolid@webrtc.org, Nov 11 2016

Labels: -Mstone-51 -M-51
Removing the (outdated) milestone label since this bug is currently blocked on several open bugs which don't have milestone labels themselves.
Project Member

Comment 40 by stefan@webrtc.org, Apr 28 2017

Components: -Video Internals
Video issues have been resolved. Remaining issues are in base/. Moving to Internals.
Project Member

Comment 41 by stefan@webrtc.org, Apr 28 2017

Components: -Internals Video
Changed the wrong bug. Moving back.
Project Member

Comment 42 by stefan@webrtc.org, Aug 24

Status: Fixed (was: Available)
Marking as fixed for now as the remaining ones doesn't appear to be urgent.

Sign in to add a comment