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 41 matching lines @@
 
 ZygoteHostImpl::ZygoteHostImpl()
     : control_fd_(-1),
       pid_(-1),
       init_(false),
       using_suid_sandbox_(false),
       have_read_sandbox_status_word_(false),
       sandbox_status_(0) {}
 
 ZygoteHostImpl::~ZygoteHostImpl() {
-  if (init_)
-    close(control_fd_);
+  TearDown();
 }
 
 // static
 ZygoteHostImpl* ZygoteHostImpl::GetInstance() {
   return Singleton<ZygoteHostImpl>::get();
 }
 
 void ZygoteHostImpl::Init(const std::string& sandbox_cmd) {
   DCHECK(!init_);
   init_ = true;
@@ skipping 136 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::TearDown() {
+  if (init_) {
+    // 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();
+    }

[piman, 2014/01/28 02:20:44]

nit: maybe reset control_fd_ to -1 for added defense against races / bad usage.

Speaking of races, this is called on the main (UI) thread, before the IO thread
is torn down. I think most of the other methods are called from the IO thread.
Isn't there a race risk? Should this be done under control_lock_? And have
SendMessage/ReadReply properly check the value of control_fd and/or init_ and
return failure?

[jln (very slow on Chromium), 2014/01/28 02:52:45]

Done. I hadn't done it because "init_" is checked in almost all places anyways.
That race existed before (in the destructor), but you're right that this could
make it worse.

That a lock could be problematic for fringe cases (for instance if Zygote is
SIGSTOP-ed), but otherwise we should be fine. Added!

+    init_ = false;
+  }
+}
+
 bool ZygoteHostImpl::SendMessage(const Pickle& data,
                                  const std::vector<int>* fds) {
   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 << ")";
 
@@ skipping 242 matching lines @@
   return RenderSandboxHostLinux::GetInstance()->pid();
 }
 
 int ZygoteHostImpl::GetSandboxStatus() const {
   if (have_read_sandbox_status_word_)
     return sandbox_status_;
   return 0;
 }
 
 }  // namespace content