Fix race between Thread ctor/dtor and MessageQueueManager registrations.

webrtc/base/thread.cc

Index: webrtc/base/thread.cc
diff --git a/webrtc/base/thread.cc b/webrtc/base/thread.cc
index 4197d28175a05037f5ad953aba64d6b72ae62bef..9de99cdc710e552c51c153311807951c49d46025 100644
--- a/webrtc/base/thread.cc
+++ b/webrtc/base/thread.cc
@@ -139,7 +139,7 @@ Thread::ScopedDisallowBlockingCalls::~ScopedDisallowBlockingCalls() {
 }
 
 Thread::Thread(SocketServer* ss)
-    : MessageQueue(ss),
+    : MessageQueue(ss, false),
       running_(true, false),
 #if defined(WEBRTC_WIN)
       thread_(NULL),
@@ -148,11 +148,12 @@ Thread::Thread(SocketServer* ss)
       owned_(true),
       blocking_calls_allowed_(true) {
   SetName("Thread", this);  // default name
+  MessageQueueManager::Add(this);

[pthatcher1, 2016/02/03 23:51:47]

So, is this basically to make sure that SocketServer::SetMessageQueue is called
after MessageQueueManager::Add is called?  Because that's the only different
that I can see.

If that's the case, can you explain why that's needed?

[joachim, 2016/02/04 00:01:20]

Please see my comment in messagequeue.cc. The change is about calling
MessageQueueManager::Add after the Thread object is created completely.

 }
 
 Thread::~Thread() {
   Stop();
-  Clear(NULL);

[pthatcher1, 2016/02/03 23:51:47]

Why do we want to not call Clear(NULL) here?  Is it because we want this to be
called first?

SignalQueueDestroyed();
MessageQueueManager::Remove(this)

?


That's the only difference I see.  If we're just trying to fix the race
condition, it seems at least a little dangerous to introduce another behavior
change at the same time.  

[joachim, 2016/02/04 00:01:20]

We want to have the object removed from the MessageQueueManager before the
constructor starts running. This is done by DoDestroy in the parent class.
Otherwise another thread might run MessageQueueManager::Clear causing a call to
Clear in this object while it is already partially destroyed.

[pthatcher1, 2016/02/04 00:50:38]

That makes it clear why we should call DoDestroy() in the destructor.  But why
not still call Clear(NULL) before calling DoDestroy?  

[joachim, 2016/02/04 01:00:09]

DoDestroy calls Clear(NULL) internally so I think it's redundant calling it also
from here.

+  DoDestroy();

[pthatcher1, 2016/02/03 23:51:47]

Can you leave a comment like "We can't rely on the implicit call to
MessageQueue::~MessageQueue because by then, the vtable will have been altered
in a way that causes the call to MessageQueueManager::Remove to fail."

?

[joachim, 2016/02/04 00:01:20]

I added a similar comment at the destructor of MessageQueue.

[pthatcher1, 2016/02/04 00:50:38]

FYI, I don't see the new comments.

[joachim, 2016/02/04 01:00:09]

Sorry for not being clearer, it's in messagequeue.h at the declaration of the
dtor (adopted from a similar comment in thread.h about calling "Stop"):
-----
// NOTE: ALL SUBCLASSES OF MessageQueue MUST CALL DoDestroy() IN THEIR
// DESTRUCTORS! This is required to avoid a data race between the destructor
// modifying the vtable, and the MessageQueue being removed from the
// MessageQueueManager.
-----

[pthatcher1, 2016/02/04 03:14:11]

Right, but can you put one in the ctor also explaining the need for the "don't
add to manager" boolean?

[joachim, 2016/02/04 08:35:56]

Done.

 }
 
 bool Thread::SleepMs(int milliseconds) {