Fixing CRD curtain mode connection failures on Win8+ official builds

remoting/host/win/wts_session_process_delegate.cc

Index: remoting/host/win/wts_session_process_delegate.cc
diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc
index 780531c0e674f70698b203a6e155d828ad4b8e4f..aa06fb53b2d39d4b44f3298a7fa5dccf60b4cae1 100644
--- a/remoting/host/win/wts_session_process_delegate.cc
+++ b/remoting/host/win/wts_session_process_delegate.cc
@@ -155,6 +155,9 @@ class WtsSessionProcessDelegate::Core
   // 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_;
 
@@ -252,8 +255,9 @@ void WtsSessionProcessDelegate::Core::Send(IPC::Message* message) {
 void WtsSessionProcessDelegate::Core::CloseChannel() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
-  if (!channel_)
+  if (!channel_) {
     return;
+  }
 
   channel_.reset();
   elevated_server_handle_.reset();
@@ -273,11 +277,13 @@ void WtsSessionProcessDelegate::Core::KillProcess() {
   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();
@@ -304,10 +310,41 @@ void WtsSessionProcessDelegate::Core::OnIOCompleted(
       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;
     }
   }
@@ -489,8 +526,9 @@ void WtsSessionProcessDelegate::Core::InitializeJobCompleted(ScopedHandle job) {
 
   job_ = std::move(job);
 
-  if (launch_pending_)
+  if (launch_pending_) {
     DoLaunchProcess();
+  }
 }
 
 void WtsSessionProcessDelegate::Core::OnActiveProcessZero() {
@@ -506,11 +544,11 @@ void WtsSessionProcessDelegate::Core::OnActiveProcessZero() {
 void WtsSessionProcessDelegate::Core::OnProcessLaunchDetected(
     base::ProcessId pid) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  if (!elevated_server_handle_.is_valid())
-    return;
+  DCHECK_NE(pid, elevated_launcher_pid_);
 
-  if (pid == elevated_launcher_pid_)
+  if (!elevated_server_handle_.is_valid()) {
     return;
+  }
 
   DWORD desired_access =
       SYNCHRONIZE | PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION;