When clearing the priority message queue, don't copy an item to itself.
This avoids a memcpy to overlapping---in this case the same---memory locations.
BUG=4100
R=juberti@webrtc.org, pthatcher@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/33019004
Cr-Commit-Position: refs/heads/master@{#8449}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8449 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc
index 74b1024..e5d69a3 100644
--- a/webrtc/base/messagequeue.cc
+++ b/webrtc/base/messagequeue.cc
@@ -361,7 +361,6 @@
}
// Remove from priority queue. Not directly iterable, so use this approach
-
PriorityQueue::container_type::iterator new_end = dmsgq_.container().begin();
for (PriorityQueue::container_type::iterator it = new_end;
it != dmsgq_.container().end(); ++it) {
@@ -372,7 +371,10 @@
delete it->msg_.pdata;
}
} else {
- *new_end++ = *it;
+ if (new_end != it) {
+ *new_end = *it;
+ }
+ new_end++;
}
}
dmsgq_.container().erase(new_end, dmsgq_.container().end());
diff --git a/webrtc/base/messagequeue_unittest.cc b/webrtc/base/messagequeue_unittest.cc
index 871542d..ac9affe 100644
--- a/webrtc/base/messagequeue_unittest.cc
+++ b/webrtc/base/messagequeue_unittest.cc
@@ -20,7 +20,7 @@
using namespace rtc;
-class MessageQueueTest: public testing::Test, public MessageQueue {
+class MessageQueueForTest : public MessageQueue {
public:
bool IsLocked_Worker() {
if (!crit_.TryEnter()) {
@@ -29,24 +29,38 @@
crit_.Leave();
return false;
}
+
bool IsLocked() {
// We have to do this on a worker thread, or else the TryEnter will
// succeed, since our critical sections are reentrant.
Thread worker;
worker.Start();
return worker.Invoke<bool>(
- rtc::Bind(&MessageQueueTest::IsLocked_Worker, this));
+ rtc::Bind(&MessageQueueForTest::IsLocked_Worker, this));
+ }
+
+ size_t GetDmsgqSize() {
+ return dmsgq_.size();
+ }
+
+ const DelayedMessage& GetDmsgqTop() {
+ return dmsgq_.top();
}
};
+class MessageQueueTest : public testing::Test {
+ protected:
+ MessageQueueForTest q_;
+};
+
struct DeletedLockChecker {
- DeletedLockChecker(MessageQueueTest* test, bool* was_locked, bool* deleted)
- : test(test), was_locked(was_locked), deleted(deleted) { }
+ DeletedLockChecker(MessageQueueForTest* q, bool* was_locked, bool* deleted)
+ : q_(q), was_locked(was_locked), deleted(deleted) { }
~DeletedLockChecker() {
*deleted = true;
- *was_locked = test->IsLocked();
+ *was_locked = q_->IsLocked();
}
- MessageQueueTest* test;
+ MessageQueueForTest* q_;
bool* was_locked;
bool* deleted;
};
@@ -73,8 +87,7 @@
TEST_F(MessageQueueTest,
DelayedPostsWithIdenticalTimesAreProcessedInFifoOrder) {
- MessageQueue q;
- DelayedPostsWithIdenticalTimesAreProcessedInFifoOrder(&q);
+ DelayedPostsWithIdenticalTimesAreProcessedInFifoOrder(&q_);
NullSocketServer nullss;
MessageQueue q_nullss(&nullss);
DelayedPostsWithIdenticalTimesAreProcessedInFifoOrder(&q_nullss);
@@ -83,10 +96,10 @@
TEST_F(MessageQueueTest, DisposeNotLocked) {
bool was_locked = true;
bool deleted = false;
- DeletedLockChecker* d = new DeletedLockChecker(this, &was_locked, &deleted);
- Dispose(d);
+ DeletedLockChecker* d = new DeletedLockChecker(&q_, &was_locked, &deleted);
+ q_.Dispose(d);
Message msg;
- EXPECT_FALSE(Get(&msg, 0));
+ EXPECT_FALSE(q_.Get(&msg, 0));
EXPECT_TRUE(deleted);
EXPECT_FALSE(was_locked);
}
@@ -102,18 +115,138 @@
bool* deleted_;
};
-TEST_F(MessageQueueTest, DiposeHandlerWithPostedMessagePending) {
+// TODO(decurtis): Test that ordering of elements is done properly.
+// TODO(decurtis): Test that timestamps are being properly set.
+
+TEST_F(MessageQueueTest, DisposeHandlerWithPostedMessagePending) {
bool deleted = false;
DeletedMessageHandler *handler = new DeletedMessageHandler(&deleted);
// First, post a dispose.
- Dispose(handler);
+ q_.Dispose(handler);
// Now, post a message, which should *not* be returned by Get().
- Post(handler, 1);
+ q_.Post(handler, 1);
Message msg;
- EXPECT_FALSE(Get(&msg, 0));
+ EXPECT_FALSE(q_.Get(&msg, 0));
EXPECT_TRUE(deleted);
}
+// Test Clear for removing messages that have been posted for times in
+// the past.
+TEST_F(MessageQueueTest, ClearPast) {
+ TimeStamp now = Time();
+ Message msg;
+
+ // Test removing the only element.
+ q_.PostAt(now - 4, NULL, 1);
+ q_.Clear(NULL, 1, NULL);
+
+ // Make sure the queue is empty now.
+ EXPECT_FALSE(q_.Get(&msg, 0));
+
+ // Test removing the one element with a two element list.
+ q_.PostAt(now - 4, NULL, 1);
+ q_.PostAt(now - 2, NULL, 3);
+
+ q_.Clear(NULL, 1, NULL);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(3U, msg.message_id);
+
+ // Make sure the queue is empty now.
+ EXPECT_FALSE(q_.Get(&msg, 0));
+
+
+ // Test removing the three element with a two element list.
+ q_.PostAt(now - 4, NULL, 1);
+ q_.PostAt(now - 2, NULL, 3);
+
+ q_.Clear(NULL, 3, NULL);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(1U, msg.message_id);
+
+ // Make sure the queue is empty now.
+ EXPECT_FALSE(q_.Get(&msg, 0));
+
+
+ // Test removing the two element in a three element list.
+ q_.PostAt(now - 4, NULL, 1);
+ q_.PostAt(now - 3, NULL, 2);
+ q_.PostAt(now - 2, NULL, 3);
+
+ q_.Clear(NULL, 2, NULL);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(1U, msg.message_id);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(3U, msg.message_id);
+
+ // Make sure the queue is empty now.
+ EXPECT_FALSE(q_.Get(&msg, 0));
+
+
+ // Test not clearing any messages.
+ q_.PostAt(now - 4, NULL, 1);
+ q_.PostAt(now - 3, NULL, 2);
+ q_.PostAt(now - 2, NULL, 3);
+
+ // Remove nothing.
+ q_.Clear(NULL, 0, NULL);
+ q_.Clear(NULL, 4, NULL);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(1U, msg.message_id);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(2U, msg.message_id);
+
+ EXPECT_TRUE(q_.Get(&msg, 0));
+ EXPECT_EQ(3U, msg.message_id);
+
+ // Make sure the queue is empty now.
+ EXPECT_FALSE(q_.Get(&msg, 0));
+}
+
+// Test clearing messages that have been posted for the future.
+TEST_F(MessageQueueTest, ClearFuture) {
+ EXPECT_EQ(0U, q_.GetDmsgqSize());
+ q_.PostDelayed(10, NULL, 4);
+ EXPECT_EQ(1U, q_.GetDmsgqSize());
+ q_.PostDelayed(13, NULL, 4);
+ EXPECT_EQ(2U, q_.GetDmsgqSize());
+ q_.PostDelayed(9, NULL, 2);
+ EXPECT_EQ(3U, q_.GetDmsgqSize());
+ q_.PostDelayed(11, NULL, 10);
+ EXPECT_EQ(4U, q_.GetDmsgqSize());
+
+ EXPECT_EQ(9, q_.GetDmsgqTop().cmsDelay_);
+
+ MessageList removed;
+ q_.Clear(NULL, 10, &removed);
+ EXPECT_EQ(1U, removed.size());
+ EXPECT_EQ(3U, q_.GetDmsgqSize());
+
+ removed.clear();
+ q_.Clear(NULL, 4, &removed);
+ EXPECT_EQ(2U, removed.size());
+ EXPECT_EQ(1U, q_.GetDmsgqSize());
+
+ removed.clear();
+ q_.Clear(NULL, 4, &removed);
+ EXPECT_EQ(0U, removed.size());
+ EXPECT_EQ(1U, q_.GetDmsgqSize());
+
+ removed.clear();
+ q_.Clear(NULL, 2, &removed);
+ EXPECT_EQ(1U, removed.size());
+ EXPECT_EQ(0U, q_.GetDmsgqSize());
+
+ Message msg;
+ EXPECT_FALSE(q_.Get(&msg, 0));
+}
+
+
struct UnwrapMainThreadScope {
UnwrapMainThreadScope() : rewrap_(Thread::Current() != NULL) {
if (rewrap_) ThreadManager::Instance()->UnwrapCurrentThread();
@@ -125,7 +258,7 @@
bool rewrap_;
};
-TEST(MessageQueueManager, Clear) {
+TEST(MessageQueueManager, DeletedHandler) {
UnwrapMainThreadScope s;
if (MessageQueueManager::IsInitialized()) {
LOG(LS_INFO) << "Unable to run MessageQueueManager::Clear test, since the "