generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

When clearing the priority message queue, don't copy an item to itself.

Browse Source 
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
This commit is contained in:
decurtis@webrtc.org 2015-02-21 01:45:04 +00:00
parent d3a487c28b
commit 2bffc3cb72
2 changed files with 153 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
webrtc/base/messagequeue.cc
View File

@ -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());

165
webrtc/base/messagequeue_unittest.cc
View File

@ -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<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 @@ 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 "