Fixing CRD curtain mode connection failures on Win8+ official builds

I've root caused this and the problem was introduced in this CL:
https://codereview.chromium.org/2446053002

That CL moved the logic which reported the worker process launch from the
IPC Channel Connected method to an IO completion port listener.  This works
fine on un-official builds, however it breaks on official builds.  The
reason for the failure is that the remoting_desktop process on official
builds are signed and include 'UiAccess' in their manifest.  When the
launcher process uses ShellExecuteEx to launch the worker process in this
scenario, we actually see two processes get launched sequentially.  The
first starts and exists with error code 'STATUS_ELEVATION_REQUIRED' and
the second launches with the correct permissions.  This behavior worked
fine before as we listened for the connection to the IPC channel which
was done by the second, successful process launch.  With the new code,
we observed the first process launch, set up the Mojo channel for it, and
tried waiting on its process handle which exits immediately.  The second
process then starts and fails to connect to the Mojo channel.

I investigated whether UiAccess is truly required for the desktop binary
and I think that it is.  For the Ctrl+Alt+Del scenario, there are registry
keys that can be set which will require that flag.  For Alt+Tab, it is
possible that some windows might not be accessible if they have a high
high enough integrity level (+ UiAccess themselves).

So instead of removing the UiAccess flag, my approach is to listen for the
worker process creation and exit events.  I store the value of the last seen
worker process id and use that in our process launch detection code once the
launcher process exits.  This allows both un-official builds (which do not
require the extra permissions hop) and official builds will work consistently.

BUG=666992

Review-Url: https://codereview.chromium.org/2568983004
Cr-Commit-Position: refs/heads/master@{#438077}

remoting/host/desktop_session_win.cc

@@ -618,11 +618,11 @@ void DesktopSessionWin::OnSessionAttached(uint32_t session_id) {
 
   ReportElapsedTime("attached");
 
-  // Launch elevated on Win8 to be able to inject Alt+Tab.
+  // Launch elevated on Win8+ to enable injection of Alt+Tab and Ctrl+Alt+Del.
   bool launch_elevated = base::win::GetVersion() >= base::win::VERSION_WIN8;
 
   // Get the name of the executable to run. |kDesktopBinaryName| specifies
-  // uiAccess="true" in it's manifest.
+  // uiAccess="true" in its manifest.
   base::FilePath desktop_binary;
   bool result;
   if (launch_elevated) {

remoting/host/win/BUILD.gn

@@ -430,7 +430,7 @@ executable("remoting_desktop") {
   ]
 
   if (is_official_build) {
-    deps += [ ":dpi_aware_elevated_exe_manifest" ]
+    deps += [ ":dpi_aware_uiaccess_require_admin_exe_manifest" ]
   } else {
     deps += [ ":dpi_aware_exe_manifest" ]
   }

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();
@@ -296,7 +302,9 @@ void WtsSessionProcessDelegate::Core::OnIOCompleted(
   DCHECK(io_task_runner_->BelongsToCurrentThread());
 
   // |bytes_transferred| is used in job object notifications to supply
-  // the message ID; |context| carries process ID.
+  // the message ID; |context| carries process ID for the events we listen for.
+  base::ProcessId process_id =
+      static_cast<base::ProcessId>(reinterpret_cast<uintptr_t>(context));
   switch (bytes_transferred) {
     case JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO: {
       caller_task_runner_->PostTask(
@@ -304,10 +312,38 @@ 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))));
+      if (elevated_launcher_pid_ == base::kNullProcessId) {
+        // Ignore process launch events when we don't have a valid launcher pid.
+        return;
+      }
+
+      if (process_id != elevated_launcher_pid_) {
+        DCHECK_EQ(worker_process_pid_, base::kNullProcessId);
+        worker_process_pid_ = process_id;
+      }
+      break;
+    }
+    case JOB_OBJECT_MSG_EXIT_PROCESS: {
+      if (process_id == worker_process_pid_) {
+        // In official builds the first launch of a UiAccess enabled binary
+        // will fail due to 'STATUS_ELEVATION_REQUIRED'.  This is an artifact of
+        // using ShellExecuteEx() to launch the process.  In this scenario, we
+        // will clear out the previously stored value for |worker_process_pid_|
+        // and retry after the subsequent relaunch of the worker process.
+        worker_process_pid_ = base::kNullProcessId;
+      } else if (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 +525,9 @@ void WtsSessionProcessDelegate::Core::InitializeJobCompleted(ScopedHandle job) {
 
   job_ = std::move(job);
 
-  if (launch_pending_)
+  if (launch_pending_) {
     DoLaunchProcess();
+  }
 }
 
 void WtsSessionProcessDelegate::Core::OnActiveProcessZero() {
@@ -506,11 +543,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;