Fix race between Thread ctor/dtor and MessageQueueManager registrations.

webrtc/base/messagequeue.h

Index: webrtc/base/messagequeue.h
diff --git a/webrtc/base/messagequeue.h b/webrtc/base/messagequeue.h
index 856a6eb4bc89cc1c71c9cb520d3ee2493d92075a..3b55454d8df1248ff4872327c81ecd73032b4abf 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 should pass false for add_to_queue_manager and
+  // call MessageQueueManager::Add from their constructor to prevent races
+  // between the MessageQueueManager already using the object while the vtable
+  // is still being created.
+  explicit MessageQueue(SocketServer* ss = NULL,
+                        bool add_to_queue_manager = true);

[Taylor Brandstetter, 2016/02/04 19:16:38]

Instead of the "add_to_queue_manager" approach, why not have a "DoInit()" which
calls MessageQueueManager::Add, to mirror "DoDestroy()"? And in
MessageQueueManager::AddInternal, do nothing if a MessageQueue is added twice.
That would make the rule simpler for subclasses: "Call DoInit in the
constructor, and DoDestroy in the destructor"

[joachim, 2016/02/04 20:22:31]

Hmm, wouldn't that be the same race as before if both MessageQueue and Thread
call DoInit in their constructors?
1. Thread ctor start
2. MessageQueue ctor start
3. MessageQueue ctor calls DoInit -> partial object gets registered
4. Thread ctor calls DoInit -> noop, object is already registered

However a better naming instead of "add_to_queue_manager" would probably be
"init_queue" and instead of calling "MessageQueueManager::Add" from the subclass
call a method "DoInit" that calls MQM::Add.

I uploaded a new patchset for further discussion.

[Taylor Brandstetter, 2016/02/04 21:05:58]

You're right; I wasn't thinking straight...
I like what you did in the new patchset. Though the name "init_thread" may be a
little confusing.

[joachim, 2016/02/04 22:54:08]

I think I'll leave it at "init_queue" even for the Thread classes because it's
the initialization of the underlying MessageQueue that is controlled through the
flag.

+
+  // 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.

[Taylor Brandstetter, 2016/02/04 19:16:38]

Isn't it more of a race between the MessageQueue being removed from the
MessageQueueManager and MessageQueueManager::Clear being called on another
thread?

[joachim, 2016/02/04 20:22:31]

Right, I updated the comment.

   virtual ~MessageQueue();
 
   SocketServer* socketserver() { return ss_; }
@@ -241,6 +252,9 @@ class MessageQueue {
                    uint32_t id,
                    MessageData* pdata);
 
+  // Perform cleanup, subclasses 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 +266,7 @@ class MessageQueue {
   PriorityQueue dmsgq_;
   uint32_t dmsgq_next_num_;
   CriticalSection crit_;
+  bool fDestroyed_;
 
  private:
   RTC_DISALLOW_COPY_AND_ASSIGN(MessageQueue);