Enforce handle ownership in base::Process

content/browser/child_process_launcher.cc

Index: content/browser/child_process_launcher.cc
diff --git a/content/browser/child_process_launcher.cc b/content/browser/child_process_launcher.cc
index b9573974eca8e38629b7c5c5a3eb23a1ee45fa2e..cc5385a048469b8df67956840a5c1f64363c5039 100644
--- a/content/browser/child_process_launcher.cc
+++ b/content/browser/child_process_launcher.cc
@@ -75,11 +75,10 @@ class ChildProcessLauncher::Context
         {
   }
 
-  void Launch(
-      SandboxedProcessLauncherDelegate* delegate,
-      base::CommandLine* cmd_line,
-      int child_process_id,
-      Client* client) {
+  void Launch(SandboxedProcessLauncherDelegate* delegate,
+              base::CommandLine* cmd_line,
+              int child_process_id,
+              Client* client) {
     client_ = client;
 
     CHECK(BrowserThread::GetCurrentThreadIdentifier(&client_thread_id_));
@@ -92,13 +91,12 @@ class ChildProcessLauncher::Context
 #endif
     BrowserThread::PostTask(
         BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
-        base::Bind(
-            &Context::LaunchInternal,
-            make_scoped_refptr(this),
-            client_thread_id_,
-            child_process_id,
-            delegate,
-            cmd_line));
+        base::Bind(&Context::LaunchInternal,
+                   make_scoped_refptr(this),
+                   client_thread_id_,
+                   child_process_id,
+                   delegate,
+                   cmd_line));
   }
 
 #if defined(OS_ANDROID)
@@ -112,14 +110,14 @@ class ChildProcessLauncher::Context
     if (BrowserThread::CurrentlyOn(client_thread_id)) {
       // This is always invoked on the UI thread which is commonly the
       // |client_thread_id| so we can shortcut one PostTask.
-      this_object->Notify(handle);
+      this_object->Notify(base::ProcessObject(handle));

[scottmg, 2014/10/14 18:51:03]

I'm a little worried that it's going to be difficult to make progress on
ownership, since handle<->ProcessObject isn't restricted.

[rvargas (doing something else), 2014/10/14 19:56:54]

The problem is that restricting the behavior implies fixing potentially hundreds
of callers at the same time :(.

The idea for this code is that StartSandboxedProcess,
base::LaunchElevatedProcess, StartChildProcess, etc, will return a proper
base::Process, but we have to be able to change a few files at a time.

     } else {
       BrowserThread::PostTask(
           client_thread_id, FROM_HERE,
           base::Bind(
               &ChildProcessLauncher::Context::Notify,
               this_object,
-              handle));
+              base::Passed(base::ProcessObject(handle))));
     }
   }
 #endif
@@ -135,6 +133,19 @@ class ChildProcessLauncher::Context
     terminate_child_on_shutdown_ = terminate_on_shutdown;
   }
 
+  void GetTerminationStatus() {
+    termination_status_ =
+        base::GetTerminationStatus(process_.Handle(), &exit_code_);
+  }
+
+  void SetProcessBackgrounded(bool background) {
+    base::ProcessObject to_pass = process_.Duplicate();
+    BrowserThread::PostTask(
+        BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
+        base::Bind(&Context::SetProcessBackgroundedInternal,
+                   base::Passed(&to_pass), background));
+  }
+
  private:
   friend class base::RefCountedThreadSafe<ChildProcessLauncher::Context>;
   friend class ChildProcessLauncher;
@@ -323,7 +334,7 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
           use_zygote,
 #endif
-          handle));
+          base::Passed(base::ProcessObject(handle))));
 #endif  // !defined(OS_ANDROID)
   }
 
@@ -331,21 +342,21 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
       bool zygote,
 #endif
-      base::ProcessHandle handle) {
+      base::ProcessObject process) {
 #if defined(OS_ANDROID)
     // Finally close the ipcfd
     base::ScopedFD ipcfd_closer(ipcfd_);
 #endif
     starting_ = false;
-    process_.set_handle(handle);
-    if (!handle)
+    process_ = process.Pass();
+    if (!process_.IsValid())
       LOG(ERROR) << "Failed to launch child process";
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
     zygote_ = zygote;
 #endif
     if (client_) {
-      if (handle) {
+      if (process_.IsValid()) {
         client_->OnProcessLaunched();
       } else {
         client_->OnProcessLaunchFailed();
@@ -356,7 +367,7 @@ class ChildProcessLauncher::Context
   }
 
   void Terminate() {
-    if (!process_.handle())
+    if (!process_.IsValid())
       return;
 
     if (!terminate_child_on_shutdown_)
@@ -371,16 +382,14 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
             zygote_,
 #endif
-            process_.handle()));
-    process_.set_handle(base::kNullProcessHandle);
+            base::Passed(&process_)));
   }
 
-  static void SetProcessBackgrounded(base::ProcessHandle handle,
-                                     bool background) {
-    base::Process process(handle);
+  static void SetProcessBackgroundedInternal(base::ProcessObject process,
+                                             bool background) {
     process.SetProcessBackgrounded(background);
 #if defined(OS_ANDROID)
-    SetChildProcessInForeground(handle, !background);
+    SetChildProcessInForeground(process.Handle(), !background);
 #endif
   }
 
@@ -388,12 +397,12 @@ class ChildProcessLauncher::Context
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
       bool zygote,
 #endif
-      base::ProcessHandle handle) {
+      base::ProcessObject process) {
 #if defined(OS_ANDROID)
-    VLOG(0) << "ChromeProcess: Stopping process with handle " << handle;
-    StopChildProcess(handle);
+    VLOG(1) << "ChromeProcess: Stopping process with handle "
+            << process.Handle();
+    StopChildProcess(process.Handle());
 #else
-    base::Process process(handle);
      // Client has gone away, so just kill the process.  Using exit code 0
     // means that UMA won't treat this as a crash.
     process.Terminate(RESULT_CODE_NORMAL_EXIT);
@@ -403,20 +412,19 @@ class ChildProcessLauncher::Context
     if (zygote) {
       // If the renderer was created via a zygote, we have to proxy the reaping
       // through the zygote process.
-      ZygoteHostImpl::GetInstance()->EnsureProcessTerminated(handle);
+      ZygoteHostImpl::GetInstance()->EnsureProcessTerminated(process.Handle());
     } else
 #endif  // !OS_MACOSX
     {
-      base::EnsureProcessTerminated(handle);
+      base::EnsureProcessTerminated(process.Handle());
     }
 #endif  // OS_POSIX
-    process.Close();
 #endif  // defined(OS_ANDROID)
   }
 
   Client* client_;
   BrowserThread::ID client_thread_id_;
-  base::Process process_;
+  base::ProcessObject process_;
   base::TerminationStatus termination_status_;
   int exit_code_;
   bool starting_;
@@ -453,16 +461,15 @@ bool ChildProcessLauncher::IsStarting() {
   return context_->starting_;
 }
 
-base::ProcessHandle ChildProcessLauncher::GetHandle() {
+const base::ProcessObject& ChildProcessLauncher::GetProcess() const {
   DCHECK(!context_->starting_);
-  return context_->process_.handle();
+  return context_->process_;
 }
 
 base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
     bool known_dead,
     int* exit_code) {
-  base::ProcessHandle handle = context_->process_.handle();
-  if (handle == base::kNullProcessHandle) {
+  if (!context_->process_.IsValid()) {
     // Process is already gone, so return the cached termination status.
     if (exit_code)
       *exit_code = context_->exit_code_;
@@ -471,25 +478,27 @@ base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   if (context_->zygote_) {
     context_->termination_status_ = ZygoteHostImpl::GetInstance()->
-        GetTerminationStatus(handle, known_dead, &context_->exit_code_);
+        GetTerminationStatus(context_->process_.Handle(), known_dead,
+                             &context_->exit_code_);
   } else if (known_dead) {
     context_->termination_status_ =
-        base::GetKnownDeadTerminationStatus(handle, &context_->exit_code_);
+        base::GetKnownDeadTerminationStatus(context_->process_.Handle(),
+                                            &context_->exit_code_);
   } else {
 #elif defined(OS_MACOSX)
   if (known_dead) {
     context_->termination_status_ =
-        base::GetKnownDeadTerminationStatus(handle, &context_->exit_code_);
+        base::GetKnownDeadTerminationStatus(context_->process_.Handle(),
+                                            &context_->exit_code_);
   } else {
 #elif defined(OS_ANDROID)
-  if (IsChildProcessOomProtected(handle)) {
-      context_->termination_status_ = base::TERMINATION_STATUS_OOM_PROTECTED;
+  if (IsChildProcessOomProtected(context_->process_.Handle())) {
+    context_->termination_status_ = base::TERMINATION_STATUS_OOM_PROTECTED;
   } else {
 #else
   {
 #endif
-    context_->termination_status_ =
-        base::GetTerminationStatus(handle, &context_->exit_code_);
+    context_->GetTerminationStatus();
   }
 
   if (exit_code)
@@ -508,11 +517,7 @@ base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
 }
 
 void ChildProcessLauncher::SetProcessBackgrounded(bool background) {
-  BrowserThread::PostTask(
-     BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
-     base::Bind(
-         &ChildProcessLauncher::Context::SetProcessBackgrounded,
-         GetHandle(), background));
+  context_->process_.SetProcessBackgrounded(background);
 }
 
 void ChildProcessLauncher::SetTerminateChildOnShutdown(