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

@@ -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.

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);

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) {

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);

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) {