Revert 218584 "Support a new remote IPC for for GetTerminationSt..."

Broke the ChromiumOS (amd64) bot:
chromeos-chrome-31.0.1607.0_alpha-r1: chrome/nacl/nacl_helper_linux.cc: In function 'int main(int, char**)':
chromeos-chrome-31.0.1607.0_alpha-r1: chrome/nacl/nacl_helper_linux.cc:369:33: error: narrowing conversion of 'sysconf(84)' from 'long int' to 'int' inside { } is ill-formed in C++11 [-Werror=narrowing]
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd64%29/builds/10667

> Support a new remote IPC for for GetTerminationStatus.
> 
> This does three things:
> 
> 1. Clean-up a lot of the code in the Linux NaCl loader.
> 2. Create a new IPC to the NaCl loader "Zygote" for the
> main Zygote to query the termination status of processes.
> 3. Clean-up some code in the Zygote and fix "process tracking".
> 
> zygote_linux.cc:
>  * Split GetTerminationStatus() out from HandleGetTerminationStatus().
>  * Handle the case where we need to perform a remote IPC for GetTerminationStatus()
>  * Use the new GetTerminationStatus() to support the remote case for HandleReapRequest().
>  * Replace real_pids_to_sandbox_pids mapping with process_info_map_.
>     * Update shortcut case in ForkWithRealPid() to fill this out.
>     * Update GetTerminationStatus() to remove existing entries.
> 
> zygote_fork_delegate_linux.h:
>  * Create a new GetTerminationStatus() interface.
> 
> nacl_helper_linux.cc:
>  * Split HandleZygoteRequest() out from main().
>  * Split ChildNaClLoaderInit() to handle the child side of a fork().
>  * Handle a new IPC in HandleGetTerminationStatusRequest().
> 
> nacl_fork_delegate_linux.cc:
>  * Implement the new GetTerminationStatus() interface.
>  * Use Pickle for IPCs, make IPCs easier to write with SendIPCRequestAndReadReply().
> 
> BUG=133453
> R=mseaborn@chromium.org, piman@chromium.org
> 
> Review URL: https://codereview.chromium.org/23020010

TBR=jln@chromium.org

Review URL: https://codereview.chromium.org/22886011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@218602 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 31 insertions, 94 deletions

diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 65ed435..6e42a9d 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -155,99 +155,37 @@ bool Zygote::HandleRequestFromBrowser(int fd) {
   return false;
 }
 
-// TODO(jln): remove callers to this broken API. See crbug.com/274855.
 void Zygote::HandleReapRequest(int fd,
                                const Pickle& pickle,
                                PickleIterator iter) {
   base::ProcessId child;
+  base::ProcessId actual_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.
-    LOG(ERROR) << "Child not found!";
-    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) {
-    // TODO(jln): this old code is completely broken. See crbug.com/274855.
-    base::EnsureProcessTerminated(actual_child);
+  if (UsingSUIDSandbox()) {
+    actual_child = real_pids_to_sandbox_pids[child];
+    if (!actual_child)
+      return;
+    real_pids_to_sandbox_pids.erase(child);
   } 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);
+    actual_child = child;
   }
-  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.
-    LOG(ERROR) << "Zygote::GetTerminationStatus for unknown PID "
-               << real_pid;
-    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) {
-    // 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
-      // returning eroneously that it is still running. However, we do not
-      // want to risk a bug where we're told a process is dead when it's not.
-      // By sending SIGKILL, we make sure that WaitForTerminationStatus will
-      // return quickly even in this case.
-      if (kill(child, SIGKILL)) {
-        PLOG(ERROR) << "kill (" << child << ")";
-      }
-      *status = base::WaitForTerminationStatus(child, exit_code);
-    } else {
-      // We don't know if the process is dying, so get its status but don't
-      // wait.
-      *status = base::GetTerminationStatus(child, exit_code);
-    }
-  }
-  // Successfully got a status for |real_pid|.
-  if (*status != base::TERMINATION_STATUS_STILL_RUNNING) {
-    // Time to forget about this process.
-    process_info_map_.erase(real_pid);
-  }
-  return true;
+  base::EnsureProcessTerminated(actual_child);
 }
 
 void Zygote::HandleGetTerminationStatus(int fd,
                                         const Pickle& pickle,
                                         PickleIterator iter) {
   bool known_dead;
-  base::ProcessHandle child_requested;
+  base::ProcessHandle child;
 
   if (!pickle.ReadBool(&iter, &known_dead) ||
-      !pickle.ReadInt(&iter, &child_requested)) {
+      !pickle.ReadInt(&iter, &child)) {
     LOG(WARNING) << "Error parsing GetTerminationStatus request "
                  << "from browser";
     return;
@@ -255,13 +193,26 @@ void Zygote::HandleGetTerminationStatus(int fd,
 
   base::TerminationStatus status;
   int exit_code;
-
-  bool got_termination_status =
-      GetTerminationStatus(child_requested, known_dead, &status, &exit_code);
-  if (!got_termination_status) {
+  if (UsingSUIDSandbox())
+    child = real_pids_to_sandbox_pids[child];
+  if (child) {
+    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
+      // returning eroneously that it is still running. However, we do not
+      // want to risk a bug where we're told a process is dead when it's not.
+      // By sending SIGKILL, we make sure that WaitForTerminationStatus will
+      // return quickly even in this case.
+      if (kill(child, SIGKILL)) {
+        PLOG(ERROR) << "kill (" << child << ")";
+      }
+      status = base::WaitForTerminationStatus(child, &exit_code);
+    } else {
+      status = base::GetTerminationStatus(child, &exit_code);
+    }
+  } else {
     // Assume that if we can't find the child in the sandbox, then
     // it terminated normally.
-    // TODO(jln): add a DCHECK.
     status = base::TERMINATION_STATUS_NORMAL_TERMINATION;
     exit_code = RESULT_CODE_NORMAL_EXIT;
   }
@@ -285,15 +236,8 @@ int Zygote::ForkWithRealPid(const std::string& 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())) {
-    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;
+    return fork();
   }
 
   int dummy_fd;
@@ -382,20 +326,13 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
         LOG(ERROR) << "METHOD_GET_CHILD_WITH_INODE failed";
         goto error;
       }
+
+      real_pids_to_sandbox_pids[real_pid] = pid;
     } 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;
-    }
-    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";