Remove uses of CloseProcessHandle.

remoting/host/ipc_desktop_environment.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 "remoting/host/ipc_desktop_environment.h"
 
 #include <utility>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ skipping 163 matching lines @@
   }
 
   if (i != active_connections_.end()) {
     daemon_channel_->Send(new ChromotingNetworkDaemonMsg_SetScreenResolution(
         i->first, resolution));
   }
 }
 
 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));
 #endif  // defined(OS_POSIX)
   }
 }
 
 void IpcDesktopEnvironmentFactory::OnTerminalDisconnected(int terminal_id) {
   if (!caller_task_runner_->BelongsToCurrentThread()) {
     caller_task_runner_->PostTask(FROM_HERE, base::Bind(
         &IpcDesktopEnvironmentFactory::OnTerminalDisconnected,
         base::Unretained(this), terminal_id));
     return;
   }
 
   ActiveConnectionsList::iterator i = active_connections_.find(terminal_id);
   if (i != active_connections_.end()) {
     DesktopSessionProxy* desktop_session_proxy = i->second;
     active_connections_.erase(i);
 
     // Disconnect the client session.
     desktop_session_proxy->DisconnectSession();
   }
 }
 
 }  // namespace remoting