Linux: tear down Zygote at browser shutdown.

content/browser/zygote_host/zygote_host_impl_linux.cc

Index: content/browser/zygote_host/zygote_host_impl_linux.cc
diff --git a/content/browser/zygote_host/zygote_host_impl_linux.cc b/content/browser/zygote_host/zygote_host_impl_linux.cc
index 044dedc9efa578aaf4a8249f0d914b99c5085bda..b7a147d4c9dc117fb2814f4a7d9c966df720a8a1 100644
--- a/content/browser/zygote_host/zygote_host_impl_linux.cc
+++ b/content/browser/zygote_host/zygote_host_impl_linux.cc
@@ -52,16 +52,18 @@ ZygoteHost* ZygoteHost::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() {
@@ -217,8 +219,48 @@ void ZygoteHostImpl::Init(const std::string& sandbox_cmd) {
   // 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 << ")";
@@ -233,6 +275,7 @@ bool ZygoteHostImpl::SendMessage(const Pickle& data,
 }
 
 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.
@@ -336,6 +379,7 @@ pid_t ZygoteHostImpl::ForkRequest(
   AdjustRendererOOMScore(pid, kLowestRendererOomScore);
 #endif
 
+  ZygoteChildBorn(pid);
   return pid;
 }
 
@@ -415,6 +459,7 @@ void ZygoteHostImpl::EnsureProcessTerminated(pid_t process) {
   pickle.WriteInt(process);
   if (!SendMessage(pickle, NULL))
     LOG(ERROR) << "Failed to send Reap message to zygote";
+  ZygoteChildDied(process);
 }
 
 base::TerminationStatus ZygoteHostImpl::GetTerminationStatus(
@@ -427,10 +472,6 @@ base::TerminationStatus ZygoteHostImpl::GetTerminationStatus(
   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;
@@ -441,26 +482,33 @@ base::TerminationStatus ZygoteHostImpl::GetTerminationStatus(
     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);
 }