Remove uses of CloseProcessHandle.

remoting/host/ipc_desktop_environment.cc

Index: remoting/host/ipc_desktop_environment.cc
diff --git a/remoting/host/ipc_desktop_environment.cc b/remoting/host/ipc_desktop_environment.cc
index f1b1f753ff35d3c736a17755ed03e5e9789cfb71..7015343806caf598446ee5f1dc79d7f02a40282b 100644
--- a/remoting/host/ipc_desktop_environment.cc
+++ b/remoting/host/ipc_desktop_environment.cc
@@ -181,22 +181,28 @@ void IpcDesktopEnvironmentFactory::SetScreenResolution(
 
 void IpcDesktopEnvironmentFactory::OnDesktopSessionAgentAttached(
     int terminal_id,
-    base::ProcessHandle desktop_process,
+    base::ProcessHandle desktop_process_handle,
     IPC::PlatformFileForTransit desktop_pipe) {
   if (!caller_task_runner_->BelongsToCurrentThread()) {
     caller_task_runner_->PostTask(FROM_HERE, base::Bind(
         &IpcDesktopEnvironmentFactory::OnDesktopSessionAgentAttached,
-        base::Unretained(this), terminal_id, desktop_process, desktop_pipe));
+        base::Unretained(this), terminal_id, desktop_process_handle,
+        desktop_pipe));
     return;
   }
 
+  base::Process desktop_process;
+  // Unit tests may pass a pseudo-handle.
+  if (desktop_process_handle == base::GetCurrentProcessHandle())
+    desktop_process = base::Process::Current();

[Sergey Ulanov, 2015/01/24 18:04:29]

nit: please add {} here and in the else case

Can this logic be moved in the base::Process constructor?

[rvargas (doing something else), 2015/02/02 20:57:00]

done
 
Unfortunately, it cannot. This is not pretty because it is not a safe construct.
This code is basically equivalent to

if (handle == INVALID_HANDLE_VALUE)
  Do whatever I'm doing on the current process;

which is pretty bad!

IMO The problem is that the IPC code is venting a raw handle instead of an
object. If this code could receive a Process, the unit test (which is the source
of CurrentProcess) could pass the right (and safe) thing.

I thought about hacking something at the test side insteadof doing this, but
although that solves the test failure, it leaves the problem open for the
future: Do you expect any valid caller to pass CurrentProcess? If the answer is
no, I can fix the test... otherwise, we are forced to having unsafe code here.

Your call.

(BTW, I believe the right fix resides in the IPC layer, but we are going to mojo
so it's not really worth it... not that mojo fixes anything here).

[Sergey Ulanov, 2015/02/02 21:36:31]

Current process handle is used in tests only, so it would be ideal if this
function could get base::Process instead of base::ProcessHandle. In the same
time if it takes too much of your time to fix it then I'm fine keeping it as is
for now, maybe with a TODO to fix later.

[rvargas (doing something else), 2015/02/04 03:21:38]

Done.

+  else
+    desktop_process = base::Process(desktop_process_handle);
+
   ActiveConnectionsList::iterator i = active_connections_.find(terminal_id);
   if (i != active_connections_.end()) {
     i->second->DetachFromDesktop();
-    i->second->AttachToDesktop(desktop_process, desktop_pipe);
+    i->second->AttachToDesktop(desktop_process.Pass(), desktop_pipe);
   } else {
-    base::CloseProcessHandle(desktop_process);
-
 #if defined(OS_POSIX)
     DCHECK(desktop_pipe.auto_close);
     base::File pipe_closer(IPC::PlatformFileForTransitToFile(desktop_pipe));