Support re-entrant calls to MessageQueueManager::Clear.

webrtc/rtc_base/messagequeue.cc

Index: webrtc/rtc_base/messagequeue.cc
diff --git a/webrtc/rtc_base/messagequeue.cc b/webrtc/rtc_base/messagequeue.cc
index c22cea70e92005cff2d48a67602e9e0d7ac4e82c..7ed87ba1b1d5b0a93c8c4151289deb08a17d620f 100644
--- a/webrtc/rtc_base/messagequeue.cc
+++ b/webrtc/rtc_base/messagequeue.cc
@@ -23,28 +23,6 @@ namespace {
 const int kMaxMsgLatency = 150;  // 150 ms
 const int kSlowDispatchLoggingThreshold = 50;  // 50 ms
 
-class SCOPED_LOCKABLE DebugNonReentrantCritScope {
- public:
-  DebugNonReentrantCritScope(const CriticalSection* cs, bool* locked)
-      EXCLUSIVE_LOCK_FUNCTION(cs)
-      : cs_(cs), locked_(locked) {
-    cs_->Enter();
-    RTC_DCHECK(!*locked_);
-    *locked_ = true;
-  }
-
-  ~DebugNonReentrantCritScope() UNLOCK_FUNCTION() {
-    *locked_ = false;
-    cs_->Leave();
-  }
-
- private:
-  const CriticalSection* const cs_;
-  bool* locked_;
-
-  RTC_DISALLOW_COPY_AND_ASSIGN(DebugNonReentrantCritScope);
-};

[Taylor Brandstetter, 2017/07/05 22:41:47]

This still seems like a useful type of helper class, to ensure we never
increment "processing_" and forget to decrement it.

[joachim, 2017/07/05 23:03:11]

Done (renamed to "MarkProcessingCritScope").

-
 class FunctorPostMessageHandler : public MessageHandler {
  public:
   void OnMessage(Message* msg) override {
@@ -73,7 +51,7 @@ bool MessageQueueManager::IsInitialized() {
   return instance_ != nullptr;
 }
 
-MessageQueueManager::MessageQueueManager() : locked_(false) {}
+MessageQueueManager::MessageQueueManager() : processing_(0) {}
 
 MessageQueueManager::~MessageQueueManager() {
 }
@@ -82,7 +60,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 +79,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 +103,13 @@ void MessageQueueManager::Clear(MessageHandler *handler) {
   return Instance()->ClearInternal(handler);
 }
 void MessageQueueManager::ClearInternal(MessageHandler *handler) {
-  DebugNonReentrantCritScope cs(&crit_, &locked_);
+  CritScope cs(&crit_);

[Taylor Brandstetter, 2017/07/05 22:41:47]

May want to comment here that this allows a deleted Message object to cause
ClearInternal to be called again in a re-entrant fashion.

[joachim, 2017/07/05 23:03:11]

Done.

+  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);
+  }
+  processing_--;
 }
 
 void MessageQueueManager::ProcessAllMessageQueues() {
@@ -154,7 +139,8 @@ void MessageQueueManager::ProcessAllMessageQueuesInternal() {
   };
 
   {
-    DebugNonReentrantCritScope cs(&crit_, &locked_);
+    CritScope cs(&crit_);
+    processing_++;
     for (MessageQueue* queue : message_queues_) {
       if (!queue->IsProcessingMessages()) {
         // If the queue is not processing messages, it can
@@ -165,6 +151,7 @@ void MessageQueueManager::ProcessAllMessageQueuesInternal() {
       queue->PostDelayed(RTC_FROM_HERE, 0, nullptr, MQID_DISPOSE,
                          new ScopedIncrement(&queues_not_done));
     }
+    processing_--;
   }
   // Note: One of the message queues may have been on this thread, which is why
   // we can't synchronously wait for queues_not_done to go to 0; we need to