From 25d1f28fa978f555d9a5a8855712db5a74d6828d Mon Sep 17 00:00:00 2001 From: jbauch Date: Fri, 5 Feb 2016 00:25:02 -0800 Subject: [PATCH] Fix race between Thread ctor/dtor and MessageQueueManager registrations. This CL fixes a race where for Thread objects the parent MessageQueue constructor registers the object in the MessageQueueManager even though the Thread is not constructed completely yet. Same happens during destruction. BUG=webrtc:1225 Review URL: https://codereview.webrtc.org/1666863002 Cr-Commit-Position: refs/heads/master@{#11497} --- webrtc/base/messagequeue.cc | 26 +++++++++++++++++++++--- webrtc/base/messagequeue.h | 23 ++++++++++++++++++++- webrtc/base/thread.cc | 9 ++++++--- webrtc/base/thread.h | 14 +++++++++---- webrtc/base/thread_unittest.cc | 37 ++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 11 deletions(-) diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc index 857cf12927..bbdb941ffa 100644 --- a/webrtc/base/messagequeue.cc +++ b/webrtc/base/messagequeue.cc @@ -116,9 +116,9 @@ void MessageQueueManager::ClearInternal(MessageHandler *handler) { //------------------------------------------------------------------ // MessageQueue -MessageQueue::MessageQueue(SocketServer* ss) +MessageQueue::MessageQueue(SocketServer* ss, bool init_queue) : ss_(ss), fStop_(false), fPeekKeep_(false), - dmsgq_next_num_(0) { + dmsgq_next_num_(0), fInitialized_(false), fDestroyed_(false) { if (!ss_) { // Currently, MessageQueue holds a socket server, and is the base class for // Thread. It seems like it makes more sense for Thread to hold the socket @@ -129,10 +129,30 @@ MessageQueue::MessageQueue(SocketServer* ss) ss_ = default_ss_.get(); } ss_->SetMessageQueue(this); - MessageQueueManager::Add(this); + if (init_queue) { + DoInit(); + } } MessageQueue::~MessageQueue() { + DoDestroy(); +} + +void MessageQueue::DoInit() { + if (fInitialized_) { + return; + } + + fInitialized_ = true; + MessageQueueManager::Add(this); +} + +void MessageQueue::DoDestroy() { + if (fDestroyed_) { + return; + } + + fDestroyed_ = true; // The signal is done from here to ensure // that it always gets called when the queue // is going away. diff --git a/webrtc/base/messagequeue.h b/webrtc/base/messagequeue.h index 856a6eb4bc..a7991a8923 100644 --- a/webrtc/base/messagequeue.h +++ b/webrtc/base/messagequeue.h @@ -167,7 +167,18 @@ class MessageQueue { public: static const int kForever = -1; - explicit MessageQueue(SocketServer* ss = NULL); + // Create a new MessageQueue and optionally assign it to the passed + // SocketServer. Subclasses that override Clear should pass false for + // init_queue and call DoInit() from their constructor to prevent races + // with the MessageQueueManager using the object while the vtable is still + // being created. + explicit MessageQueue(SocketServer* ss = NULL, + bool init_queue = true); + + // NOTE: SUBCLASSES OF MessageQueue THAT OVERRIDE Clear MUST CALL + // DoDestroy() IN THEIR DESTRUCTORS! This is required to avoid a data race + // between the destructor modifying the vtable, and the MessageQueueManager + // calling Clear on the object from a different thread. virtual ~MessageQueue(); SocketServer* socketserver() { return ss_; } @@ -241,6 +252,14 @@ class MessageQueue { uint32_t id, MessageData* pdata); + // Perform initialization, subclasses must call this from their constructor + // if false was passed as init_queue to the MessageQueue constructor. + void DoInit(); + + // Perform cleanup, subclasses that override Clear must call this from the + // destructor. + void DoDestroy(); + // The SocketServer is not owned by MessageQueue. SocketServer* ss_; // If a server isn't supplied in the constructor, use this one. @@ -252,6 +271,8 @@ class MessageQueue { PriorityQueue dmsgq_; uint32_t dmsgq_next_num_; CriticalSection crit_; + bool fInitialized_; + bool fDestroyed_; private: RTC_DISALLOW_COPY_AND_ASSIGN(MessageQueue); diff --git a/webrtc/base/thread.cc b/webrtc/base/thread.cc index 4197d28175..cda4ba475c 100644 --- a/webrtc/base/thread.cc +++ b/webrtc/base/thread.cc @@ -138,8 +138,8 @@ Thread::ScopedDisallowBlockingCalls::~ScopedDisallowBlockingCalls() { thread_->SetAllowBlockingCalls(previous_state_); } -Thread::Thread(SocketServer* ss) - : MessageQueue(ss), +Thread::Thread(SocketServer* ss, bool init_queue) + : MessageQueue(ss, false), running_(true, false), #if defined(WEBRTC_WIN) thread_(NULL), @@ -148,11 +148,14 @@ Thread::Thread(SocketServer* ss) owned_(true), blocking_calls_allowed_(true) { SetName("Thread", this); // default name + if (init_queue) { + DoInit(); + } } Thread::~Thread() { Stop(); - Clear(NULL); + DoDestroy(); } bool Thread::SleepMs(int milliseconds) { diff --git a/webrtc/base/thread.h b/webrtc/base/thread.h index f91aa56733..df5a686952 100644 --- a/webrtc/base/thread.h +++ b/webrtc/base/thread.h @@ -94,7 +94,13 @@ class Runnable { class Thread : public MessageQueue { public: - explicit Thread(SocketServer* ss = NULL); + // Create a new Thread and optionally assign it to the passed SocketServer. + // Subclasses that override Clear should pass false for init_queue and call + // DoInit() from their constructor to prevent races with the + // MessageQueueManager already using the object while the vtable is still + // being created. + explicit Thread(SocketServer* ss = nullptr, bool init_queue = true); + // 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 @@ -285,7 +291,7 @@ class Thread : public MessageQueue { class AutoThread : public Thread { public: - explicit AutoThread(SocketServer* ss = 0); + explicit AutoThread(SocketServer* ss = nullptr); ~AutoThread() override; private: @@ -297,10 +303,10 @@ class AutoThread : public Thread { class ComThread : public Thread { public: ComThread() {} - virtual ~ComThread() { Stop(); } + ~ComThread() override { Stop(); } protected: - virtual void Run(); + void Run() override; private: RTC_DISALLOW_COPY_AND_ASSIGN(ComThread); diff --git a/webrtc/base/thread_unittest.cc b/webrtc/base/thread_unittest.cc index 240ffc411f..7889e29da8 100644 --- a/webrtc/base/thread_unittest.cc +++ b/webrtc/base/thread_unittest.cc @@ -13,6 +13,7 @@ #include "webrtc/base/event.h" #include "webrtc/base/gunit.h" #include "webrtc/base/physicalsocketserver.h" +#include "webrtc/base/sigslot.h" #include "webrtc/base/socketaddress.h" #include "webrtc/base/thread.h" @@ -377,6 +378,42 @@ TEST(ThreadTest, ThreeThreadsInvoke) { EXPECT_TRUE_WAIT(thread_a_called.Get(), 2000); } +// Set the name on a thread when the underlying QueueDestroyed signal is +// triggered. This causes an error if the object is already partially +// destroyed. +class SetNameOnSignalQueueDestroyedTester : public sigslot::has_slots<> { + public: + SetNameOnSignalQueueDestroyedTester(Thread* thread) : thread_(thread) { + thread->SignalQueueDestroyed.connect( + this, &SetNameOnSignalQueueDestroyedTester::OnQueueDestroyed); + } + + void OnQueueDestroyed() { + // Makes sure that if we access the Thread while it's being destroyed, that + // it doesn't cause a problem because the vtable has been modified. + thread_->SetName("foo", nullptr); + } + + private: + Thread* thread_; +}; + +TEST(ThreadTest, SetNameOnSignalQueueDestroyed) { + Thread* thread1 = new Thread(); + SetNameOnSignalQueueDestroyedTester tester1(thread1); + delete thread1; + + Thread* thread2 = new AutoThread(); + SetNameOnSignalQueueDestroyedTester tester2(thread2); + delete thread2; + +#if defined(WEBRTC_WIN) + Thread* thread3 = new ComThread(); + SetNameOnSignalQueueDestroyedTester tester3(thread3); + delete thread3; +#endif +} + class AsyncInvokeTest : public testing::Test { public: void IntCallback(int value) {