Fix race between Thread ctor/dtor and MessageQueueManager registrations.

webrtc/base/thread.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 121 matching lines @@
   : thread_(Thread::Current()),
     previous_state_(thread_->SetAllowBlockingCalls(false)) {
 }
 
 Thread::ScopedDisallowBlockingCalls::~ScopedDisallowBlockingCalls() {
   ASSERT(thread_->IsCurrent());
   thread_->SetAllowBlockingCalls(previous_state_);
 }
 
 Thread::Thread(SocketServer* ss)
-    : MessageQueue(ss),
+    : MessageQueue(ss, false),
       running_(true, false),
 #if defined(WEBRTC_WIN)
       thread_(NULL),
       thread_id_(0),
 #endif
       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) {
   AssertBlockingIsAllowedOnCurrentThread();
 
 #if defined(WEBRTC_WIN)
   ::Sleep(milliseconds);
   return true;
 #else
   // POSIX has both a usleep() and a nanosleep(), but the former is deprecated,
@@ skipping 365 matching lines @@
   if (SUCCEEDED(hr)) {
     Thread::Run();
     CoUninitialize();
   } else {
     LOG(LS_ERROR) << "CoInitialize failed, hr=" << hr;
   }
 }
 #endif
 
 }  // namespace rtc