From 3c5d2b43ecf80ec9619c5036938d96ca765fed52 Mon Sep 17 00:00:00 2001 From: "wu@webrtc.org" Date: Fri, 18 Oct 2013 16:27:26 +0000 Subject: [PATCH] Thread::Stop() must be called before any subclass's destructor completes. Update Thread documentation, fix all subclasses that had a problem. This is to avoid a data racing between the destructor modifying the vtable, and Thread::PreRun calling virtual method Run at the same time. For example: [ RUN ] FileMediaEngineTest.TestGetCapabilities ================== WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=2967) Read of size 8 at 0x7d480000bd00 by thread T1: #0 talk_base::Thread::PreRun(void*) /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/base/thread.cc:353 (libjingle_media_unittest+0x000000234da8) Previous write of size 8 at 0x7d480000bd00 by main thread: #0 talk_base::Thread::~Thread() /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/base/thread.cc:158 (libjingle_media_unittest+0x00000023478c) #1 ~RtpSenderReceiver /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/media/base/filemediaengine.cc:122 (libjingle_media_unittest+0x0000001b551f) ... RISK=P2 TESTED=try bots and tsan BUG=2078,2080 R=fischman@webrtc.org Review URL: https://webrtc-codereview.appspot.com/2428004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@4999 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/base/cpumonitor_unittest.cc | 3 +++ talk/base/dbus.cc | 4 ++++ talk/base/logging_unittest.cc | 6 +++++ talk/base/maccocoasocketserver_unittest.mm | 3 +++ talk/base/macsocketserver_unittest.cc | 3 +++ talk/base/signalthread.h | 1 + talk/base/signalthread_unittest.cc | 4 ++++ talk/base/thread.cc | 1 + talk/base/thread.h | 7 ++++++ talk/base/thread_unittest.cc | 3 ++- talk/base/win32socketserver.h | 1 + talk/media/base/filemediaengine.cc | 5 ++++ talk/media/devices/filevideocapturer.cc | 4 ++++ talk/media/devices/gdivideorenderer.cc | 4 ++++ talk/xmllite/xmlelement_unittest.cc | 4 ++++ talk/xmpp/xmppthread.cc | 1 + tools/valgrind-webrtc/tsan/suppressions.txt | 26 --------------------- 17 files changed, 53 insertions(+), 27 deletions(-) diff --git a/talk/base/cpumonitor_unittest.cc b/talk/base/cpumonitor_unittest.cc index 952b89e73a..b9f5ba33e7 100644 --- a/talk/base/cpumonitor_unittest.cc +++ b/talk/base/cpumonitor_unittest.cc @@ -54,6 +54,9 @@ class BusyThread : public talk_base::Thread { BusyThread(double load, double duration, double interval) : load_(load), duration_(duration), interval_(interval) { } + virtual ~BusyThread() { + Stop(); + } void Run() { Timing time; double busy_time = interval_ * load_ / 100.0; diff --git a/talk/base/dbus.cc b/talk/base/dbus.cc index 8e071c7c7a..78e717a646 100644 --- a/talk/base/dbus.cc +++ b/talk/base/dbus.cc @@ -158,6 +158,10 @@ class DBusMonitor::DBusMonitoringThread : public talk_base::Thread { ASSERT(filter_list_); } + virtual ~DBusMonitoringThread() { + Stop(); + } + // Override virtual method of Thread. Context: worker-thread. virtual void Run() { ASSERT(NULL == connection_); diff --git a/talk/base/logging_unittest.cc b/talk/base/logging_unittest.cc index b0c219fa3f..53cab666cc 100644 --- a/talk/base/logging_unittest.cc +++ b/talk/base/logging_unittest.cc @@ -87,6 +87,12 @@ TEST(LogTest, MultipleStreams) { // Ensure we don't crash when adding/removing streams while threads are going. // We should restore the correct global state at the end. class LogThread : public Thread { + public: + virtual ~LogThread() { + Stop(); + } + + private: void Run() { // LS_SENSITIVE to avoid cluttering up any real logging going on LOG(LS_SENSITIVE) << "LOG"; diff --git a/talk/base/maccocoasocketserver_unittest.mm b/talk/base/maccocoasocketserver_unittest.mm index d6f4b2c11c..818c30d860 100644 --- a/talk/base/maccocoasocketserver_unittest.mm +++ b/talk/base/maccocoasocketserver_unittest.mm @@ -36,6 +36,9 @@ class WakeThread : public Thread { public: WakeThread(SocketServer* ss) : ss_(ss) { } + virtual ~WakeThread() { + Stop(); + } void Run() { ss_->WakeUp(); } diff --git a/talk/base/macsocketserver_unittest.cc b/talk/base/macsocketserver_unittest.cc index a4a71019dd..f10aebcde3 100644 --- a/talk/base/macsocketserver_unittest.cc +++ b/talk/base/macsocketserver_unittest.cc @@ -37,6 +37,9 @@ class WakeThread : public Thread { public: WakeThread(SocketServer* ss) : ss_(ss) { } + virtual ~WakeThread() { + Stop(); + } void Run() { ss_->WakeUp(); } diff --git a/talk/base/signalthread.h b/talk/base/signalthread.h index 79c00be312..46dd0a3bad 100644 --- a/talk/base/signalthread.h +++ b/talk/base/signalthread.h @@ -123,6 +123,7 @@ class SignalThread class Worker : public Thread { public: explicit Worker(SignalThread* parent) : parent_(parent) {} + virtual ~Worker() { Stop(); } virtual void Run() { parent_->Run(); } private: diff --git a/talk/base/signalthread_unittest.cc b/talk/base/signalthread_unittest.cc index 4458f52ef2..e5734d4df1 100644 --- a/talk/base/signalthread_unittest.cc +++ b/talk/base/signalthread_unittest.cc @@ -120,6 +120,10 @@ class OwnerThread : public Thread, public sigslot::has_slots<> { has_run_(false) { } + virtual ~OwnerThread() { + Stop(); + } + virtual void Run() { SignalThreadTest::SlowSignalThread* signal_thread = new SignalThreadTest::SlowSignalThread(harness_); diff --git a/talk/base/thread.cc b/talk/base/thread.cc index 316e14b061..d07efb5156 100644 --- a/talk/base/thread.cc +++ b/talk/base/thread.cc @@ -557,6 +557,7 @@ AutoThread::AutoThread(SocketServer* ss) : Thread(ss) { } AutoThread::~AutoThread() { + Stop(); if (ThreadManager::Instance()->CurrentThread() == this) { ThreadManager::Instance()->SetCurrentThread(NULL); } diff --git a/talk/base/thread.h b/talk/base/thread.h index e679ea4572..4dc09f641d 100644 --- a/talk/base/thread.h +++ b/talk/base/thread.h @@ -111,9 +111,15 @@ class Runnable { DISALLOW_COPY_AND_ASSIGN(Runnable); }; +// WARNING! SUBCLASSES MUST CALL Stop() IN THEIR DESTRUCTORS! See ~Thread(). + class Thread : public MessageQueue { public: explicit Thread(SocketServer* ss = NULL); + // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or + // guarantee Stop() is explicitly called before the subclass is destroyed). + // This is required to avoid a data race between the destructor modifying the + // vtable, and the Thread::PreRun calling the virtual method Run(). virtual ~Thread(); static Thread* Current(); @@ -291,6 +297,7 @@ class AutoThread : public Thread { class ComThread : public Thread { public: ComThread() {} + virtual ~ComThread() { Stop(); } protected: virtual void Run(); diff --git a/talk/base/thread_unittest.cc b/talk/base/thread_unittest.cc index 148e27b155..d6af17ac17 100644 --- a/talk/base/thread_unittest.cc +++ b/talk/base/thread_unittest.cc @@ -122,7 +122,7 @@ class MessageClient : public MessageHandler, public TestGenerator { class CustomThread : public talk_base::Thread { public: CustomThread() {} - virtual ~CustomThread() {} + virtual ~CustomThread() { Stop(); } bool Start() { return false; } }; @@ -136,6 +136,7 @@ class SignalWhenDestroyedThread : public Thread { } virtual ~SignalWhenDestroyedThread() { + Stop(); event_->Set(); } diff --git a/talk/base/win32socketserver.h b/talk/base/win32socketserver.h index 1fa65235ea..34598793db 100644 --- a/talk/base/win32socketserver.h +++ b/talk/base/win32socketserver.h @@ -156,6 +156,7 @@ class Win32Thread : public Thread { set_socketserver(&ss_); } virtual ~Win32Thread() { + Stop(); set_socketserver(NULL); } virtual void Run() { diff --git a/talk/media/base/filemediaengine.cc b/talk/media/base/filemediaengine.cc index 1a3547c0ce..e5183f6a95 100644 --- a/talk/media/base/filemediaengine.cc +++ b/talk/media/base/filemediaengine.cc @@ -125,6 +125,7 @@ class RtpSenderReceiver RtpSenderReceiver(MediaChannel* channel, talk_base::StreamInterface* input_file_stream, talk_base::StreamInterface* output_file_stream); + virtual ~RtpSenderReceiver(); // Called by media channel. Context: media channel thread. bool SetSend(bool send); @@ -183,6 +184,10 @@ RtpSenderReceiver::RtpSenderReceiver( } } +RtpSenderReceiver::~RtpSenderReceiver() { + Stop(); +} + bool RtpSenderReceiver::SetSend(bool send) { bool was_sending = sending_; sending_ = send; diff --git a/talk/media/devices/filevideocapturer.cc b/talk/media/devices/filevideocapturer.cc index 174781624b..09318f7a4b 100644 --- a/talk/media/devices/filevideocapturer.cc +++ b/talk/media/devices/filevideocapturer.cc @@ -108,6 +108,10 @@ class FileVideoCapturer::FileReadThread finished_(false) { } + virtual ~FileReadThread() { + Stop(); + } + // Override virtual method of parent Thread. Context: Worker Thread. virtual void Run() { // Read the first frame and start the message pump. The pump runs until diff --git a/talk/media/devices/gdivideorenderer.cc b/talk/media/devices/gdivideorenderer.cc index d5edfc6e43..c8024b75a9 100755 --- a/talk/media/devices/gdivideorenderer.cc +++ b/talk/media/devices/gdivideorenderer.cc @@ -69,6 +69,10 @@ class GdiVideoRenderer::VideoWindow : public talk_base::Win32Window { public: explicit WindowThread(VideoWindow* window) : window_(window) {} + virtual ~WindowThread() { + Stop(); + } + // Override virtual method of talk_base::Thread. Context: worker Thread. virtual void Run() { // Initialize the window diff --git a/talk/xmllite/xmlelement_unittest.cc b/talk/xmllite/xmlelement_unittest.cc index 6d488fa75e..3c31ce491c 100644 --- a/talk/xmllite/xmlelement_unittest.cc +++ b/talk/xmllite/xmlelement_unittest.cc @@ -235,6 +235,10 @@ class XmlElementCreatorThread : public talk_base::Thread { XmlElementCreatorThread(int count, buzz::QName qname) : count_(count), qname_(qname) {} + virtual ~XmlElementCreatorThread() { + Stop(); + } + virtual void Run() { std::vector elems; for (int i = 0; i < count_; i++) { diff --git a/talk/xmpp/xmppthread.cc b/talk/xmpp/xmppthread.cc index 43dded866c..716aaf8363 100644 --- a/talk/xmpp/xmppthread.cc +++ b/talk/xmpp/xmppthread.cc @@ -50,6 +50,7 @@ XmppThread::XmppThread() { } XmppThread::~XmppThread() { + Stop(); delete pump_; } diff --git a/tools/valgrind-webrtc/tsan/suppressions.txt b/tools/valgrind-webrtc/tsan/suppressions.txt index 65bb634413..ed044425e1 100644 --- a/tools/valgrind-webrtc/tsan/suppressions.txt +++ b/tools/valgrind-webrtc/tsan/suppressions.txt @@ -315,15 +315,6 @@ fun:webrtc::Bitrate::BitrateLast ... } -{ - bug_2078_1 - ThreadSanitizer:Race - fun:talk_base::Thread::~Thread - fun:cricket::RtpSenderReceiver::~RtpSenderReceiver - fun:cricket::RtpSenderReceiver::~RtpSenderReceiver - fun:talk_base::scoped_ptr::~scoped_ptr - ... -} { bug_2078_2 ThreadSanitizer:Race @@ -656,13 +647,6 @@ fun:talk_base::LogMessage::AddLogToStream ... } -{ - bug_2080_4 - ThreadSanitizer:Race - fun:talk_base::Thread::~Thread - fun:talk_base::LogThread::~LogThread - ... -} { bug_2080_6 ThreadSanitizer:Race @@ -848,16 +832,6 @@ fun:talk_base::scoped_ptr::~scoped_ptr ... } -{ - bug_2080_28 - ThreadSanitizer:Race - fun:talk_base::Thread::~Thread - fun:OwnerThread::~OwnerThread - fun:OwnerThread::~OwnerThread - fun:talk_base::scoped_ptr::~scoped_ptr - fun:SignalThreadTest_OwnerThreadGoesAway_Test::TestBody - fun:testing::internal::HandleSehExceptionsInMethodIfSupported -} { bug_2080_29 ThreadSanitizer:Race