Wait properly for renderer crashes

This replaces a Sleep in automation with a wait for renderer crash. It turns out that our IPC on POSIX had one loophole that caused it not to notice very early crashes, so I also fixed that.

The problem was that when the child process died before connecting to the parent's IPC channel, the parent wouldn't notice the crash because the child end of the IPC pipe was kept open for too long. This change makes the code close the child end of the pipe right after forking the child.

This might also help with automation not noticing the browser crash during initial launch, or at least should be a good step toward fixing that problem.

BUG=38497,90489

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

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

5 files changed, 54 insertions, 24 deletions

diff --git a/ipc/ipc_channel.h b/ipc/ipc_channel.h
index 8bb35c4..718f2b3 100644
--- a/ipc/ipc_channel.h
+++ b/ipc/ipc_channel.h
@@ -146,8 +146,14 @@ class IPC_EXPORT Channel : public Message::Sender {
   // On POSIX an IPC::Channel wraps a socketpair(), this method returns the
   // FD # for the client end of the socket.
   // This method may only be called on the server side of a channel.
+  // This method can be called on any thread.
   int GetClientFileDescriptor() const;
 
+  // Same as GetClientFileDescriptor, but transfers the ownership of the
+  // file descriptor to the caller.
+  // This method can be called on any thread.
+  int TakeClientFileDescriptor();
+
   // On POSIX an IPC::Channel can either wrap an established socket, or it
   // can wrap a socket that is listening for connections. Currently an
   // IPC::Channel that listens for connections can only accept one connection
diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
index a88506c..27c62f5 100644
--- a/ipc/ipc_channel_posix.cc
+++ b/ipc/ipc_channel_posix.cc
@@ -102,15 +102,9 @@ class PipeMap {
 
   // Remove the mapping for the given channel id. No error is signaled if the
   // channel_id doesn't exist
-  void RemoveAndClose(const std::string& channel_id) {
+  void Remove(const std::string& channel_id) {
     base::AutoLock locked(lock_);
-
-    ChannelToFDMap::iterator i = map_.find(channel_id);
-    if (i != map_.end()) {
-      if (HANDLE_EINTR(close(i->second)) < 0)
-        PLOG(ERROR) << "close " << channel_id;
-      map_.erase(i);
-    }
+    map_.erase(channel_id);
   }
 
   // Insert a mapping from @channel_id to @fd. It's a fatal error to insert a
@@ -403,7 +397,7 @@ bool Channel::ChannelImpl::CreatePipe(
         // Case 3 from comment above.
         // We only allow one connection.
         local_pipe = HANDLE_EINTR(dup(local_pipe));
-        PipeMap::GetInstance()->RemoveAndClose(pipe_name_);
+        PipeMap::GetInstance()->Remove(pipe_name_);
       } else {
         // Case 4a from comment above.
         // Guard against inappropriate reuse of the initial IPC channel.  If
@@ -426,6 +420,7 @@ bool Channel::ChannelImpl::CreatePipe(
         LOG(ERROR) << "Server already exists for " << pipe_name_;
         return false;
       }
+      base::AutoLock lock(client_pipe_lock_);
       if (!SocketPair(&local_pipe, &client_pipe_))
         return false;
       PipeMap::GetInstance()->Insert(pipe_name_, client_pipe_);
@@ -529,10 +524,7 @@ bool Channel::ChannelImpl::ProcessIncomingMessages() {
     }
     DCHECK(bytes_read);
 
-    if (client_pipe_ != -1) {
-      PipeMap::GetInstance()->RemoveAndClose(pipe_name_);
-      client_pipe_ = -1;
-    }
+    CloseClientFileDescriptor();
 
     // a pointer to an array of |num_wire_fds| file descriptors from the read
     const int* wire_fds = NULL;
@@ -916,10 +908,31 @@ bool Channel::ChannelImpl::Send(Message* message) {
   return true;
 }
 
-int Channel::ChannelImpl::GetClientFileDescriptor() const {
+int Channel::ChannelImpl::GetClientFileDescriptor() {
+  base::AutoLock lock(client_pipe_lock_);
   return client_pipe_;
 }
 
+int Channel::ChannelImpl::TakeClientFileDescriptor() {
+  base::AutoLock lock(client_pipe_lock_);
+  int fd = client_pipe_;
+  if (client_pipe_ != -1) {
+    PipeMap::GetInstance()->Remove(pipe_name_);
+    client_pipe_ = -1;
+  }
+  return fd;
+}
+
+void Channel::ChannelImpl::CloseClientFileDescriptor() {
+  base::AutoLock lock(client_pipe_lock_);
+  if (client_pipe_ != -1) {
+    PipeMap::GetInstance()->Remove(pipe_name_);
+    if (HANDLE_EINTR(close(client_pipe_)) < 0)
+      PLOG(ERROR) << "close " << pipe_name_;
+    client_pipe_ = -1;
+  }
+}
+
 bool Channel::ChannelImpl::AcceptsConnections() const {
   return server_listen_pipe_ != -1;
 }
@@ -1173,10 +1186,7 @@ void Channel::ChannelImpl::Close() {
     server_listen_connection_watcher_.StopWatchingFileDescriptor();
   }
 
-  if (client_pipe_ != -1) {
-    PipeMap::GetInstance()->RemoveAndClose(pipe_name_);
-    client_pipe_ = -1;
-  }
+  CloseClientFileDescriptor();
 }
 
 //------------------------------------------------------------------------------
@@ -1210,6 +1220,10 @@ int Channel::GetClientFileDescriptor() const {
   return channel_impl_->GetClientFileDescriptor();
 }
 
+int Channel::TakeClientFileDescriptor() {
+  return channel_impl_->TakeClientFileDescriptor();
+}
+
 bool Channel::AcceptsConnections() const {
   return channel_impl_->AcceptsConnections();
 }
diff --git a/ipc/ipc_channel_posix.h b/ipc/ipc_channel_posix.h
index 06c545c..e141998 100644
--- a/ipc/ipc_channel_posix.h
+++ b/ipc/ipc_channel_posix.h
@@ -57,7 +57,9 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   void Close();
   void set_listener(Listener* listener) { listener_ = listener; }
   bool Send(Message* message);
-  int GetClientFileDescriptor() const;
+  int GetClientFileDescriptor();
+  int TakeClientFileDescriptor();
+  void CloseClientFileDescriptor();
   bool AcceptsConnections() const;
   bool HasAcceptedConnection() const;
   bool GetClientEuid(uid_t* client_euid) const;
@@ -109,6 +111,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   // For a server, the client end of our socketpair() -- the other end of our
   // pipe_ that is passed to the client.
   int client_pipe_;
+  base::Lock client_pipe_lock_;  // Lock that protects |client_pipe_|.
 
 #if defined(IPC_USES_READWRITE)
   // Linux/BSD use a dedicated socketpair() for passing file descriptors.
diff --git a/ipc/ipc_channel_proxy.cc b/ipc/ipc_channel_proxy.cc
index dc990c2..fa4d69e 100644
--- a/ipc/ipc_channel_proxy.cc
+++ b/ipc/ipc_channel_proxy.cc
@@ -376,16 +376,22 @@ void ChannelProxy::ClearIPCMessageLoop() {
 #if defined(OS_POSIX) && !defined(OS_NACL)
 // See the TODO regarding lazy initialization of the channel in
 // ChannelProxy::Init().
-// We assume that IPC::Channel::GetClientFileDescriptorMapping() is thread-safe.
-int ChannelProxy::GetClientFileDescriptor() const {
-  Channel *channel = context_.get()->channel_.get();
+int ChannelProxy::GetClientFileDescriptor() {
+  Channel* channel = context_.get()->channel_.get();
   // Channel must have been created first.
   DCHECK(channel) << context_.get()->channel_id_;
   return channel->GetClientFileDescriptor();
 }
 
+int ChannelProxy::TakeClientFileDescriptor() {
+  Channel* channel = context_.get()->channel_.get();
+  // Channel must have been created first.
+  DCHECK(channel) << context_.get()->channel_id_;
+  return channel->TakeClientFileDescriptor();
+}
+
 bool ChannelProxy::GetClientEuid(uid_t* client_euid) const {
-  Channel *channel = context_.get()->channel_.get();
+  Channel* channel = context_.get()->channel_.get();
   // Channel must have been created first.
   DCHECK(channel) << context_.get()->channel_id_;
   return channel->GetClientEuid(client_euid);
diff --git a/ipc/ipc_channel_proxy.h b/ipc/ipc_channel_proxy.h
index 792e3d6..4e2ebbf 100644
--- a/ipc/ipc_channel_proxy.h
+++ b/ipc/ipc_channel_proxy.h
@@ -157,7 +157,8 @@ class IPC_EXPORT ChannelProxy : public Message::Sender {
 
 #if defined(OS_POSIX)
   // Calls through to the underlying channel's methods.
-  int GetClientFileDescriptor() const;
+  int GetClientFileDescriptor();
+  int TakeClientFileDescriptor();
   bool GetClientEuid(uid_t* client_euid) const;
 #endif  // defined(OS_POSIX)