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

8 files changed, 21 insertions, 12 deletions

diff --git a/content/browser/browser_child_process_host.cc b/content/browser/browser_child_process_host.cc
index 184ac82..d3aaac7 100644
--- a/content/browser/browser_child_process_host.cc
+++ b/content/browser/browser_child_process_host.cc
@@ -94,7 +94,7 @@ void BrowserChildProcessHost::Launch(
 #elif defined(OS_POSIX)
       use_zygote,
       environ,
-      channel()->GetClientFileDescriptor(),
+      channel()->TakeClientFileDescriptor(),
 #endif
       cmd_line,
       &client_));
diff --git a/content/browser/child_process_launcher.cc b/content/browser/child_process_launcher.cc
index c3d32a0..e438989 100644
--- a/content/browser/child_process_launcher.cc
+++ b/content/browser/child_process_launcher.cc
@@ -7,6 +7,7 @@
 #include <utility>  // For std::pair.
 
 #include "base/command_line.h"
+#include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
@@ -115,10 +116,14 @@ class ChildProcessLauncher::Context
 #endif
       CommandLine* cmd_line) {
     scoped_ptr<CommandLine> cmd_line_deleter(cmd_line);
+
     base::ProcessHandle handle = base::kNullProcessHandle;
 #if defined(OS_WIN)
     handle = sandbox::StartProcessWithAccess(cmd_line, exposed_dir);
 #elif defined(OS_POSIX)
+    // We need to close the client end of the IPC channel
+    // to reliably detect child termination.
+    file_util::ScopedFD ipcfd_closer(&ipcfd);
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
     // On Linux, we need to add some extra file descriptors for crash handling.
diff --git a/content/browser/renderer_host/browser_render_process_host.cc b/content/browser/renderer_host/browser_render_process_host.cc
index 8c3034c..a500892 100644
--- a/content/browser/renderer_host/browser_render_process_host.cc
+++ b/content/browser/renderer_host/browser_render_process_host.cc
@@ -330,7 +330,7 @@ bool BrowserRenderProcessHost::Init(bool is_accessibility_enabled) {
 #elif defined(OS_POSIX)
         renderer_prefix.empty(),
         base::environment_vector(),
-        channel_->GetClientFileDescriptor(),
+        channel_->TakeClientFileDescriptor(),
 #endif
         cmd_line,
         this));
@@ -940,6 +940,7 @@ void BrowserRenderProcessHost::OnProcessLaunched() {
       OnChannelError();
       return;
     }
+
     child_process_launcher_->SetProcessBackgrounded(backgrounded_);
   }
 
diff --git a/content/common/gpu/gpu_channel.cc b/content/common/gpu/gpu_channel.cc
index 0ad31501..e51d220 100644
--- a/content/common/gpu/gpu_channel.cc
+++ b/content/common/gpu/gpu_channel.cc
@@ -396,11 +396,11 @@ std::string GpuChannel::GetChannelName() {
 }
 
 #if defined(OS_POSIX)
-int GpuChannel::GetRendererFileDescriptor() {
-  int fd = -1;
-  if (channel_.get()) {
-    fd = channel_->GetClientFileDescriptor();
+int GpuChannel::TakeRendererFileDescriptor() {
+  if (!channel_.get()) {
+    NOTREACHED();
+    return -1;
   }
-  return fd;
+  return channel_->TakeClientFileDescriptor();
 }
 #endif  // defined(OS_POSIX)
diff --git a/content/common/gpu/gpu_channel.h b/content/common/gpu/gpu_channel.h
index b6a510f..d51c0bc 100644
--- a/content/common/gpu/gpu_channel.h
+++ b/content/common/gpu/gpu_channel.h
@@ -62,7 +62,7 @@ class GpuChannel : public IPC::Channel::Listener,
   std::string GetChannelName();
 
 #if defined(OS_POSIX)
-  int GetRendererFileDescriptor();
+  int TakeRendererFileDescriptor();
 #endif  // defined(OS_POSIX)
 
   base::ProcessHandle renderer_process() const {
diff --git a/content/common/gpu/gpu_channel_manager.cc b/content/common/gpu/gpu_channel_manager.cc
index 900e6eb..5d3aba8 100644
--- a/content/common/gpu/gpu_channel_manager.cc
+++ b/content/common/gpu/gpu_channel_manager.cc
@@ -102,9 +102,9 @@ void GpuChannelManager::OnEstablishChannel(int renderer_id) {
 #if defined(OS_POSIX)
     // On POSIX, pass the renderer-side FD. Also mark it as auto-close so
     // that it gets closed after it has been sent.
-    int renderer_fd = channel->GetRendererFileDescriptor();
+    int renderer_fd = channel->TakeRendererFileDescriptor();
     DCHECK_NE(-1, renderer_fd);
-    channel_handle.socket = base::FileDescriptor(dup(renderer_fd), true);
+    channel_handle.socket = base::FileDescriptor(renderer_fd, true);
 #endif
   }
 
diff --git a/content/plugin/plugin_channel.h b/content/plugin/plugin_channel.h
index 4439920..6a5bb74 100644
--- a/content/plugin/plugin_channel.h
+++ b/content/plugin/plugin_channel.h
@@ -51,7 +51,9 @@ class PluginChannel : public PluginChannelBase {
   void set_incognito(bool value) { incognito_ = value; }
 
 #if defined(OS_POSIX)
-  int renderer_fd() const { return channel_->GetClientFileDescriptor(); }
+  int TakeRendererFileDescriptor() {
+    return channel_->TakeClientFileDescriptor();
+  }
 #endif
 
  protected:
diff --git a/content/plugin/plugin_thread.cc b/content/plugin/plugin_thread.cc
index 6bd9b90..75252c6 100644
--- a/content/plugin/plugin_thread.cc
+++ b/content/plugin/plugin_thread.cc
@@ -159,7 +159,8 @@ void PluginThread::OnCreateChannel(int renderer_id,
     channel_handle.name = channel->channel_handle().name;
 #if defined(OS_POSIX)
     // On POSIX, pass the renderer-side FD.
-    channel_handle.socket = base::FileDescriptor(channel->renderer_fd(), false);
+    channel_handle.socket =
+        base::FileDescriptor(channel->TakeRendererFileDescriptor(), true);
 #endif
     channel->set_incognito(incognito);
   }