Linux: tear down Zygote at browser shutdown.

content/browser/zygote_host/zygote_host_impl_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/zygote_host/zygote_host_impl_linux.h"
 
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
@@ skipping 34 matching lines @@
 
 namespace content {
 
 // static
 ZygoteHost* ZygoteHost::GetInstance() {
   return ZygoteHostImpl::GetInstance();
 }
 
 ZygoteHostImpl::ZygoteHostImpl()
     : control_fd_(-1),
+      control_lock_(),
       pid_(-1),
       init_(false),
       using_suid_sandbox_(false),
+      sandbox_binary_(),
       have_read_sandbox_status_word_(false),
-      sandbox_status_(0) {}
+      sandbox_status_(0),
+      child_tracking_lock_(),
+      list_of_running_zygote_children_(),
+      should_teardown_after_last_child_exits_(false) {}
 
-ZygoteHostImpl::~ZygoteHostImpl() {
-  if (init_)
-    close(control_fd_);
-}
+ZygoteHostImpl::~ZygoteHostImpl() { TearDown(); }
 
 // static
 ZygoteHostImpl* ZygoteHostImpl::GetInstance() {
   return Singleton<ZygoteHostImpl>::get();
 }
 
 void ZygoteHostImpl::Init(const std::string& sandbox_cmd) {
   DCHECK(!init_);
   init_ = true;
 
@@ skipping 135 matching lines @@
   close(fds[1]);
   control_fd_ = fds[0];
 
   Pickle pickle;
   pickle.WriteInt(kZygoteCommandGetSandboxStatus);
   if (!SendMessage(pickle, NULL))
     LOG(FATAL) << "Cannot communicate with zygote";
   // We don't wait for the reply. We'll read it in ReadReply.
 }
 
+void ZygoteHostImpl::TearDownAfterLastChild() {
+  base::AutoLock lock(child_tracking_lock_);
+  should_teardown_after_last_child_exits_ = true;
+  if (list_of_running_zygote_children_.empty()) {
+    TearDown();

[piman, 2014/01/28 22:38:32]

Can we release the lock before calling TearDown?
The lock would only protect the set:

bool has_no_children = false;
{
  base::AutoLock lock(child_tracking_lock_);
  should_teardown_after_last_child_exits_ = true;
  has_no_children = list_of_running_zygote_children_.empty();
}
if (has_no_children)
  TearDown();


And similar in ZygoteChildDied.

That way:
- it's even clearer that we won't have contention
- we avoid potential ABA issues between child_tracking_lock_ and control_lock_

[jln (very slow on Chromium), 2014/01/28 23:22:11]

On 2014/01/28 22:38:32, piman wrote:
[...]
Yes, that's much better, thanks!
Do you really mean ABA or deadlock? The deadlock risk is that control_lock_
would be held when we call TearDown() (which acquires control_lock_ again).
I had manually convinced myself that it cannot happen as neither
ZygoteChildBorn() and ZygoteChildDied() are called with control_lock_ held.

But even with your suggested change, this remains fragile. The good news is that
this would immediately blow up in a DEBUG build without any need of a specific
race to happen.

+  }
+}
+
+// Note: this is also called from the destructor.
+void ZygoteHostImpl::TearDown() {
+  base::AutoLock lock(control_lock_);
+  if (control_fd_ > -1) {
+    // Closing the IPC channel will act as a notification to exit
+    // to the Zygote.
+    if (IGNORE_EINTR(close(control_fd_))) {
+      PLOG(ERROR) << "Could not close Zygote control channel.";
+      NOTREACHED();
+    }
+    control_fd_ = -1;
+  }
+}
+
+void ZygoteHostImpl::ZygoteChildBorn(pid_t process) {
+  base::AutoLock lock(child_tracking_lock_);
+  bool new_element_inserted =
+      list_of_running_zygote_children_.insert(process).second;
+  DCHECK(new_element_inserted);
+}
+
+void ZygoteHostImpl::ZygoteChildDied(pid_t process) {
+  base::AutoLock lock(child_tracking_lock_);
+  size_t num_erased = list_of_running_zygote_children_.erase(process);
+  DCHECK_EQ(1U, num_erased);
+  if (should_teardown_after_last_child_exits_ &&
+      list_of_running_zygote_children_.empty()) {
+    TearDown();
+  }
+}
+
 bool ZygoteHostImpl::SendMessage(const Pickle& data,
                                  const std::vector<int>* fds) {
+  DCHECK_NE(-1, control_fd_);
   CHECK(data.size() <= kZygoteMaxMessageLength)
       << "Trying to send too-large message to zygote (sending " << data.size()
       << " bytes, max is " << kZygoteMaxMessageLength << ")";
   CHECK(!fds || fds->size() <= UnixDomainSocket::kMaxFileDescriptors)
       << "Trying to send message with too many file descriptors to zygote "
       << "(sending " << fds->size() << ", max is "
       << UnixDomainSocket::kMaxFileDescriptors << ")";
 
   return UnixDomainSocket::SendMsg(control_fd_,
                                    data.data(), data.size(),
                                    fds ? *fds : std::vector<int>());
 }
 
 ssize_t ZygoteHostImpl::ReadReply(void* buf, size_t buf_len) {
+  DCHECK_NE(-1, control_fd_);
   // At startup we send a kZygoteCommandGetSandboxStatus request to the zygote,
   // but don't wait for the reply. Thus, the first time that we read from the
   // zygote, we get the reply to that request.
   if (!have_read_sandbox_status_word_) {
     if (HANDLE_EINTR(read(control_fd_, &sandbox_status_,
                           sizeof(sandbox_status_))) !=
         sizeof(sandbox_status_)) {
       return -1;
     }
     have_read_sandbox_status_word_ = true;
@@ skipping 83 matching lines @@
 #if !defined(OS_OPENBSD)
   // This is just a starting score for a renderer or extension (the
   // only types of processes that will be started this way).  It will
   // get adjusted as time goes on.  (This is the same value as
   // chrome::kLowestRendererOomScore in chrome/chrome_constants.h, but
   // that's not something we can include here.)
   const int kLowestRendererOomScore = 300;
   AdjustRendererOOMScore(pid, kLowestRendererOomScore);
 #endif
 
+  ZygoteChildBorn(pid);
   return pid;
 }
 
 #if !defined(OS_OPENBSD)
 void ZygoteHostImpl::AdjustRendererOOMScore(base::ProcessHandle pid,
                                             int score) {
   // 1) You can't change the oom_score_adj of a non-dumpable process
   //    (EPERM) unless you're root. Because of this, we can't set the
   //    oom_adj from the browser process.
   //
@@ skipping 59 matching lines @@
 #endif
 
 void ZygoteHostImpl::EnsureProcessTerminated(pid_t process) {
   DCHECK(init_);
   Pickle pickle;
 
   pickle.WriteInt(kZygoteCommandReap);
   pickle.WriteInt(process);
   if (!SendMessage(pickle, NULL))
     LOG(ERROR) << "Failed to send Reap message to zygote";
+  ZygoteChildDied(process);
 }
 
 base::TerminationStatus ZygoteHostImpl::GetTerminationStatus(
     base::ProcessHandle handle,
     bool known_dead,
     int* exit_code) {
   DCHECK(init_);
   Pickle pickle;
   pickle.WriteInt(kZygoteCommandGetTerminationStatus);
   pickle.WriteBool(known_dead);
   pickle.WriteInt(handle);
 
-  // Set this now to handle the early termination cases.
-  if (exit_code)
-    *exit_code = RESULT_CODE_NORMAL_EXIT;
-
   static const unsigned kMaxMessageLength = 128;
   char buf[kMaxMessageLength];
   ssize_t len;
   {
     base::AutoLock lock(control_lock_);
     if (!SendMessage(pickle, NULL))
       LOG(ERROR) << "Failed to send GetTerminationStatus message to zygote";
     len = ReadReply(buf, sizeof(buf));
   }
 
+  // Set this now to handle the error cases.
+  if (exit_code)
+    *exit_code = RESULT_CODE_NORMAL_EXIT;
+  int status = base::TERMINATION_STATUS_NORMAL_TERMINATION;
+
   if (len == -1) {
     LOG(WARNING) << "Error reading message from zygote: " << errno;
-    return base::TERMINATION_STATUS_NORMAL_TERMINATION;
   } else if (len == 0) {
     LOG(WARNING) << "Socket closed prematurely.";
-    return base::TERMINATION_STATUS_NORMAL_TERMINATION;
+  } else {
+    Pickle read_pickle(buf, len);
+    int tmp_status, tmp_exit_code;
+    PickleIterator iter(read_pickle);
+    if (!read_pickle.ReadInt(&iter, &tmp_status) ||
+        !read_pickle.ReadInt(&iter, &tmp_exit_code)) {
+      LOG(WARNING)
+          << "Error parsing GetTerminationStatus response from zygote.";
+    } else {
+      if (exit_code)
+        *exit_code = tmp_exit_code;
+      status = tmp_status;
+    }
   }
 
-  Pickle read_pickle(buf, len);
-  int status, tmp_exit_code;
-  PickleIterator iter(read_pickle);
-  if (!read_pickle.ReadInt(&iter, &status) ||
-      !read_pickle.ReadInt(&iter, &tmp_exit_code)) {
-    LOG(WARNING) << "Error parsing GetTerminationStatus response from zygote.";
-    return base::TERMINATION_STATUS_NORMAL_TERMINATION;
+  if (status != base::TERMINATION_STATUS_STILL_RUNNING) {
+    ZygoteChildDied(handle);
   }
-
-  if (exit_code)
-    *exit_code = tmp_exit_code;
-
   return static_cast<base::TerminationStatus>(status);
 }
 
 pid_t ZygoteHostImpl::GetPid() const {
   return pid_;
 }
 
 pid_t ZygoteHostImpl::GetSandboxHelperPid() const {
   return RenderSandboxHostLinux::GetInstance()->pid();
 }
 
 int ZygoteHostImpl::GetSandboxStatus() const {
   if (have_read_sandbox_status_word_)
     return sandbox_status_;
   return 0;
 }
 
 }  // namespace content