Support re-entrant calls to MessageQueueManager::Clear.
BUG=webrtc:7908
Review-Url: https://codereview.webrtc.org/2968753002
Cr-Commit-Position: refs/heads/master@{#18923}
diff --git a/webrtc/rtc_base/messagequeue.cc b/webrtc/rtc_base/messagequeue.cc
index c22cea7..fac7609 100644
--- a/webrtc/rtc_base/messagequeue.cc
+++ b/webrtc/rtc_base/messagequeue.cc
@@ -23,26 +23,25 @@
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 @@
return instance_ != nullptr;
}
-MessageQueueManager::MessageQueueManager() : locked_(false) {}
+MessageQueueManager::MessageQueueManager() : processing_(0) {}
MessageQueueManager::~MessageQueueManager() {
}
@@ -82,7 +81,9 @@
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 @@
// 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 @@
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 @@
};
{
- 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
diff --git a/webrtc/rtc_base/messagequeue.h b/webrtc/rtc_base/messagequeue.h
index d4057cd..0d0654e 100644
--- a/webrtc/rtc_base/messagequeue.h
+++ b/webrtc/rtc_base/messagequeue.h
@@ -70,9 +70,11 @@
// 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
diff --git a/webrtc/rtc_base/messagequeue_unittest.cc b/webrtc/rtc_base/messagequeue_unittest.cc
index 78bcfcb..e31adf9 100644
--- a/webrtc/rtc_base/messagequeue_unittest.cc
+++ b/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 @@
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));
+}