Ensure that a listener's OnChannelConnected is called before OnMessageReceived.

A race condition existed since the listener thread could dispatch messages in two ways: a task being posted to it, or because the dispatch messages event is set.  The latter is needed to avoid deadlock when multiple SyncChannels are used on the same listener thread.  If the latter method was used to dispatch a message, it might occur before the task that calls OnChannelConnected gets run.

The fix is to call OnChannelConnected manually if we notice a message is being dispatched before it's called.

Note: I couldn't think of a way to test this since it depends on a race condition in the listener message looop (whether the task or objectwatcher gets run first).

Review URL: http://codereview.chromium.org/42113

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@11522 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 20 insertions, 22 deletions

diff --git a/chrome/common/ipc_channel_proxy.cc b/chrome/common/ipc_channel_proxy.cc
index 360ead1..a9def01 100644
--- a/chrome/common/ipc_channel_proxy.cc
+++ b/chrome/common/ipc_channel_proxy.cc
@@ -18,7 +18,9 @@ ChannelProxy::Context::Context(Channel::Listener* listener,
     : listener_message_loop_(MessageLoop::current()),
       listener_(listener),
       ipc_message_loop_(ipc_message_loop),
-      channel_(NULL) {
+      channel_(NULL),
+      peer_pid_(0),
+      channel_connected_called_(false) {
   if (filter)
     filters_.push_back(filter);
 }
@@ -68,12 +70,13 @@ void ChannelProxy::Context::OnMessageReceivedNoFilter(const Message& message) {
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) {
+  peer_pid_ = peer_pid;
   for (size_t i = 0; i < filters_.size(); ++i)
     filters_[i]->OnChannelConnected(peer_pid);
 
   // See above comment about using listener_message_loop_ here.
   listener_message_loop_->PostTask(FROM_HERE, NewRunnableMethod(
-      this, &Context::OnDispatchConnected, peer_pid));
+      this, &Context::OnDispatchConnected));
 }
 
 // Called on the IPC::Channel thread
@@ -160,6 +163,8 @@ void ChannelProxy::Context::OnDispatchMessage(const Message& message) {
   if (!listener_)
     return;
 
+  OnDispatchConnected();
+
 #ifdef IPC_MESSAGE_LOG_ENABLED
   Logging* logger = Logging::current();
   if (message.type() == IPC_LOGGING_ID) {
@@ -180,9 +185,13 @@ void ChannelProxy::Context::OnDispatchMessage(const Message& message) {
 }
 
 // Called on the listener's thread
-void ChannelProxy::Context::OnDispatchConnected(int32 peer_pid) {
+void ChannelProxy::Context::OnDispatchConnected() {
+  if (channel_connected_called_)
+    return;
+
+  channel_connected_called_ = true;
   if (listener_)
-    listener_->OnChannelConnected(peer_pid);
+    listener_->OnChannelConnected(peer_pid_);
 }
 
 // Called on the listener's thread
diff --git a/chrome/common/ipc_channel_proxy.h b/chrome/common/ipc_channel_proxy.h
index 55d560e..80a9807 100644
--- a/chrome/common/ipc_channel_proxy.h
+++ b/chrome/common/ipc_channel_proxy.h
@@ -138,9 +138,11 @@ class ChannelProxy : public Message::Sender {
             MessageLoop* ipc_thread);
     virtual ~Context() { }
     MessageLoop* ipc_message_loop() const { return ipc_message_loop_; }
-    Channel::Listener* listener() const { return listener_; }
     const std::wstring& channel_id() const { return channel_id_; }
 
+    // Dispatches a message on the listener thread.
+    void OnDispatchMessage(const Message& message);
+
    protected:
     // IPC::Channel::Listener methods:
     virtual void OnMessageReceived(const Message& message);
@@ -172,8 +174,7 @@ class ChannelProxy : public Message::Sender {
     void OnSendMessage(Message* message_ptr);
     void OnAddFilter(MessageFilter* filter);
     void OnRemoveFilter(MessageFilter* filter);
-    void OnDispatchMessage(const Message& message);
-    void OnDispatchConnected(int32 peer_pid);
+    void OnDispatchConnected();
     void OnDispatchError();
 
     MessageLoop* listener_message_loop_;
@@ -184,6 +185,8 @@ class ChannelProxy : public Message::Sender {
     MessageLoop* ipc_message_loop_;
     Channel* channel_;
     std::wstring channel_id_;
+    int peer_pid_;
+    bool channel_connected_called_;
   };
 
   Context* context() { return context_; }
diff --git a/chrome/common/ipc_sync_channel.cc b/chrome/common/ipc_sync_channel.cc
index 307c1e6..842678c 100644
--- a/chrome/common/ipc_sync_channel.cc
+++ b/chrome/common/ipc_sync_channel.cc
@@ -10,7 +10,6 @@
 #include "base/message_loop.h"
 #include "base/waitable_event.h"
 #include "base/waitable_event_watcher.h"
-#include "chrome/common/ipc_logging.h"
 #include "chrome/common/ipc_sync_message.h"
 
 using base::TimeDelta;
@@ -104,20 +103,7 @@ class SyncChannel::ReceivedSyncMsgQueue :
         message_queue_.pop_front();
       }
 
-#ifdef IPC_MESSAGE_LOG_ENABLED
-      Logging* logger = Logging::current();
-      if (logger->Enabled())
-        logger->OnPreDispatchMessage(*message);
-#endif
-
-      if (context->listener())
-        context->listener()->OnMessageReceived(*message);
-
-#ifdef IPC_MESSAGE_LOG_ENABLED
-      if (logger->Enabled())
-        logger->OnPostDispatchMessage(*message, context->channel_id());
-#endif
-
+      context->OnDispatchMessage(*message);
       delete message;
     }
   }