Support re-entrant calls to MessageQueueManager::Clear.

BUG=webrtc:7908

Review-Url: https://codereview.webrtc.org/2968753002
Cr-Commit-Position: refs/heads/master@{#18923}

webrtc/rtc_base/messagequeue.cc

@@ -23,26 +23,25 @@ namespace {
 const int kMaxMsgLatency = 150;  // 150 ms
 const int kSlowDispatchLoggingThreshold = 50;  // 50 ms
 
-class SCOPED_LOCKABLE DebugNonReentrantCritScope {
+class SCOPED_LOCKABLE MarkProcessingCritScope {
  public:
-  DebugNonReentrantCritScope(const CriticalSection* cs, bool* locked)
+  MarkProcessingCritScope(const CriticalSection* cs, size_t* processing)
       EXCLUSIVE_LOCK_FUNCTION(cs)
-      : cs_(cs), locked_(locked) {
+      : cs_(cs), processing_(processing) {
     cs_->Enter();
-    RTC_DCHECK(!*locked_);
-    *locked_ = true;
+    *processing_ += 1;
   }
 
-  ~DebugNonReentrantCritScope() UNLOCK_FUNCTION() {
-    *locked_ = false;
+  ~MarkProcessingCritScope() UNLOCK_FUNCTION() {
+    *processing_ -= 1;
     cs_->Leave();
   }
 
  private:
   const CriticalSection* const cs_;
-  bool* locked_;
+  size_t* processing_;
 
-  RTC_DISALLOW_COPY_AND_ASSIGN(DebugNonReentrantCritScope);
+  RTC_DISALLOW_COPY_AND_ASSIGN(MarkProcessingCritScope);
 };
 
 class FunctorPostMessageHandler : public MessageHandler {
@@ -73,7 +72,7 @@ bool MessageQueueManager::IsInitialized() {
   return instance_ != nullptr;
 }
 
-MessageQueueManager::MessageQueueManager() : locked_(false) {}
+MessageQueueManager::MessageQueueManager() : processing_(0) {}
 
 MessageQueueManager::~MessageQueueManager() {
 }
@@ -82,7 +81,9 @@ void MessageQueueManager::Add(MessageQueue *message_queue) {
   return Instance()->AddInternal(message_queue);
 }
 void MessageQueueManager::AddInternal(MessageQueue *message_queue) {
-  DebugNonReentrantCritScope cs(&crit_, &locked_);
+  CritScope cs(&crit_);
+  // Prevent changes while the list of message queues is processed.
+  RTC_DCHECK_EQ(processing_, 0);
   message_queues_.push_back(message_queue);
 }
 
@@ -99,7 +100,9 @@ void MessageQueueManager::RemoveInternal(MessageQueue *message_queue) {
   // the ThreadManager is destroyed, and threads are no longer active).
   bool destroy = false;
   {
-    DebugNonReentrantCritScope cs(&crit_, &locked_);
+    CritScope cs(&crit_);
+    // Prevent changes while the list of message queues is processed.
+    RTC_DCHECK_EQ(processing_, 0);
     std::vector<MessageQueue *>::iterator iter;
     iter = std::find(message_queues_.begin(), message_queues_.end(),
                      message_queue);
@@ -121,10 +124,14 @@ void MessageQueueManager::Clear(MessageHandler *handler) {
   return Instance()->ClearInternal(handler);
 }
 void MessageQueueManager::ClearInternal(MessageHandler *handler) {
-  DebugNonReentrantCritScope cs(&crit_, &locked_);
+  // Deleted objects may cause re-entrant calls to ClearInternal. This is
+  // allowed as the list of message queues does not change while queues are
+  // cleared.
+  MarkProcessingCritScope cs(&crit_, &processing_);
   std::vector<MessageQueue *>::iterator iter;
-  for (iter = message_queues_.begin(); iter != message_queues_.end(); iter++)
-    (*iter)->Clear(handler);
+  for (MessageQueue* queue : message_queues_) {
+    queue->Clear(handler);
+  }
 }
 
 void MessageQueueManager::ProcessAllMessageQueues() {
@@ -154,7 +161,7 @@ void MessageQueueManager::ProcessAllMessageQueuesInternal() {
   };
 
   {
-    DebugNonReentrantCritScope cs(&crit_, &locked_);
+    MarkProcessingCritScope cs(&crit_, &processing_);
     for (MessageQueue* queue : message_queues_) {
       if (!queue->IsProcessingMessages()) {
         // If the queue is not processing messages, it can

webrtc/rtc_base/messagequeue.h

@@ -70,9 +70,11 @@ class MessageQueueManager {
   // This list contains all live MessageQueues.
   std::vector<MessageQueue*> message_queues_ GUARDED_BY(crit_);
 
-  // Acquire this with DebugNonReentrantCritScope.
+  // Methods that don't modify the list of message queues may be called in a
+  // re-entrant fashion. "processing_" keeps track of the depth of re-entrant
+  // calls.
   CriticalSection crit_;
-  bool locked_ GUARDED_BY(crit_);
+  size_t processing_ GUARDED_BY(crit_);
 };
 
 // Derive from this for specialized data

webrtc/rtc_base/messagequeue_unittest.cc

@@ -18,6 +18,8 @@
 #include "webrtc/rtc_base/gunit.h"
 #include "webrtc/rtc_base/logging.h"
 #include "webrtc/rtc_base/nullsocketserver.h"
+#include "webrtc/rtc_base/refcount.h"
+#include "webrtc/rtc_base/refcountedobject.h"
 #include "webrtc/rtc_base/thread.h"
 #include "webrtc/rtc_base/timeutils.h"
 
@@ -215,3 +217,32 @@ TEST(MessageQueueManager, ProcessAllMessageQueuesWithClearedQueue) {
   rtc::Thread::Current()->Post(RTC_FROM_HERE, &event_signaler);
   MessageQueueManager::ProcessAllMessageQueues();
 }
+
+class RefCountedHandler
+  : public MessageHandler,
+    public rtc::RefCountInterface {
+ public:
+  void OnMessage(Message* msg) override {}
+};
+
+class EmptyHandler : public MessageHandler {
+ public:
+  void OnMessage(Message* msg) override {}
+};
+
+TEST(MessageQueueManager, ClearReentrant) {
+  Thread t;
+  EmptyHandler handler;
+  RefCountedHandler* inner_handler(
+      new rtc::RefCountedObject<RefCountedHandler>());
+  // When the empty handler is destroyed, it will clear messages queued for
+  // itself. The message to be cleared itself wraps a MessageHandler object
+  // (RefCountedHandler) so this will cause the message queue to be cleared
+  // again in a re-entrant fashion, which previously triggered a DCHECK.
+  // The inner handler will be removed in a re-entrant fashion from the
+  // message queue of the thread while the outer handler is removed, verifying
+  // that the iterator is not invalidated in "MessageQueue::Clear".
+  t.Post(RTC_FROM_HERE, inner_handler, 0);
+  t.Post(RTC_FROM_HERE, &handler, 0,
+      new ScopedRefMessageData<RefCountedHandler>(inner_handler));
+}