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

Issue metadata

Status: Fixed
Owner: ----
Closed: Jun 21
Cc:
Components:
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

tsan2 found races in webrtc/base (Thread::Clear, MessageQueueManager::Clear)

Reported by henrike@webrtc.org, Oct 9 2014

Issue description

See https://webrtc-codereview.appspot.com/28689004/diff/40001/webrtc/build/tsan_suppressions_webrtc.cc for suppressions.

Repo: remove suppression and run rtc_unittest under tsan2 (linux).
 

Comment 1 by vrk@webrtc.org, Dec 17 2014

Labels: Area-Video
Project Member

Comment 2 by pbos@webrtc.org, Dec 17 2014

Labels: -Area-Video Area-Internals

Comment 3 by henrike@webrtc.org, Feb 3 2015

Owner: juberti@webrtc.org
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 27 2015

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

commit 1d36003181fed7a23537ebdeaca74ef9f50bed8f
Author: Henrik Kjellander <kjellander@webrtc.org>
Date: Fri Mar 27 12:46:34 2015

Suppress TSan errors triggered when deadlock detection is enabled.

These are problematic when running with the default TSan
settings which has deadlock detection enabled.
Our bots still run with it disabled but we want to be
able to turn it back on, thus this is needed.

BUG= 3911 ,4456
TESTED=
Successfully executed:
GYP_DEFINES="tsan=1 release_extra_cflags=-g use_allocator=none" webrtc/build/gyp_webrtc
ninja -C out/Release rtc_unittests
out/Release/rtc_unittests

R=pbos@webrtc.org

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

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

[modify] http://crrev.com/1d36003181fed7a23537ebdeaca74ef9f50bed8f/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc

Project Member

Comment 5 by anatolid@webrtc.org, Nov 3 2016

Cc: kjellander@webrtc.org
This bus has not been modified for more than a year. Is this still a valid issue?
Project Member

Comment 6 by kjellander@webrtc.org, Nov 7 2016

Owner: ----
Status: Available (was: Assigned)
I believe so, the suppressions are still there: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/sanitizers/tsan_suppressions_webrtc.cc#43

I don't think it makes sense for juberti to own the bug though.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 24 2017

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

commit 162cb53e7b6b892ef5201c5ce242520694bab9c1
Author: deadbeef <deadbeef@webrtc.org>
Date: Fri Feb 24 01:10:07 2017

Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG= webrtc:3914 ,  webrtc:3911 

Review-Url: https://codereview.webrtc.org/2694723004
Cr-Commit-Position: refs/heads/master@{#16811}

[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/tools-webrtc/sanitizers/tsan_suppressions_webrtc.cc
[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/webrtc/base/asyncinvoker-inl.h
[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/webrtc/base/asyncinvoker.cc
[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/webrtc/base/asyncinvoker.h
[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/webrtc/base/event.h
[modify] https://crrev.com/162cb53e7b6b892ef5201c5ce242520694bab9c1/webrtc/base/thread_unittest.cc

Project Member

Comment 8 by deadbeef@webrtc.org, Apr 6 2017

Summary: tsan2 found races in webrtc/base (Thread::Clear, MessageQueueManager::Clear) (was: tsan2 found races in webrtc/base)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 5

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/08672605906581c818ae428d9f2a2de489278d34

commit 08672605906581c818ae428d9f2a2de489278d34
Author: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Mon Mar 05 20:09:20 2018

Fixing data race on vptr of Thread subclasses.

Thread's constructor calls DoInit, which registers itself with the
MessageQueueManager. This could result in the vptr being read before
the subclass has had a chance to modify it (for example, if another
thread happens to call MessageQueueManager::Clear at the right time).

This is exactly why there's a "DoInit" method, which is intended to be
called by the fully instantiated subclass. This was being done between
MessageQueue/Thread, but not between Thread and its subclasses.

Bug:  webrtc:3911 
Change-Id: I94d8855da56d9aaf22470ddca12d0b1dd5de249d
Reviewed-on: https://webrtc-review.googlesource.com/59466
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22297}
[modify] https://crrev.com/08672605906581c818ae428d9f2a2de489278d34/rtc_base/signalthread.cc
[modify] https://crrev.com/08672605906581c818ae428d9f2a2de489278d34/rtc_base/signalthread.h
[modify] https://crrev.com/08672605906581c818ae428d9f2a2de489278d34/rtc_base/thread.cc
[modify] https://crrev.com/08672605906581c818ae428d9f2a2de489278d34/rtc_base/thread.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 20

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/3d8b171fe35d06df4a5b388730af3464a08831de

commit 3d8b171fe35d06df4a5b388730af3464a08831de
Author: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Wed Jun 20 19:42:48 2018

Removing some TSan suppressions around Thread class.

Specifically, removing suppressions for:

race:rtc::MessageQueueManager::Clear
race:rtc::Thread::Clear
deadlock:rtc::MessageQueueManager::Clear
deadlock:rtc::MessageQueueManager::ClearInternal

These issues have hopefully been fixed by this and other CLs:
https://webrtc-review.googlesource.com/c/src/+/59466

NOTRY=True

Bug:  webrtc:3911 , webrtc:4456
Change-Id: I12ce9df0d74381cce4a05e69382029d7fabe2c42
Reviewed-on: https://webrtc-review.googlesource.com/59840
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23689}
[modify] https://crrev.com/3d8b171fe35d06df4a5b388730af3464a08831de/tools_webrtc/sanitizers/tsan_suppressions_webrtc.cc

Project Member

Comment 11 by deadbeef@chromium.org, Jun 21

Status: Fixed (was: Available)

Sign in to add a comment