Fix posix IPC channel hanging problem.

If a channel closes right before a send call, listeners might not be notified of
the problem, which can cause hangs. This CL fixes that and adds a test that makes
sure that this does not happen in the future.

This is similar to cl/150893002, but takes a slightly different approach to how to
make sure everything happens in the right order. In particular, it avoids closing
the socket (and calling OnChannelError()) from Send().

BUG=338709

Review URL: https://codereview.chromium.org/172773002

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

2 files changed, 61 insertions, 10 deletions

diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
index 8788532..2bd0989 100644
--- a/ipc/ipc_channel_posix.cc
+++ b/ipc/ipc_channel_posix.cc
@@ -327,7 +327,7 @@ bool Channel::ChannelImpl::CreatePipe(
 
 bool Channel::ChannelImpl::Connect() {
   if (server_listen_pipe_ == -1 && pipe_ == -1) {
-    DLOG(INFO) << "Channel creation failed: " << pipe_name_;
+    DLOG(WARNING) << "Channel creation failed: " << pipe_name_;
     return false;
   }
 
@@ -464,15 +464,19 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages() {
       CloseFileDescriptors(msg);
 
     if (bytes_written < 0 && !SocketWriteErrorIsRecoverable()) {
+      // We can't close the pipe here, because calling OnChannelError
+      // may destroy this object, and that would be bad if we are
+      // called from Send(). Instead, we return false and hope the
+      // caller will close the pipe. If they do not, the pipe will
+      // still be closed next time OnFileCanReadWithoutBlocking is
+      // called.
 #if defined(OS_MACOSX)
       // On OSX writing to a pipe with no listener returns EPERM.
       if (errno == EPERM) {
-        Close();
         return false;
       }
 #endif  // OS_MACOSX
       if (errno == EPIPE) {
-        Close();
         return false;
       }
       PLOG(ERROR) << "pipe error on "
@@ -680,7 +684,7 @@ void Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int fd) {
   // If we're a server and handshaking, then we want to make sure that we
   // only send our handshake message after we've processed the client's.
   // This gives us a chance to kill the client if the incoming handshake
-  // is invalid. This also flushes any closefd messagse.
+  // is invalid. This also flushes any closefd messages.
   if (!is_blocked_on_write_) {
     if (!ProcessOutgoingMessages()) {
       ClosePipeOnError();
diff --git a/ipc/ipc_channel_posix_unittest.cc b/ipc/ipc_channel_posix_unittest.cc
index dbd854e..5157a01 100644
--- a/ipc/ipc_channel_posix_unittest.cc
+++ b/ipc/ipc_channel_posix_unittest.cc
@@ -41,7 +41,9 @@ class IPCChannelPosixTestListener : public IPC::Listener {
   };
 
   IPCChannelPosixTestListener(bool quit_only_on_message)
-      : status_(DISCONNECTED), quit_only_on_message_(quit_only_on_message) {}
+      : status_(DISCONNECTED),
+        quit_only_on_message_(quit_only_on_message) {
+  }
 
   virtual ~IPCChannelPosixTestListener() {}
 
@@ -61,9 +63,7 @@ class IPCChannelPosixTestListener : public IPC::Listener {
 
   virtual void OnChannelError() OVERRIDE {
     status_ = CHANNEL_ERROR;
-    if (!quit_only_on_message_) {
-      QuitRunLoop();
-    }
+    QuitRunLoop();
   }
 
   virtual void OnChannelDenied() OVERRIDE {
@@ -83,7 +83,13 @@ class IPCChannelPosixTestListener : public IPC::Listener {
   STATUS status() { return status_; }
 
   void QuitRunLoop() {
-    base::MessageLoopForIO::current()->QuitNow();
+    base::MessageLoopForIO* loop = base::MessageLoopForIO::current();
+    if (loop->is_running()) {
+      loop->QuitNow();
+    } else {
+      // Die as soon as Run is called.
+      loop->PostTask(FROM_HERE, loop->QuitClosure());
+    }
   }
 
  private:
@@ -186,7 +192,7 @@ void IPCChannelPosixTest::SpinRunLoop(base::TimeDelta delay) {
   // in the case of a bad test. Usually, the run loop will quit sooner than
   // that because all tests use a IPCChannelPosixTestListener which quits the
   // current run loop on any channel activity.
-  loop->PostDelayedTask(FROM_HERE, base::MessageLoop::QuitClosure(), delay);
+  loop->PostDelayedTask(FROM_HERE, loop->QuitClosure(), delay);
   loop->Run();
 }
 
@@ -228,6 +234,47 @@ TEST_F(IPCChannelPosixTest, BasicConnected) {
   ASSERT_FALSE(channel2.AcceptsConnections());
 }
 
+// If a connection closes right before a Send() call, we may end up closing
+// the connection without notifying the listener, which can cause hangs in
+// sync_message_filter and others. Make sure the listener is notified.
+TEST_F(IPCChannelPosixTest, SendHangTest) {
+  IPCChannelPosixTestListener out_listener(true);
+  IPCChannelPosixTestListener in_listener(true);
+  IPC::ChannelHandle in_handle("IN");
+  IPC::Channel in_chan(in_handle, IPC::Channel::MODE_SERVER, &in_listener);
+  base::FileDescriptor out_fd(in_chan.TakeClientFileDescriptor(), false);
+  IPC::ChannelHandle out_handle("OUT", out_fd);
+  IPC::Channel out_chan(out_handle, IPC::Channel::MODE_CLIENT, &out_listener);
+  ASSERT_TRUE(in_chan.Connect());
+  ASSERT_TRUE(out_chan.Connect());
+  in_chan.Close();  // simulate remote process dying at an unfortunate time.
+  // Send will fail, because it cannot write the message.
+  ASSERT_FALSE(out_chan.Send(new IPC::Message(
+      0,  // routing_id
+      kQuitMessage,  // message type
+      IPC::Message::PRIORITY_NORMAL)));
+  SpinRunLoop(TestTimeouts::action_max_timeout());
+  ASSERT_EQ(IPCChannelPosixTestListener::CHANNEL_ERROR, out_listener.status());
+}
+
+// If a connection closes right before a Connect() call, we may end up closing
+// the connection without notifying the listener, which can cause hangs in
+// sync_message_filter and others. Make sure the listener is notified.
+TEST_F(IPCChannelPosixTest, AcceptHangTest) {
+  IPCChannelPosixTestListener out_listener(true);
+  IPCChannelPosixTestListener in_listener(true);
+  IPC::ChannelHandle in_handle("IN");
+  IPC::Channel in_chan(in_handle, IPC::Channel::MODE_SERVER, &in_listener);
+  base::FileDescriptor out_fd(in_chan.TakeClientFileDescriptor(), false);
+  IPC::ChannelHandle out_handle("OUT", out_fd);
+  IPC::Channel out_chan(out_handle, IPC::Channel::MODE_CLIENT, &out_listener);
+  ASSERT_TRUE(in_chan.Connect());
+  in_chan.Close();  // simulate remote process dying at an unfortunate time.
+  ASSERT_FALSE(out_chan.Connect());
+  SpinRunLoop(TestTimeouts::action_max_timeout());
+  ASSERT_EQ(IPCChannelPosixTestListener::CHANNEL_ERROR, out_listener.status());
+}
+
 TEST_F(IPCChannelPosixTest, AdvancedConnected) {
   // Test creating a connection to an external process.
   IPCChannelPosixTestListener listener(false);