diff options
author | kkania@chromium.org <kkania@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-16 01:06:46 +0000 |
---|---|---|
committer | kkania@chromium.org <kkania@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-16 01:06:46 +0000 |
commit | 952394afc0a0dc5e3e6a1f3c2e1c0fa99b7b681f (patch) | |
tree | 85d8246efdada076c5dda1b0a561d6c3322f37d1 /ipc | |
parent | 8bbba87862f0ce45234380cebf60e3ded5d88147 (diff) | |
download | chromium_src-952394afc0a0dc5e3e6a1f3c2e1c0fa99b7b681f.zip chromium_src-952394afc0a0dc5e3e6a1f3c2e1c0fa99b7b681f.tar.gz chromium_src-952394afc0a0dc5e3e6a1f3c2e1c0fa99b7b681f.tar.bz2 |
Allow proxy channels to be created without initializing the underlying channel.
This fixes a bug where a client needed to guarantee a message filter was in
place before any messages were received.
It also follows the style of not having constructors that do complex
initialization.
BUG=102894
TEST=none
Review URL: http://codereview.chromium.org/8417054
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@110229 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'ipc')
-rw-r--r-- | ipc/ipc_channel_proxy.cc | 20 | ||||
-rw-r--r-- | ipc/ipc_channel_proxy.h | 28 | ||||
-rw-r--r-- | ipc/ipc_sync_channel.cc | 33 | ||||
-rw-r--r-- | ipc/ipc_sync_channel.h | 13 | ||||
-rw-r--r-- | ipc/ipc_sync_channel_unittest.cc | 79 |
5 files changed, 139 insertions, 34 deletions
diff --git a/ipc/ipc_channel_proxy.cc b/ipc/ipc_channel_proxy.cc index 26d285a..fb365d6 100644 --- a/ipc/ipc_channel_proxy.cc +++ b/ipc/ipc_channel_proxy.cc @@ -285,18 +285,15 @@ ChannelProxy::ChannelProxy(const IPC::ChannelHandle& channel_handle, Channel::Listener* listener, base::MessageLoopProxy* ipc_thread) : context_(new Context(listener, ipc_thread)), - outgoing_message_filter_(NULL) { - Init(channel_handle, mode, ipc_thread, true); + outgoing_message_filter_(NULL), + did_init_(false) { + Init(channel_handle, mode, true); } -ChannelProxy::ChannelProxy(const IPC::ChannelHandle& channel_handle, - Channel::Mode mode, - base::MessageLoopProxy* ipc_thread, - Context* context, - bool create_pipe_now) +ChannelProxy::ChannelProxy(Context* context) : context_(context), - outgoing_message_filter_(NULL) { - Init(channel_handle, mode, ipc_thread, create_pipe_now); + outgoing_message_filter_(NULL), + did_init_(false) { } ChannelProxy::~ChannelProxy() { @@ -305,8 +302,8 @@ ChannelProxy::~ChannelProxy() { void ChannelProxy::Init(const IPC::ChannelHandle& channel_handle, Channel::Mode mode, - base::MessageLoopProxy* ipc_thread_loop, bool create_pipe_now) { + DCHECK(!did_init_); #if defined(OS_POSIX) // When we are creating a server on POSIX, we need its file descriptor // to be created immediately so that it can be accessed and passed @@ -332,6 +329,8 @@ void ChannelProxy::Init(const IPC::ChannelHandle& channel_handle, // complete initialization on the background thread context_->ipc_message_loop()->PostTask( FROM_HERE, base::Bind(&Context::OnChannelOpened, context_.get())); + + did_init_ = true; } void ChannelProxy::Close() { @@ -347,6 +346,7 @@ void ChannelProxy::Close() { } bool ChannelProxy::Send(Message* message) { + DCHECK(did_init_); if (outgoing_message_filter()) message = outgoing_message_filter()->Rewrite(message); diff --git a/ipc/ipc_channel_proxy.h b/ipc/ipc_channel_proxy.h index 4e2ebbf..521389f 100644 --- a/ipc/ipc_channel_proxy.h +++ b/ipc/ipc_channel_proxy.h @@ -120,8 +120,21 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { Channel::Listener* listener, base::MessageLoopProxy* ipc_thread_loop); + // Creates an uninitialized channel proxy. Init must be called to receive + // or send any messages. This two-step setup allows message filters to be + // added before any messages are sent or received. + ChannelProxy(Channel::Listener* listener, + base::MessageLoopProxy* ipc_thread_loop); + virtual ~ChannelProxy(); + // Initializes the channel proxy. Only call this once to initialize a channel + // proxy that was not initialized in its constructor. If create_pipe_now is + // true, the pipe is created synchronously. Otherwise it's created on the IO + // thread. + void Init(const IPC::ChannelHandle& channel_handle, Channel::Mode mode, + bool create_pipe_now); + // Close the IPC::Channel. This operation completes asynchronously, once the // background thread processes the command to close the channel. It is ok to // call this method multiple times. Redundant calls are ignored. @@ -165,13 +178,8 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { protected: class Context; // A subclass uses this constructor if it needs to add more information - // to the internal state. If create_pipe_now is true, the pipe is created - // immediately. Otherwise it's created on the IO thread. - ChannelProxy(const IPC::ChannelHandle& channel_handle, - Channel::Mode mode, - base::MessageLoopProxy* ipc_thread_loop, - Context* context, - bool create_pipe_now); + // to the internal state. + ChannelProxy(Context* context); // Used internally to hold state that is referenced on the IPC thread. class Context : public base::RefCountedThreadSafe<Context>, @@ -257,15 +265,15 @@ class IPC_EXPORT ChannelProxy : public Message::Sender { private: friend class SendTask; - void Init(const IPC::ChannelHandle& channel_handle, Channel::Mode mode, - base::MessageLoopProxy* ipc_thread_loop, bool create_pipe_now); - // By maintaining this indirection (ref-counted) to our internal state, we // can safely be destroyed while the background thread continues to do stuff // that involves this data. scoped_refptr<Context> context_; OutgoingMessageFilter* outgoing_message_filter_; + + // Whether the channel has been initialized. + bool did_init_; }; } // namespace IPC diff --git a/ipc/ipc_sync_channel.cc b/ipc/ipc_sync_channel.cc index 163c7ca..f3c5384 100644 --- a/ipc/ipc_sync_channel.cc +++ b/ipc/ipc_sync_channel.cc @@ -378,18 +378,19 @@ SyncChannel::SyncChannel( base::MessageLoopProxy* ipc_message_loop, bool create_pipe_now, WaitableEvent* shutdown_event) - : ChannelProxy( - channel_handle, mode, ipc_message_loop, - new SyncContext(listener, ipc_message_loop, shutdown_event), - create_pipe_now), + : ChannelProxy(new SyncContext(listener, ipc_message_loop, shutdown_event)), sync_messages_with_no_timeout_allowed_(true) { - // Ideally we only want to watch this object when running a nested message - // loop. However, we don't know when it exits if there's another nested - // message loop running under it or not, so we wouldn't know whether to - // stop or keep watching. So we always watch it, and create the event as - // manual reset since the object watcher might otherwise reset the event - // when we're doing a WaitMany. - dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(), this); + ChannelProxy::Init(channel_handle, mode, create_pipe_now); + StartWatching(); +} + +SyncChannel::SyncChannel( + Channel::Listener* listener, + base::MessageLoopProxy* ipc_message_loop, + WaitableEvent* shutdown_event) + : ChannelProxy(new SyncContext(listener, ipc_message_loop, shutdown_event)), + sync_messages_with_no_timeout_allowed_(true) { + StartWatching(); } SyncChannel::~SyncChannel() { @@ -514,4 +515,14 @@ void SyncChannel::OnWaitableEventSignaled(WaitableEvent* event) { sync_context()->DispatchMessages(); } +void SyncChannel::StartWatching() { + // Ideally we only want to watch this object when running a nested message + // loop. However, we don't know when it exits if there's another nested + // message loop running under it or not, so we wouldn't know whether to + // stop or keep watching. So we always watch it, and create the event as + // manual reset since the object watcher might otherwise reset the event + // when we're doing a WaitMany. + dispatch_watcher_.StartWatching(sync_context()->GetDispatchEvent(), this); +} + } // namespace IPC diff --git a/ipc/ipc_sync_channel.h b/ipc/ipc_sync_channel.h index e43933e..ae9a765 100644 --- a/ipc/ipc_sync_channel.h +++ b/ipc/ipc_sync_channel.h @@ -62,12 +62,22 @@ class SyncMessage; class IPC_EXPORT SyncChannel : public ChannelProxy, public base::WaitableEventWatcher::Delegate { public: + // Creates and initializes a sync channel. If create_pipe_now is specified, + // the channel will be initialized synchronously. SyncChannel(const IPC::ChannelHandle& channel_handle, Channel::Mode mode, Channel::Listener* listener, base::MessageLoopProxy* ipc_message_loop, bool create_pipe_now, base::WaitableEvent* shutdown_event); + + // Creates an uninitialized sync channel. Call ChannelProxy::Init to + // initialize the channel. This two-step setup allows message filters to be + // added before any messages are sent or received. + SyncChannel(Channel::Listener* listener, + base::MessageLoopProxy* ipc_message_loop, + base::WaitableEvent* shutdown_event); + virtual ~SyncChannel(); virtual bool Send(Message* message); @@ -186,6 +196,9 @@ class IPC_EXPORT SyncChannel : public ChannelProxy, // shuts down. static void WaitForReplyWithNestedMessageLoop(SyncContext* context); + // Starts the dispatch watcher. + void StartWatching(); + bool sync_messages_with_no_timeout_allowed_; // Used to signal events between the IPC and listener threads. diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc index 029e49c..6982b28 100644 --- a/ipc/ipc_sync_channel_unittest.cc +++ b/ipc/ipc_sync_channel_unittest.cc @@ -120,6 +120,7 @@ class Worker : public Channel::Listener, public Message::Sender { DCHECK_EQ(answer, (succeed ? 10 : 0)); return result; } + const std::string& channel_name() { return channel_name_; } Channel::Mode mode() { return mode_; } WaitableEvent* done_event() { return done_.get(); } WaitableEvent* shutdown_event() { return &shutdown_event_; } @@ -156,6 +157,12 @@ class Worker : public Channel::Listener, public Message::Sender { NOTREACHED(); } + virtual SyncChannel* CreateChannel() { + return new SyncChannel( + channel_name_, mode_, this, ipc_thread_.message_loop_proxy(), true, + &shutdown_event_); + } + base::Thread* ListenerThread() { return overrided_thread_ ? overrided_thread_ : &listener_thread_; } @@ -167,9 +174,7 @@ class Worker : public Channel::Listener, public Message::Sender { void OnStart() { // Link ipc_thread_, listener_thread_ and channel_ altogether. StartThread(&ipc_thread_, MessageLoop::TYPE_IO); - channel_.reset(new SyncChannel( - channel_name_, mode_, this, ipc_thread_.message_loop_proxy(), true, - &shutdown_event_)); + channel_.reset(CreateChannel()); channel_created_->Signal(); Run(); } @@ -311,6 +316,74 @@ TEST_F(IPCSyncChannelTest, Simple) { namespace { +// Worker classes which override how the sync channel is created to use the +// two-step initialization (calling the lightweight constructor and then +// ChannelProxy::Init separately) process. +class TwoStepServer : public Worker { + public: + explicit TwoStepServer(bool create_pipe_now) + : Worker(Channel::MODE_SERVER, "simpler_server"), + create_pipe_now_(create_pipe_now) { } + + void Run() { + SendAnswerToLife(false, base::kNoTimeout, true); + Done(); + } + + virtual SyncChannel* CreateChannel() { + SyncChannel* channel = new SyncChannel( + this, ipc_thread().message_loop_proxy(), shutdown_event()); + channel->Init(channel_name(), mode(), create_pipe_now_); + return channel; + } + + bool create_pipe_now_; +}; + +class TwoStepClient : public Worker { + public: + TwoStepClient(bool create_pipe_now) + : Worker(Channel::MODE_CLIENT, "simple_client"), + create_pipe_now_(create_pipe_now) { } + + void OnAnswer(int* answer) { + *answer = 42; + Done(); + } + + virtual SyncChannel* CreateChannel() { + SyncChannel* channel = new SyncChannel( + this, ipc_thread().message_loop_proxy(), shutdown_event()); + channel->Init(channel_name(), mode(), create_pipe_now_); + return channel; + } + + bool create_pipe_now_; +}; + +void TwoStep(bool create_server_pipe_now, bool create_client_pipe_now) { + std::vector<Worker*> workers; + workers.push_back(new TwoStepServer(create_server_pipe_now)); + workers.push_back(new TwoStepClient(create_client_pipe_now)); + RunTest(workers); +} + +} // namespace + +// Tests basic two-step initialization, where you call the lightweight +// constructor then Init. +TEST_F(IPCSyncChannelTest, TwoStepInitialization) { + TwoStep(false, false); + TwoStep(false, true); + TwoStep(true, false); + TwoStep(true, true); +} + + +//----------------------------------------------------------------------------- + +namespace { + class DelayClient : public Worker { public: DelayClient() : Worker(Channel::MODE_CLIENT, "delay_client") { } |