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}

rtc_base/signalthread.cc

@@ -11,6 +11,7 @@
 #include "rtc_base/signalthread.h"
 
 #include "rtc_base/checks.h"
+#include "rtc_base/ptr_util.h"
 
 namespace rtc {
 
@@ -124,6 +125,12 @@ void SignalThread::OnMessage(Message *msg) {
   }
 }
 
+SignalThread::Worker::Worker(SignalThread* parent)
+    : Thread(MakeUnique<NullSocketServer>(), /*do_init=*/false),
+      parent_(parent) {
+  DoInit();
+}
+
 SignalThread::Worker::~Worker() {
   Stop();
 }

rtc_base/signalthread.h

@@ -104,9 +104,7 @@ class SignalThread
 
   class Worker : public Thread {
    public:
-    explicit Worker(SignalThread* parent)
-        : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())),
-          parent_(parent) {}
+    explicit Worker(SignalThread* parent);
     ~Worker() override;
     void Run() override;
     bool IsProcessingMessages() override;

rtc_base/thread.cc

@@ -124,15 +124,25 @@ Thread::ScopedDisallowBlockingCalls::~ScopedDisallowBlockingCalls() {
 // DEPRECATED.
 Thread::Thread() : Thread(SocketServer::CreateDefault()) {}
 
-Thread::Thread(SocketServer* ss) : MessageQueue(ss, false) {
-  SetName("Thread", this);  // default name
-  DoInit();
-}
+Thread::Thread(SocketServer* ss) : Thread(ss, /*do_init=*/true) {}
 
 Thread::Thread(std::unique_ptr<SocketServer> ss)
+    : Thread(std::move(ss), /*do_init=*/true) {}
+
+Thread::Thread(SocketServer* ss, bool do_init)
+    : MessageQueue(ss, /*do_init=*/false) {
+  SetName("Thread", this);  // default name
+  if (do_init) {
+    DoInit();
+  }
+}
+
+Thread::Thread(std::unique_ptr<SocketServer> ss, bool do_init)
     : MessageQueue(std::move(ss), false) {
   SetName("Thread", this);  // default name
-  DoInit();
+  if (do_init) {
+    DoInit();
+  }
 }
 
 Thread::~Thread() {
@@ -524,7 +534,9 @@ bool Thread::IsRunning() {
 #endif
 }
 
-AutoThread::AutoThread() : Thread(SocketServer::CreateDefault()) {
+AutoThread::AutoThread()
+    : Thread(SocketServer::CreateDefault(), /*do_init=*/false) {
+  DoInit();
   if (!ThreadManager::Instance()->CurrentThread()) {
     ThreadManager::Instance()->SetCurrentThread(this);
   }
@@ -539,7 +551,8 @@ AutoThread::~AutoThread() {
 }
 
 AutoSocketServerThread::AutoSocketServerThread(SocketServer* ss)
-    : Thread(ss) {
+    : Thread(ss, /*do_init=*/false) {
+  DoInit();
   old_thread_ = ThreadManager::Instance()->CurrentThread();
   // Temporarily set the current thread to nullptr so that we can keep checks
   // around that catch unintentional pointer overwrites.

rtc_base/thread.h

@@ -111,6 +111,11 @@ class RTC_LOCKABLE Thread : public MessageQueue {
 
   explicit Thread(SocketServer* ss);
   explicit Thread(std::unique_ptr<SocketServer> ss);
+  // Constructors meant for subclasses; they should call DoInit themselves and
+  // pass false for |do_init|, so that DoInit is called only on the fully
+  // instantiated class, which avoids a vptr data race.
+  Thread(SocketServer* ss, bool do_init);
+  Thread(std::unique_ptr<SocketServer> ss, bool do_init);
 
   // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or
   // guarantee Stop() is explicitly called before the subclass is destroyed).