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

21 files changed, 152 insertions, 86 deletions

diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index b5b058a..8377cc8 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -100,6 +100,7 @@ class InitialLoadObserver::TabTime {
 InitialLoadObserver::InitialLoadObserver(size_t tab_count,
                                          AutomationProvider* automation)
     : automation_(automation->AsWeakPtr()),
+      crashed_tab_count_(0),
       outstanding_tab_count_(tab_count),
       init_time_(base::TimeTicks::Now()) {
   if (outstanding_tab_count_ > 0) {
@@ -107,6 +108,8 @@ InitialLoadObserver::InitialLoadObserver(size_t tab_count,
                    NotificationService::AllSources());
     registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
                    NotificationService::AllSources());
+    registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                   NotificationService::AllSources());
   }
 }
 
@@ -128,12 +131,37 @@ void InitialLoadObserver::Observe(int type,
         finished_tabs_.insert(source.map_key());
         iter->second.set_stop_time(base::TimeTicks::Now());
       }
-      if (outstanding_tab_count_ == finished_tabs_.size())
-        ConditionMet();
+    }
+  } else if (type == content::NOTIFICATION_RENDERER_PROCESS_CLOSED) {
+    base::TerminationStatus status =
+        Details<RenderProcessHost::RendererClosedDetails>(details)->status;
+    switch (status) {
+      case base::TERMINATION_STATUS_NORMAL_TERMINATION:
+        break;
+
+      case base::TERMINATION_STATUS_ABNORMAL_TERMINATION:
+      case base::TERMINATION_STATUS_PROCESS_WAS_KILLED:
+      case base::TERMINATION_STATUS_PROCESS_CRASHED:
+        crashed_tab_count_++;
+        break;
+
+      case base::TERMINATION_STATUS_STILL_RUNNING:
+        LOG(ERROR) << "Got RENDERER_PROCESS_CLOSED notification, "
+                   << "but the process is still running. We may miss further "
+                   << "crash notification, resulting in hangs.";
+        break;
+
+      default:
+        LOG(ERROR) << "Unhandled termination status " << status;
+        NOTREACHED();
+        break;
     }
   } else {
     NOTREACHED();
   }
+
+  if (finished_tabs_.size() + crashed_tab_count_ >= outstanding_tab_count_)
+    ConditionMet();
 }
 
 DictionaryValue* InitialLoadObserver::GetTimingInformation() const {
diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h
index 4aa04b0..c65d208 100644
--- a/chrome/browser/automation/automation_provider_observers.h
+++ b/chrome/browser/automation/automation_provider_observers.h
@@ -109,6 +109,7 @@ class InitialLoadObserver : public NotificationObserver {
   NotificationRegistrar registrar_;
 
   base::WeakPtr<AutomationProvider> automation_;
+  size_t crashed_tab_count_;
   size_t outstanding_tab_count_;
   base::TimeTicks init_time_;
   TabTimeMap loading_tabs_;
diff --git a/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc b/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
index 09a29a4..83d5283 100644
--- a/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
+++ b/chrome/browser/importer/firefox_importer_unittest_utils_mac.cc
@@ -7,6 +7,7 @@
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/importer/firefox_importer_utils.h"
@@ -30,7 +31,7 @@ const char kTestChannelID[] = "T1";
 // |handle| - On return, the process handle to use to communicate with the
 // child.
 bool LaunchNSSDecrypterChildProcess(const FilePath& nss_path,
-    const IPC::Channel& channel, base::ProcessHandle* handle) {
+    IPC::Channel* channel, base::ProcessHandle* handle) {
   CommandLine cl(*CommandLine::ForCurrentProcess());
   cl.AppendSwitchASCII(switches::kTestChildProcess, "NSSDecrypterChildProcess");
 
@@ -43,13 +44,13 @@ bool LaunchNSSDecrypterChildProcess(const FilePath& nss_path,
   dyld_override.second = nss_path.value();
   env.push_back(dyld_override);
 
-  base::file_handle_mapping_vector fds_to_map;
-  const int ipcfd = channel.GetClientFileDescriptor();
-  if (ipcfd > -1) {
-    fds_to_map.push_back(std::pair<int,int>(ipcfd, kPrimaryIPCChannel + 3));
-  } else {
+  int ipcfd = channel->TakeClientFileDescriptor();
+  if (ipcfd == -1)
     return false;
-  }
+
+  file_util::ScopedFD client_file_descriptor_closer(&ipcfd);
+  base::file_handle_mapping_vector fds_to_map;
+  fds_to_map.push_back(std::pair<int,int>(ipcfd, kPrimaryIPCChannel + 3));
 
   bool debug_on_start = CommandLine::ForCurrentProcess()->HasSwitch(
                             switches::kDebugChildren);
@@ -139,7 +140,7 @@ bool FFUnitTestDecryptorProxy::Setup(const FilePath& nss_path) {
 
   // Spawn child and set up sync IPC connection.
   bool ret = LaunchNSSDecrypterChildProcess(nss_path,
-                                            *(channel_.get()),
+                                            channel_.get(),
                                             &child_process_);
   return ret && (child_process_ != 0);
 }
diff --git a/chrome/common/logging_chrome_uitest.cc b/chrome/common/logging_chrome_uitest.cc
index b72661a..d4bbfa1 100644
--- a/chrome/common/logging_chrome_uitest.cc
+++ b/chrome/common/logging_chrome_uitest.cc
@@ -84,16 +84,14 @@ TEST_F(ChromeLoggingTest, EnvironmentLogFileName) {
 #endif
 
 #if !defined(NDEBUG)  // We don't have assertions in release builds.
-// Tests whether we correctly fail on browser assertions during tests.
+// Tests whether we correctly fail on renderer assertions during tests.
 class AssertionTest : public UITest {
  protected:
-  AssertionTest() : UITest() {
-    // Initial loads will never complete due to assertion.
+  AssertionTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
-    // We're testing the renderer rather than the browser assertion here,
-    // because the browser assertion would flunk the test during SetUp()
-    // (since TAU wouldn't be able to find the browser window).
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererAssertTest);
   }
 };
@@ -116,13 +114,11 @@ TEST_F(AssertionTest, Assertion) {
 // Only works on Linux in Release mode with CHROME_HEADLESS=1
 class CheckFalseTest : public UITest {
  protected:
-  CheckFalseTest() : UITest() {
-    // Initial loads will never complete due to assertion.
+  CheckFalseTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
-    // We're testing the renderer rather than the browser assertion here,
-    // because the browser assertion would flunk the test during SetUp()
-    // (since TAU wouldn't be able to find the browser window).
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererCheckFalseTest);
   }
 };
@@ -144,10 +140,11 @@ TEST_F(CheckFalseTest, CheckFails) {
 // Tests whether we correctly fail on browser crashes during UI Tests.
 class RendererCrashTest : public UITest {
  protected:
-  RendererCrashTest() : UITest() {
-    // Initial loads will never complete due to crash.
+  RendererCrashTest() {
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Make crash notifications on launch work on Win.
     wait_for_initial_loads_ = false;
-
+#endif
     launch_arguments_.AppendSwitch(switches::kRendererCrashTest);
   }
 };
diff --git a/chrome/test/automation/automation_proxy.cc b/chrome/test/automation/automation_proxy.cc
index 4704deb..10bcf6d 100644
--- a/chrome/test/automation/automation_proxy.cc
+++ b/chrome/test/automation/automation_proxy.cc
@@ -432,15 +432,9 @@ scoped_refptr<BrowserProxy> AutomationProxy::GetLastActiveBrowserWindow() {
   return ProxyObjectFromHandle<BrowserProxy>(handle);
 }
 
-#if defined(OS_POSIX)
-base::file_handle_mapping_vector AutomationProxy::fds_to_map() const {
-  base::file_handle_mapping_vector map;
-  const int ipcfd = channel_->GetClientFileDescriptor();
-  if (ipcfd > -1)
-    map.push_back(std::make_pair(ipcfd, kPrimaryIPCChannel + 3));
-  return map;
-}
-#endif  // defined(OS_POSIX)
+IPC::SyncChannel* AutomationProxy::channel() {
+  return channel_.get();
+}
 
 bool AutomationProxy::Send(IPC::Message* message) {
   return Send(message,
diff --git a/chrome/test/automation/automation_proxy.h b/chrome/test/automation/automation_proxy.h
index a41689d..0fccd90 100644
--- a/chrome/test/automation/automation_proxy.h
+++ b/chrome/test/automation/automation_proxy.h
@@ -222,9 +222,7 @@ class AutomationProxy : public IPC::Channel::Listener,
                             const std::string& password) WARN_UNUSED_RESULT;
 #endif
 
-#if defined(OS_POSIX)
-  base::file_handle_mapping_vector fds_to_map() const;
-#endif
+  IPC::SyncChannel* channel();
 
   // AutomationMessageSender implementation.
   virtual bool Send(IPC::Message* message) WARN_UNUSED_RESULT;
diff --git a/chrome/test/automation/proxy_launcher.cc b/chrome/test/automation/proxy_launcher.cc
index 34c4060..469a0f2 100644
--- a/chrome/test/automation/proxy_launcher.cc
+++ b/chrome/test/automation/proxy_launcher.cc
@@ -26,6 +26,7 @@
 #include "content/common/child_process_info.h"
 #include "content/common/debug_flags.h"
 #include "ipc/ipc_channel.h"
+#include "ipc/ipc_descriptors.h"
 #include "sql/connection.h"
 
 namespace {
@@ -109,11 +110,11 @@ bool ProxyLauncher::WaitForBrowserLaunch(bool wait_for_initial_loads) {
       return false;
     }
   } else {
-    // TODO(phajdan.jr): We should get rid of this sleep, but some tests
-    // "rely" on it, e.g. AssertionTest.Assertion and CheckFalseTest.CheckFails.
-    // Those tests do not wait in any way until the crash gets noticed,
-    // so it's possible for the browser to exit before the tested crash happens.
+#if defined(OS_WIN)
+    // TODO(phajdan.jr): Get rid of this Sleep when logging_chrome_uitest
+    // stops "relying" on it.
     base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms());
+#endif
   }
 
   if (!automation()->SetFilteredInet(ShouldFilterInet())) {
@@ -198,7 +199,7 @@ bool ProxyLauncher::LaunchBrowser(const LaunchState& state) {
   if (!state.setup_profile_callback.is_null())
     state.setup_profile_callback.Run();
 
-  if (!LaunchBrowserHelper(state, false, &process_)) {
+  if (!LaunchBrowserHelper(state, true, false, &process_)) {
     LOG(ERROR) << "LaunchBrowserHelper failed.";
     return false;
   }
@@ -210,7 +211,7 @@ bool ProxyLauncher::LaunchBrowser(const LaunchState& state) {
 #if !defined(OS_MACOSX)
 bool ProxyLauncher::LaunchAnotherBrowserBlockUntilClosed(
     const LaunchState& state) {
-  return LaunchBrowserHelper(state, true, NULL);
+  return LaunchBrowserHelper(state, false, true, NULL);
 }
 #endif
 
@@ -435,7 +436,9 @@ void ProxyLauncher::PrepareTestCommandline(CommandLine* command_line,
   command_line->AppendSwitch(switches::kUnlimitedQuotaForFiles);
 }
 
-bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
+bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state,
+                                        bool main_launch,
+                                        bool wait,
                                         base::ProcessHandle* process) {
   CommandLine command_line(state.command);
 
@@ -459,8 +462,8 @@ bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
             << browser_wrapper;
   }
 
-  // TODO(phajdan.jr): Only run it for "main" browser launch.
-  browser_launch_time_ = base::TimeTicks::Now();
+  if (main_launch)
+    browser_launch_time_ = base::TimeTicks::Now();
 
   base::LaunchOptions options;
   options.wait = wait;
@@ -468,10 +471,14 @@ bool ProxyLauncher::LaunchBrowserHelper(const LaunchState& state, bool wait,
 #if defined(OS_WIN)
   options.start_hidden = !state.show_window;
 #elif defined(OS_POSIX)
+  int ipcfd = -1;
+  file_util::ScopedFD ipcfd_closer(&ipcfd);
   base::file_handle_mapping_vector fds;
-  if (automation_proxy_.get())
-    fds = automation_proxy_->fds_to_map();
-  options.fds_to_remap = &fds;
+  if (main_launch && automation_proxy_.get()) {
+    ipcfd = automation_proxy_->channel()->TakeClientFileDescriptor();
+    fds.push_back(std::make_pair(ipcfd, kPrimaryIPCChannel + 3));
+    options.fds_to_remap = &fds;
+  }
 #endif
 
   return base::LaunchProcess(command_line, options, process);
diff --git a/chrome/test/automation/proxy_launcher.h b/chrome/test/automation/proxy_launcher.h
index 0e497b7..64f5b2b 100644
--- a/chrome/test/automation/proxy_launcher.h
+++ b/chrome/test/automation/proxy_launcher.h
@@ -166,6 +166,7 @@ class ProxyLauncher {
                               bool include_testing_id);
 
   bool LaunchBrowserHelper(const LaunchState& state,
+                           bool main_launch,
                            bool wait,
                            base::ProcessHandle* process) WARN_UNUSED_RESULT;
 
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);
   }
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)