Fixing curtain mode using non-default RDP port for Windows host.

Fixing curtain mode using non-default RDP port for Windows host.

While adding debug logging for the Win 10 curtain mode issue, I realized that
the host was not able to connect to its RDP session if a non-default port
number was set in the registry.  This is because the RDP session runs as
'Local Service' and does not have access to the registry.  This code was
originally added to address crbug.com/241214 and I think it may have been
implemented and tested on XP where it would work, but it wouldn't have on
later OSes.

My change is to move the code which needs to read the port number from the
registry from the RDP process to the Daemon process.  The port number will
then be passed to the RDP process when it is created.  The code which reads
from the registry will be reused in an upcoming CL so it has been put into
a helper method even though there is only one caller.

BUG=608477
Committed: https://crrev.com/69094d5f4d39b81cce2e5234804c9868d428af76
Cr-Commit-Position: refs/heads/master@{#391335}

[joedow, 2016-05-02 22:19:41]

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

[joedow, 2016-05-02 22:19:41]

Fixing a bug I found while adding debug logging for the Win 10 curtain mode
issue.  basically the code in the RDP process is attempting to read from the
registry but does not have the permissions to access the key, thus it will
always fail.

My fix is to move the registry reading code to the daemon process and pass the
port value to the RdpClient when it is created.

PTAL!

Thanks,
Joe

[Jamie, 2016-05-03 00:18:12]

LGTM unless you decide to fail the connection in case of error as I suggest.

https://codereview.chromium.org/1935003003/diff/1/remoting/host/desktop_sessi...
File remoting/host/desktop_session_win.cc (right):

https://codereview.chromium.org/1935003003/diff/1/remoting/host/desktop_sessi...
remoting/host/desktop_session_win.cc:277: server_port = kDefaultRdpPort;
I realize you haven't changed the behaviour here, but this feels wrong. If the
user has attempted to configure something but set a bogus value, we should log
an error and fail (this would especially be true if we logged somewhere the user
could easily read it). Falling back on the default might give them false
confidence that they have done what they intended, leading to confusing
behaviour later on.

[joedow, 2016-05-03 19:07:08]

Thanks!

https://codereview.chromium.org/1935003003/diff/1/remoting/host/desktop_sessi...
File remoting/host/desktop_session_win.cc (right):

https://codereview.chromium.org/1935003003/diff/1/remoting/host/desktop_sessi...
remoting/host/desktop_session_win.cc:277: server_port = kDefaultRdpPort;
On 2016/05/03 00:18:12, Jamie wrote:
> I realize you haven't changed the behaviour here, but this feels wrong. If the
> user has attempted to configure something but set a bogus value, we should log
> an error and fail (this would especially be true if we logged somewhere the
user
> could easily read it). Falling back on the default might give them false
> confidence that they have done what they intended, leading to confusing
> behaviour later on.

I think that makes sense.  It wouldn't have worked before but would have been
difficult to track the failure down.  This makes it deterministic which is good.

[joedow, 2016-05-03 19:08:12]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-05-03 19:08:12]

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

[commit-bot: I haz the power, 2016-05-03 19:10:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1935003003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1935003003/20001

[commit-bot: I haz the power, 2016-05-03 20:00:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-03 20:01:39]

Description was changed from

==========
Fixing curtain mode using non-default RDP port for Windows host.

While adding debug logging for the Win 10 curtain mode issue, I realized that
the host was not able to connect to its RDP session if a non-default port
number was set in the registry.  This is because the RDP session runs as
'Local Service' and does not have access to the registry.  This code was
originally added to address crbug.com/241214 and I think it may have been
implemented and tested on XP where it would work, but it wouldn't have on
later OSes.

My change is to move the code which needs to read the port number from the
registry from the RDP process to the Daemon process.  The port number will
then be passed to the RDP process when it is created.  The code which reads
from the registry will be reused in an upcoming CL so it has been put into
a helper method even though there is only one caller.

BUG=608477
==========

to

==========
Fixing curtain mode using non-default RDP port for Windows host.

While adding debug logging for the Win 10 curtain mode issue, I realized that
the host was not able to connect to its RDP session if a non-default port
number was set in the registry.  This is because the RDP session runs as
'Local Service' and does not have access to the registry.  This code was
originally added to address crbug.com/241214 and I think it may have been
implemented and tested on XP where it would work, but it wouldn't have on
later OSes.

My change is to move the code which needs to read the port number from the
registry from the RDP process to the Daemon process.  The port number will
then be passed to the RDP process when it is created.  The code which reads
from the registry will be reused in an upcoming CL so it has been put into
a helper method even though there is only one caller.

BUG=608477

Committed: https://crrev.com/69094d5f4d39b81cce2e5234804c9868d428af76
Cr-Commit-Position: refs/heads/master@{#391335}
==========

[commit-bot: I haz the power, 2016-05-03 20:01:40]

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