From f729d15a933e28cfc000953b75535acc90c3aa5e Mon Sep 17 00:00:00 2001 From: "rsleevi@chromium.org" Date: Sat, 28 Apr 2012 02:12:00 +0000 Subject: [PATCH] RefCounted types should not have public destructors, ipc/ edition BUG=123295 TEST=none Review URL: http://codereview.chromium.org/10008108 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@134421 0039d316-1c4b-4281-b951-d872f2087c98 --- ipc/ipc_channel_proxy.cc | 34 ++++---------------- ipc/ipc_channel_proxy.h | 11 +++++-- ipc/ipc_sync_channel_unittest.cc | 3 ++ ipc/ipc_sync_message_filter.cc | 54 ++++++++++++++++---------------- ipc/ipc_sync_message_filter.h | 6 ++-- 5 files changed, 49 insertions(+), 59 deletions(-) diff --git a/ipc/ipc_channel_proxy.cc b/ipc/ipc_channel_proxy.cc index 7eed1bd8103e29..f0e97152ffa025 100644 --- a/ipc/ipc_channel_proxy.cc +++ b/ipc/ipc_channel_proxy.cc @@ -14,33 +14,10 @@ namespace IPC { -// This helper ensures the message is deleted if the task is deleted without -// having been run. -class SendCallbackHelper - : public base::RefCountedThreadSafe { - public: - SendCallbackHelper(ChannelProxy::Context* context, Message* message) - : context_(context), - message_(message) { - } - - void Send() { - context_->OnSendMessage(message_.release()); - } - - private: - scoped_refptr context_; - scoped_ptr message_; - - DISALLOW_COPY_AND_ASSIGN(SendCallbackHelper); -}; - //------------------------------------------------------------------------------ ChannelProxy::MessageFilter::MessageFilter() {} -ChannelProxy::MessageFilter::~MessageFilter() {} - void ChannelProxy::MessageFilter::OnFilterAdded(Channel* channel) {} void ChannelProxy::MessageFilter::OnFilterRemoved() {} @@ -59,6 +36,8 @@ void ChannelProxy::MessageFilter::OnDestruct() const { delete this; } +ChannelProxy::MessageFilter::~MessageFilter() {} + //------------------------------------------------------------------------------ ChannelProxy::Context::Context(Channel::Listener* listener, @@ -186,13 +165,12 @@ void ChannelProxy::Context::OnChannelClosed() { } // Called on the IPC::Channel thread -void ChannelProxy::Context::OnSendMessage(Message* message) { +void ChannelProxy::Context::OnSendMessage(scoped_ptr message) { if (!channel_.get()) { - delete message; OnChannelClosed(); return; } - if (!channel_->Send(message)) + if (!channel_->Send(message.release())) OnChannelError(); } @@ -368,8 +346,8 @@ bool ChannelProxy::Send(Message* message) { context_->ipc_message_loop()->PostTask( FROM_HERE, - base::Bind(&SendCallbackHelper::Send, - new SendCallbackHelper(context_.get(), message))); + base::Bind(&ChannelProxy::Context::OnSendMessage, + context_, base::Passed(scoped_ptr(message)))); return true; } diff --git a/ipc/ipc_channel_proxy.h b/ipc/ipc_channel_proxy.h index 5cbe664151940c..8c0031a01f8e7d 100644 --- a/ipc/ipc_channel_proxy.h +++ b/ipc/ipc_channel_proxy.h @@ -58,7 +58,6 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { : public base::RefCountedThreadSafe { public: MessageFilter(); - virtual ~MessageFilter(); // Called on the background thread to provide the filter with access to the // channel. Called when the IPC channel is initialized or when AddFilter @@ -90,6 +89,13 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { // derived classes the option of controlling which thread they're deleted // on etc. virtual void OnDestruct() const; + + protected: + virtual ~MessageFilter(); + + private: + friend class base::RefCountedThreadSafe; }; struct MessageFilterTraits { @@ -98,6 +104,7 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { } }; + // Interface for a filter to be imposed on outgoing messages which can // re-write the message. Used mainly for testing. class OutgoingMessageFilter { @@ -233,7 +240,7 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { const Channel::Mode& mode); // Methods called on the IO thread. - void OnSendMessage(Message* message_ptr); + void OnSendMessage(scoped_ptr message_ptr); void OnAddFilter(); void OnRemoveFilter(MessageFilter* filter); diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc index 0424b2fb17d667..7f0a7ab1b8c1b5 100644 --- a/ipc/ipc_sync_channel_unittest.cc +++ b/ipc/ipc_sync_channel_unittest.cc @@ -1110,6 +1110,9 @@ class TestSyncMessageFilter : public SyncMessageFilter { worker_->Done(); } + private: + virtual ~TestSyncMessageFilter() {} + Worker* worker_; base::Thread thread_; }; diff --git a/ipc/ipc_sync_message_filter.cc b/ipc/ipc_sync_message_filter.cc index e49ebd640cb8ef..347a68c4c48cf3 100644 --- a/ipc/ipc_sync_message_filter.cc +++ b/ipc/ipc_sync_message_filter.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -21,9 +21,6 @@ SyncMessageFilter::SyncMessageFilter(base::WaitableEvent* shutdown_event) shutdown_event_(shutdown_event) { } -SyncMessageFilter::~SyncMessageFilter() { -} - bool SyncMessageFilter::Send(Message* message) { { base::AutoLock auto_lock(lock_); @@ -69,29 +66,6 @@ bool SyncMessageFilter::Send(Message* message) { return pending_message.send_result; } -void SyncMessageFilter::SendOnIOThread(Message* message) { - if (channel_) { - channel_->Send(message); - return; - } - - if (message->is_sync()) { - // We don't know which thread sent it, but it doesn't matter, just signal - // them all. - SignalAllEvents(); - } - - delete message; -} - -void SyncMessageFilter::SignalAllEvents() { - base::AutoLock auto_lock(lock_); - for (PendingSyncMessages::iterator iter = pending_sync_messages_.begin(); - iter != pending_sync_messages_.end(); ++iter) { - (*iter)->done_event->Signal(); - } -} - void SyncMessageFilter::OnFilterAdded(Channel* channel) { channel_ = channel; base::AutoLock auto_lock(lock_); @@ -125,4 +99,30 @@ bool SyncMessageFilter::OnMessageReceived(const Message& message) { return false; } +SyncMessageFilter::~SyncMessageFilter() { +} + +void SyncMessageFilter::SendOnIOThread(Message* message) { + if (channel_) { + channel_->Send(message); + return; + } + + if (message->is_sync()) { + // We don't know which thread sent it, but it doesn't matter, just signal + // them all. + SignalAllEvents(); + } + + delete message; +} + +void SyncMessageFilter::SignalAllEvents() { + base::AutoLock auto_lock(lock_); + for (PendingSyncMessages::iterator iter = pending_sync_messages_.begin(); + iter != pending_sync_messages_.end(); ++iter) { + (*iter)->done_event->Signal(); + } +} + } // namespace IPC diff --git a/ipc/ipc_sync_message_filter.h b/ipc/ipc_sync_message_filter.h index 1f0f46d86c72c9..a4c27a269a316b 100644 --- a/ipc/ipc_sync_message_filter.h +++ b/ipc/ipc_sync_message_filter.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -30,7 +30,6 @@ class IPC_EXPORT SyncMessageFilter : public ChannelProxy::MessageFilter, public Message::Sender { public: explicit SyncMessageFilter(base::WaitableEvent* shutdown_event); - virtual ~SyncMessageFilter(); // Message::Sender implementation. virtual bool Send(Message* message) OVERRIDE; @@ -41,6 +40,9 @@ class IPC_EXPORT SyncMessageFilter : public ChannelProxy::MessageFilter, virtual void OnChannelClosing() OVERRIDE; virtual bool OnMessageReceived(const Message& message) OVERRIDE; + protected: + virtual ~SyncMessageFilter(); + private: void SendOnIOThread(Message* message); // Signal all the pending sends as done, used in an error condition.