r137471 - in trunk/src: chrome/nacl ipc tools/heapcheck tools/valgrind/memcheck

[dmic...@chromium.org]

Author: dmic...@chromium.org
Date: Wed May 16 11:58:32 2012
New Revision: 137471

Log:
PPAPI/NaCl: Fix leaky NaClIPCAdapter test.

The NaClIPCAdapter was not leaking, but the Channel deletion is posted as a task that the test was not running. This makes all the NaClIPCAdapter test cases do RunAllPending on shutdown to delete stuff. Unfortunately, one of the tests results in calling "Close()" on the channel in one of these tasks, and that fails for IPC::TestSink, because IPC::Channel::Close dereferences channel_impl_ unconditionally, and the channel_impl_ is NULL for TestSink. So this patch also makes Channel::Close() do nothing if channel_impl_ is NULL.

BUG=127954
TEST=


Review URL: https://chromiumcodereview.appspot.com/10383167

Modified:
trunk/src/chrome/nacl/nacl_ipc_adapter_unittest.cc
trunk/src/ipc/ipc_channel_posix.cc
trunk/src/ipc/ipc_channel_win.cc
trunk/src/tools/heapcheck/suppressions.txt
trunk/src/tools/valgrind/memcheck/suppressions.txt

Modified: trunk/src/chrome/nacl/nacl_ipc_adapter_unittest.cc
==============================================================================
--- trunk/src/chrome/nacl/nacl_ipc_adapter_unittest.cc (original)
+++ trunk/src/chrome/nacl/nacl_ipc_adapter_unittest.cc Wed May 16 11:58:32 2012
@@ -24,13 +24,19 @@
virtual void SetUp() OVERRIDE {
sink_ = new IPC::TestSink;

- // Takes ownership of the sink_ pointer.
+ // Takes ownership of the sink_ pointer. Note we provide the current message
+ // loop instead of using a real IO thread. This should work OK since we do
+ // not need real IPC for the tests.
adapter_ = new NaClIPCAdapter(scoped_ptr<IPC::Channel>(sink_),
base::MessageLoopProxy::current());
}
virtual void TearDown() OVERRIDE {
sink_ = NULL; // This pointer is actually owned by the IPCAdapter.
adapter_ = NULL;
+ // The adapter destructor has to post a task to destroy the Channel on the
+ // IO thread. For the purposes of the test, we just need to make sure that
+ // task gets run, or it will appear as a leak.
+ message_loop_.RunAllPending();
}

protected:
@@ -108,7 +114,7 @@
EXPECT_EQ(buf_size, result);

// Check that the message came out the other end in the test sink
- // (messages are posted to we have to pump).
+ // (messages are posted, so we have to pump).
message_loop_.RunAllPending();
ASSERT_EQ(1u, sink_->message_count());
const IPC::Message* msg = sink_->GetMessageAt(0);

Modified: trunk/src/ipc/ipc_channel_posix.cc
==============================================================================
--- trunk/src/ipc/ipc_channel_posix.cc (original)
+++ trunk/src/ipc/ipc_channel_posix.cc Wed May 16 11:58:32 2012
@@ -1126,7 +1126,8 @@
}

void Channel::Close() {
- channel_impl_->Close();
+ if (channel_impl_)
+ channel_impl_->Close();
}

void Channel::set_listener(Listener* listener) {

Modified: trunk/src/ipc/ipc_channel_win.cc
==============================================================================
--- trunk/src/ipc/ipc_channel_win.cc (original)
+++ trunk/src/ipc/ipc_channel_win.cc Wed May 16 11:58:32 2012
@@ -462,7 +462,8 @@
}

void Channel::Close() {
- channel_impl_->Close();
+ if (channel_impl_)
+ channel_impl_->Close();
}

void Channel::set_listener(Listener* listener) {

Modified: trunk/src/tools/heapcheck/suppressions.txt
==============================================================================
--- trunk/src/tools/heapcheck/suppressions.txt (original)
+++ trunk/src/tools/heapcheck/suppressions.txt Wed May 16 11:58:32 2012
@@ -1842,44 +1842,6 @@
fun:history::HistoryBackendTest_DeleteAll_Test::TestBody
}
{
- bug_127954a
- Heapcheck:Leak
- fun:::NaClIPCAdapterTest::SetUp
-}
-{
- bug_127954b
- Heapcheck:Leak
- fun:__gnu_cxx::new_allocator::allocate
- fun:std::_Vector_base::_M_allocate
- fun:std::vector::_M_insert_aux
- fun:std::vector::push_back
- fun:IPC::TestSink::OnMessageReceived
- fun:IPC::TestSink::Send
- fun:NaClIPCAdapter::SendMessageOnIOThread
-}
-{
- bug_127954c
- Heapcheck:Leak
- fun:base::internal::WeakReferenceOwner::GetRef
- fun:base::SupportsWeakPtr::AsWeakPtr
- fun:*Iterator
- fun:IPC::TestSink::OnMessageReceived
- fun:IPC::TestSink::Send
- fun:NaClIPCAdapter::SendMessageOnIOThread
-}
-{
- bug_127954d
- Heapcheck:Leak
- fun:Pickle::Resize
- fun:Pickle::Pickle
- fun:*Message
- fun:__gnu_cxx::new_allocator::construct
- fun:std::vector::push_back
- fun:IPC::TestSink::OnMessageReceived
- fun:IPC::TestSink::Send
- fun:NaClIPCAdapter::SendMessageOnIOThread
-}
-{
bug_128145
Heapcheck:Leak
...

Modified: trunk/src/tools/valgrind/memcheck/suppressions.txt
==============================================================================
--- trunk/src/tools/valgrind/memcheck/suppressions.txt (original)
+++ trunk/src/tools/valgrind/memcheck/suppressions.txt Wed May 16 11:58:32 2012
@@ -5762,12 +5762,6 @@
fun:_ZN16RenderThreadImpl4InitEv
}
{
- bug_127954
- Memcheck:Leak
- fun:_Znw*
- fun:_ZN12_GLOBAL__N_118NaClIPCAdapterTest5SetUpEv
-}
-{
bug_128145a
Memcheck:Leak
fun:_Znw*