Fixing CRD curtain mode connection failures on Win8+ official builds

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
Committed: https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063
Cr-Commit-Position: refs/heads/master@{#438077}

[joedow, 2016-12-12 22:01:49]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 22:03:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-12-12 23:40:48]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2016-12-12 23:40:48]

PTAL!

Thanks,
Joe

[commit-bot: I haz the power, 2016-12-13 00:35:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 00:35:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

lgtm

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_delegate.cc:314:
static_cast<base::ProcessId>(reinterpret_cast<uintptr_t>(context));
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.

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_delegate.cc:333: // and retry after the
subsequent relaunch of the worker process.
It's not clear from this comment who relaunches the process. Mention here that
ShellExecuteEx() does it for us?

[joedow, 2016-12-13 03:45:41]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 03:45:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thanks!

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_delegate.cc:314:
static_cast<base::ProcessId>(reinterpret_cast<uintptr_t>(context));
On 2016/12/13 01:10:37, Sergey Ulanov wrote:
> 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.

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

https://codereview.chromium.org/2568983004/diff/1/remoting/host/win/wts_sessi...
remoting/host/win/wts_session_process_delegate.cc:333: // and retry after the
subsequent relaunch of the worker process.
On 2016/12/13 01:10:37, Sergey Ulanov wrote:
> It's not clear from this comment who relaunches the process. Mention here that
> ShellExecuteEx() does it for us?

Done.

[joedow, 2016-12-13 05:49:17]

The CQ bit was unchecked by joedow@chromium.org

[joedow, 2016-12-13 05:49:20]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-12-13 05:49:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2568983004/#ps20001
(title: "Addressing CR Feedback")

[commit-bot: I haz the power, 2016-12-13 05:49:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 06:11:20]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481608160494470,
"parent_rev": "6936da87f8723691b74dc9576420f4ec958bdfa1", "commit_rev":
"5993e981f3d37186b4d01e08720eceb0036d3ab4"}

[commit-bot: I haz the power, 2016-12-13 06:11:45]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-12-13 06:11:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-13 06:14:43]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063
Cr-Commit-Position: refs/heads/master@{#438077}
==========

[commit-bot: I haz the power, 2016-12-13 06:14:44]

Patchset 2 (id:??) landed as
https://crrev.com/1cdb04810de9689ba0ea5ab7e70bd35ce5477063
Cr-Commit-Position: refs/heads/master@{#438077}