From 2bffc3cb72e2250cbf6ed7e5f4b399395ca046cb Mon Sep 17 00:00:00 2001 From: "decurtis@webrtc.org" Date: Sat, 21 Feb 2015 01:45:04 +0000 Subject: [PATCH] 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 --- webrtc/base/messagequeue.cc | 6 +- webrtc/base/messagequeue_unittest.cc | 165 ++++++++++++++++++++++++--- 2 files changed, 153 insertions(+), 18 deletions(-) diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc index 74b1024de..e5d69a324 100644 --- a/webrtc/base/messagequeue.cc +++ b/webrtc/base/messagequeue.cc @@ -361,7 +361,6 @@ void MessageQueue::Clear(MessageHandler *phandler, uint32 id, } // 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 @@ void MessageQueue::Clear(MessageHandler *phandler, uint32 id, 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 871542df2..ac9affece 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 @@ class MessageQueueTest: public testing::Test, public MessageQueue { 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( - 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 @@ static void DelayedPostsWithIdenticalTimesAreProcessedInFifoOrder( 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, 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 @@ class DeletedMessageHandler : public MessageHandler { 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 @@ struct UnwrapMainThreadScope { bool rewrap_; }; -TEST(MessageQueueManager, Clear) { +TEST(MessageQueueManager, DeletedHandler) { UnwrapMainThreadScope s; if (MessageQueueManager::IsInitialized()) { LOG(LS_INFO) << "Unable to run MessageQueueManager::Clear test, since the "