Zygote: small cleanup

content/zygote/zygote_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/zygote/zygote_linux.h"
 
 #include <fcntl.h>
 #include <string.h>
 #include <sys/socket.h>
 #include <sys/types.h>
@@ skipping 81 matching lines @@
 #endif
   }
 
   for (;;) {
     // This function call can return multiple times, once per fork().
     if (HandleRequestFromBrowser(kBrowserDescriptor))
       return true;
   }
 }
 
+bool Zygote::GetProcessInfo(base::ProcessHandle pid,
+                            ZygoteProcessInfo* process_info) {
+  DCHECK(process_info);
+  const ZygoteProcessMap::const_iterator it = process_info_map_.find(pid);
+  if (it == process_info_map_.end()) {
+    return false;
+  }
+  *process_info = it->second;
+  return true;
+}
+
 bool Zygote::UsingSUIDSandbox() const {
   return sandbox_flags_ & kSandboxLinuxSUID;
 }
 
 bool Zygote::HandleRequestFromBrowser(int fd) {
   std::vector<int> fds;
   char buf[kZygoteMaxMessageLength];
   const ssize_t len = UnixDomainSocket::RecvMsg(fd, buf, sizeof(buf), &fds);
 
   if (len == 0 || (len == -1 && errno == ECONNRESET)) {
@@ skipping 47 matching lines @@
 void Zygote::HandleReapRequest(int fd,
                                const Pickle& pickle,
                                PickleIterator iter) {
   base::ProcessId child;
 
   if (!pickle.ReadInt(&iter, &child)) {
     LOG(WARNING) << "Error parsing reap request from browser";
     return;
   }
 
-  if (process_info_map_.find(child) == process_info_map_.end()) {
-    // TODO(jln): test on more bots and add a DCHECK.
+  ZygoteProcessInfo child_info;
+  if (!GetProcessInfo(child, &child_info)) {
     LOG(ERROR) << "Child not found!";
+    NOTREACHED();
     return;
   }
-  const base::ProcessId actual_child = process_info_map_[child].internal_pid;
-  const bool started_from_helper =
-      process_info_map_[child].started_from_helper;
-  if (!started_from_helper) {
+
+  if (!child_info.started_from_helper) {
     // TODO(jln): this old code is completely broken. See crbug.com/274855.
-    base::EnsureProcessTerminated(actual_child);
+    base::EnsureProcessTerminated(child_info.internal_pid);
   } else {
     // For processes from the helper, send a GetTerminationStatus request
     // with known_dead set to true.
     // This is not perfect, as the process may be killed instantly, but is
     // better than ignoring the request.
     base::TerminationStatus status;
     int exit_code;
     bool got_termination_status =
         GetTerminationStatus(child, true /* known_dead */, &status, &exit_code);
     DCHECK(got_termination_status);
   }
   process_info_map_.erase(child);
 }
 
 bool Zygote::GetTerminationStatus(base::ProcessHandle real_pid,
                                   bool known_dead,
                                   base::TerminationStatus* status,
                                   int* exit_code) {
-  // Do we know about this child?
-  if (process_info_map_.find(real_pid) == process_info_map_.end()) {
-    // TODO(jln): test on more bots and add a DCHECK.
+
+  ZygoteProcessInfo child_info;
+  if (!GetProcessInfo(real_pid, &child_info)) {
     LOG(ERROR) << "Zygote::GetTerminationStatus for unknown PID "
                << real_pid;
+    NOTREACHED();
     return false;
   }
   // We know about |real_pid|.
-  const base::ProcessHandle child =
-      process_info_map_[real_pid].internal_pid;
-  const bool started_from_helper =
-      process_info_map_[real_pid].started_from_helper;
-  if (started_from_helper) {
+  const base::ProcessHandle child = child_info.internal_pid;
+  if (child_info.started_from_helper) {
     // Let the helper handle the request.
     DCHECK(helper_);
     if (!helper_->GetTerminationStatus(child, known_dead, status, exit_code)) {
       return false;
     }
   } else {
     // Handle the request directly.
     if (known_dead) {
       // If we know that the process is already dead and the kernel is cleaning
       // it up, we do want to wait until it becomes a zombie and not risk
@@ skipping 33 matching lines @@
   }
 
   base::TerminationStatus status;
   int exit_code;
 
   bool got_termination_status =
       GetTerminationStatus(child_requested, known_dead, &status, &exit_code);
   if (!got_termination_status) {
     // Assume that if we can't find the child in the sandbox, then
     // it terminated normally.
-    // TODO(jln): add a DCHECK.
+    NOTREACHED();
     status = base::TERMINATION_STATUS_NORMAL_TERMINATION;
     exit_code = RESULT_CODE_NORMAL_EXIT;
   }
 
   Pickle write_pickle;
   write_pickle.WriteInt(static_cast<int>(status));
   write_pickle.WriteInt(exit_code);
   ssize_t written =
       HANDLE_EINTR(write(fd, write_pickle.data(), write_pickle.size()));
   if (written != static_cast<ssize_t>(write_pickle.size()))
     PLOG(ERROR) << "write";
 }
 
 int Zygote::ForkWithRealPid(const std::string& process_type,
                             std::vector<int>& fds,
                             const std::string& channel_switch,
                             std::string* uma_name,
                             int* uma_sample,
                             int* uma_boundary_value) {
   const bool use_helper = (helper_ && helper_->CanHelp(process_type,
                                                        uma_name,
                                                        uma_sample,
                                                        uma_boundary_value));
-  // TODO(jln): this shortcut is silly and does nothing but make the code
-  // harder to follow and to test. Get rid of it.
-  if (!(use_helper || UsingSUIDSandbox())) {

[Mark Seaborn, 2013/08/21 17:49:33]

I assume you've tested the removal of this by running Chromium with the SUID
sandbox disabled?  I don't think that will be tested by the bots now.

[jln (very slow on Chromium), 2013/08/21 19:22:32]

Yes, that was tested. We still have a few bots without the sandbox, including
some of the ASAN bots.

-    pid_t pid = fork();
-    if (pid > 0) {
-      process_info_map_[pid].internal_pid = pid;
-      process_info_map_[pid].started_from_helper = use_helper;
-    }
-    return pid;
-  }
-
   int dummy_fd;
   ino_t dummy_inode;
   int pipe_fds[2] = { -1, -1 };
   base::ProcessId pid = 0;
 
   dummy_fd = socket(PF_UNIX, SOCK_DGRAM, 0);
   if (dummy_fd < 0) {
     LOG(ERROR) << "Failed to create dummy FD";
     goto error;
   }
@@ skipping 74 matching lines @@
         goto error;
       }
     } else {
       // If no SUID sandbox is involved then no pid translation is
       // necessary.
       real_pid = pid;
     }
 
     // Now set-up this process to be tracked by the Zygote.
     if (process_info_map_.find(real_pid) != process_info_map_.end()) {
-      // TODO(jln): add DCHECK.
       LOG(ERROR) << "Already tracking PID " << real_pid;
+      NOTREACHED();
     }
     process_info_map_[real_pid].internal_pid = pid;
     process_info_map_[real_pid].started_from_helper = use_helper;
 
     if (use_helper) {
       if (!helper_->AckChild(pipe_fds[1], channel_switch)) {
         LOG(ERROR) << "Failed to synchronise with zygote fork helper";
         goto error;
       }
     } else {
@@ skipping 137 matching lines @@
                                     PickleIterator iter) {
   if (HANDLE_EINTR(write(fd, &sandbox_flags_, sizeof(sandbox_flags_))) !=
                    sizeof(sandbox_flags_)) {
     PLOG(ERROR) << "write";
   }
 
   return false;
 }
 
 }  // namespace content