Fixing CRD curtain mode connection failures on Win8+ official builds

remoting/host/win/wts_session_process_delegate.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.
 //
 // This file implements the Windows service controlling Me2Me host processes
 // running within user sessions.
 
 #include "remoting/host/win/wts_session_process_delegate.h"
 
 #include <utility>
@@ skipping 137 matching lines @@
   // The handle of the worker process, if launched.
   base::win::ScopedHandle worker_process_;
 
   // If launching elevated, this holds the server handle after launch, until
   // the final process launches.
   mojo::edk::ScopedPlatformHandle elevated_server_handle_;
 
   // If launching elevated, this is the pid of the launcher process.
   base::ProcessId elevated_launcher_pid_ = base::kNullProcessId;
 
+  // Tracks the id of the worker process.
+  base::ProcessId worker_process_pid_ = base::kNullProcessId;
+
   // The mojo child token for the process being launched.
   std::string mojo_child_token_;
 
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
 WtsSessionProcessDelegate::Core::Core(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     std::unique_ptr<base::CommandLine> target_command,
     bool launch_elevated,
@@ skipping 77 matching lines @@
   if (channel_) {
     channel_->Send(message);
   } else {
     delete message;
   }
 }
 
 void WtsSessionProcessDelegate::Core::CloseChannel() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
-  if (!channel_)
+  if (!channel_) {
     return;
+  }
 
   channel_.reset();
   elevated_server_handle_.reset();
   elevated_launcher_pid_ = base::kNullProcessId;
   if (!mojo_child_token_.empty()) {
     mojo::edk::ChildProcessLaunchFailed(mojo_child_token_);
     mojo_child_token_.clear();
   }
 }
 
 void WtsSessionProcessDelegate::Core::KillProcess() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   CloseChannel();
 
   event_handler_ = nullptr;
   launch_pending_ = false;
 
   if (launch_elevated_) {
-    if (job_.IsValid())
+    if (job_.IsValid()) {
       TerminateJobObject(job_.Get(), CONTROL_C_EXIT);
+    }
   } else {
-    if (worker_process_.IsValid())
+    if (worker_process_.IsValid()) {
       TerminateProcess(worker_process_.Get(), CONTROL_C_EXIT);
+    }
   }
 
   worker_process_.Close();
 }
 
 WtsSessionProcessDelegate::Core::~Core() {
   DCHECK(!channel_);
   DCHECK(!event_handler_);
   DCHECK(!worker_process_.IsValid());
 }
 
 void WtsSessionProcessDelegate::Core::OnIOCompleted(
     base::MessagePumpForIO::IOContext* context,
     DWORD bytes_transferred,
     DWORD error) {
   DCHECK(io_task_runner_->BelongsToCurrentThread());
 
   // |bytes_transferred| is used in job object notifications to supply
   // the message ID; |context| carries process ID.
   switch (bytes_transferred) {
     case JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO: {
       caller_task_runner_->PostTask(
           FROM_HERE, base::Bind(&Core::OnActiveProcessZero, this));
       break;
     }
     case JOB_OBJECT_MSG_NEW_PROCESS: {
-      caller_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&Core::OnProcessLaunchDetected, this,
-                                static_cast<base::ProcessId>(
-                                    reinterpret_cast<uintptr_t>(context))));
+      base::ProcessId new_process_id =
+          static_cast<base::ProcessId>(reinterpret_cast<uintptr_t>(context));

[Sergey Ulanov, 2016/12/13 01:10:37]

nit: move this above the switch statement and rename it to |process_id|, so it
doesn't have to be duplicated. That way it would also be next to the comment
that explains why context=process_id.

Also I think think static_cast<> is not necessary here.

[joedow, 2016/12/13 03:52:42]

Done.  The cast dance seems to be required, I get compilation errors otherwise.

+      if (elevated_launcher_pid_ == base::kNullProcessId) {
+        // Ignore process launch events when we don't have a valid launcher pid.
+        return;
+      }
+
+      if (new_process_id != elevated_launcher_pid_) {
+        DCHECK_EQ(worker_process_pid_, base::kNullProcessId);
+        worker_process_pid_ = new_process_id;
+      }
+      break;
+    }
+    case JOB_OBJECT_MSG_EXIT_PROCESS: {
+      base::ProcessId exiting_process_id =
+          static_cast<base::ProcessId>(reinterpret_cast<uintptr_t>(context));
+      if (exiting_process_id == worker_process_pid_) {
+        // In official builds the first launch of a UiAccess enabled binary
+        // will fail due to 'STATUS_ELEVATION_REQUIRED'.  In that scenario, we
+        // will clear out the previously stored value for |worker_process_pid_|
+        // and retry after the subsequent relaunch of the worker process.

[Sergey Ulanov, 2016/12/13 01:10:37]

It's not clear from this comment who relaunches the process. Mention here that
ShellExecuteEx() does it for us?

[joedow, 2016/12/13 03:52:42]

Done.

+        worker_process_pid_ = base::kNullProcessId;
+      } else if (exiting_process_id == elevated_launcher_pid_) {
+        if (worker_process_pid_ == base::kNullProcessId) {
+          // The elevated launcher process can fail to launch without attemping
+          // to launch the worker.  In this scenario, the failure will be
+          // detected outside this method and the elevated launcher will be
+          // launched again.
+          return;
+        }
+
+        caller_task_runner_->PostTask(
+            FROM_HERE, base::Bind(&Core::OnProcessLaunchDetected, this,
+                                  worker_process_pid_));
+      }
       break;
     }
   }
 }
 
 bool WtsSessionProcessDelegate::Core::OnMessageReceived(
     const IPC::Message& message) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   return event_handler_->OnMessageReceived(message);
@@ skipping 161 matching lines @@
   caller_task_runner_->PostTask(FROM_HERE, base::Bind(
       &Core::InitializeJobCompleted, this, base::Passed(&job)));
 }
 
 void WtsSessionProcessDelegate::Core::InitializeJobCompleted(ScopedHandle job) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
   DCHECK(!job_.IsValid());
 
   job_ = std::move(job);
 
-  if (launch_pending_)
+  if (launch_pending_) {
     DoLaunchProcess();
+  }
 }
 
 void WtsSessionProcessDelegate::Core::OnActiveProcessZero() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   if (launch_pending_) {
     LOG(ERROR) << "The worker process exited before connecting via IPC.";
     launch_pending_ = false;
     ReportFatalError();
   }
 }
 
 void WtsSessionProcessDelegate::Core::OnProcessLaunchDetected(
     base::ProcessId pid) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  if (!elevated_server_handle_.is_valid())
+  DCHECK_NE(pid, elevated_launcher_pid_);
+
+  if (!elevated_server_handle_.is_valid()) {
     return;
-
-  if (pid == elevated_launcher_pid_)
-    return;
+  }
 
   DWORD desired_access =
       SYNCHRONIZE | PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION;
   base::win::ScopedHandle worker_process(
       OpenProcess(desired_access, false, pid));
   if (!worker_process.IsValid()) {
     PLOG(ERROR) << "Failed to open process " << pid;
     ReportFatalError();
     return;
   }
@@ skipping 70 matching lines @@
 
 void WtsSessionProcessDelegate::CloseChannel() {
   core_->CloseChannel();
 }
 
 void WtsSessionProcessDelegate::KillProcess() {
   core_->KillProcess();
 }
 
 }  // namespace remoting