Chromoting: stopping the service if the host ID is permanently not recognized by the could.

Chromoting: stopping the service if a permanent error is encountered (such as the host ID is permanently not recognized by the could or invalid host configuration).

BUG=123715, 112160
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132675

[alexeypa (please no reviews), 2012-04-17 00:22:20]

PTAL

[simonmorris, 2012-04-17 00:46:43]

http://codereview.chromium.org/10106013/diff/1/remoting/host/wts_session_proc...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/wts_session_proc...
remoting/host/wts_session_process_launcher_win.cc:350: bool stop_trying =
GetExitCodeProcess(process_.handle(), &exit_code) && (
It would be safer to put this logic in cross-platform code.

[Sergey Ulanov, 2012-04-17 17:24:24]

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:11: const int kSuccessExitCode = 0;
Make it enum?

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:14: 
Please add one more for invalid OAuth credentials.

[alexeypa (please no reviews), 2012-04-17 17:45:01]

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:14: 
On 2012/04/17 17:24:24, sergeyu wrote:
> Please add one more for invalid OAuth credentials.

You must have been reading my thoughts. I'm actually adding it right now.

[Lambros, 2012-04-17 18:04:02]

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:12: const int kInvalidHostConfigurationExitCode = 1;
Please could you also update the me2me_virtual_host.py script, which currently
looks for an exit code of 100 to prevent re-launching?  Probably worth adding a
comment here, explaining that these codes must match up with
me2me_virtual_host.py. That script already has a similar comment pointing here.

[alexeypa (please no reviews), 2012-04-17 18:35:53]

PTAL.

sergeyu -> url_request_context.*
lambros -> me2me_virtual_host.py
simonmorris -> everything else.

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:11: const int kSuccessExitCode = 0;
On 2012/04/17 17:24:24, sergeyu wrote:
> Make it enum?

Done.

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:12: const int kInvalidHostConfigurationExitCode = 1;
On 2012/04/17 18:04:03, Lambros wrote:
> Please could you also update the me2me_virtual_host.py script, which currently
> looks for an exit code of 100 to prevent re-launching?  Probably worth adding
a
> comment here, explaining that these codes must match up with
> me2me_virtual_host.py. That script already has a similar comment pointing
here.

Done.

http://codereview.chromium.org/10106013/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:14: 
On 2012/04/17 17:24:24, sergeyu wrote:
> Please add one more for invalid OAuth credentials.

Done.

http://codereview.chromium.org/10106013/diff/1/remoting/host/wts_session_proc...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/1/remoting/host/wts_session_proc...
remoting/host/wts_session_process_launcher_win.cc:350: bool stop_trying =
GetExitCodeProcess(process_.handle(), &exit_code) && (
On 2012/04/17 00:46:43, simonmorris wrote:
> It would be safer to put this logic in cross-platform code.

Done.

[Sergey Ulanov, 2012-04-17 18:56:12]

http://codereview.chromium.org/10106013/diff/4003/remoting/host/url_request_c...
File remoting/host/url_request_context.cc (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/host/url_request_c...
remoting/host/url_request_context.cc:65: new
URLRequestContext(proxy_config_service_.Pass());
What if this method is called more than once? Should we duplicate
proxy_config_server_ here?

[Lambros, 2012-04-17 19:01:18]

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
File remoting/tools/me2me_virtual_host.py (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:734: # These exit-codes must match the one
used by the host.
s/one/ones
or
s/the one/those

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:736: if os.WEXITSTATUS(status) == 1:
Please don't use exit code number 1.  The problem is that, if Xvfb crashes, the
me2me_host process will exit with code 1, because it links with libX11.so, and
that library calls exit(1) whenever the connection to the X server is lost (yes,
I know libraries shouldn't call exit() - this is a well-known quirk of libX11 :)

In that case, this script would try to re-launch Xvfb, but it would exit instead
of relaunching the me2me_host, because it mistakes the exit code 1 to mean that
the Host configuration is invalid.

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:748: os.WEXITSTATUS(status) == 3:
In case 3, should remove auth.config_file rather than host.config_file.  If you
want, you could then move these remove() calls into the previous if statement.
You could put os.WEXITSTATUS(status) into a temporary "exit_code" variable to
avoid repeating this code.

[alexeypa (please no reviews), 2012-04-17 19:20:32]

http://codereview.chromium.org/10106013/diff/4003/remoting/host/url_request_c...
File remoting/host/url_request_context.cc (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/host/url_request_c...
remoting/host/url_request_context.cc:65: new
URLRequestContext(proxy_config_service_.Pass());
On 2012/04/17 18:56:12, sergeyu wrote:
> What if this method is called more than once? Should we duplicate
> proxy_config_server_ here?

The same copy of url_request_context_ is returned no matter how many times
GetURLRequestContext() is called. This code matches
ShellDevToolsDelegate::GetURLRequestContext().

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
File remoting/tools/me2me_virtual_host.py (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:734: # These exit-codes must match the one
used by the host.
On 2012/04/17 19:01:19, Lambros wrote:
> s/one/ones
> or
> s/the one/those

Done.

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:736: if os.WEXITSTATUS(status) == 1:
On 2012/04/17 19:01:19, Lambros wrote:
> Please don't use exit code number 1.  The problem is that, if Xvfb crashes,
the
> me2me_host process will exit with code 1, because it links with libX11.so, and
> that library calls exit(1) whenever the connection to the X server is lost
(yes,
> I know libraries shouldn't call exit() - this is a well-known quirk of libX11
:)
> 
> In that case, this script would try to re-launch Xvfb, but it would exit
instead
> of relaunching the me2me_host, because it mistakes the exit code 1 to mean
that
> the Host configuration is invalid.

Done.

http://codereview.chromium.org/10106013/diff/4003/remoting/tools/me2me_virtua...
remoting/tools/me2me_virtual_host.py:748: os.WEXITSTATUS(status) == 3:
On 2012/04/17 19:01:19, Lambros wrote:
> In case 3, should remove auth.config_file rather than host.config_file.  If
you
> want, you could then move these remove() calls into the previous if statement.
> You could put os.WEXITSTATUS(status) into a temporary "exit_code" variable to
> avoid repeating this code.

Done.

[Lambros, 2012-04-17 19:22:58]

Python LGTM

[simonmorris, 2012-04-17 19:52:30]

lgtm, after these two comments are addressed.

http://codereview.chromium.org/10106013/diff/10001/remoting/host/heartbeat_se...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/heartbeat_se...
remoting/host/heartbeat_sender.h:82: class Delegate {
Optional nit: maybe "Listener" instead of "Delegate"? That'd be consistent with
SignalStrategy, for example.

http://codereview.chromium.org/10106013/diff/10001/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.cc:44: };
It would be safer to define these alongside the exit codes, in cross-platform
code.

[alexeypa (please no reviews), 2012-04-17 20:01:05]

http://codereview.chromium.org/10106013/diff/10001/remoting/host/heartbeat_se...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/heartbeat_se...
remoting/host/heartbeat_sender.h:82: class Delegate {
On 2012/04/17 19:52:30, simonmorris wrote:
> Optional nit: maybe "Listener" instead of "Delegate"? That'd be consistent
with
> SignalStrategy, for example.

Done.

http://codereview.chromium.org/10106013/diff/10001/remoting/host/wts_session_...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/wts_session_...
remoting/host/wts_session_process_launcher_win.cc:44: };
On 2012/04/17 19:52:30, simonmorris wrote:
> It would be safer to define these alongside the exit codes, in cross-platform
> code.

Done.

[Sergey Ulanov, 2012-04-17 20:01:24]

lgtm

http://codereview.chromium.org/10106013/diff/4003/remoting/host/wts_session_p...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/host/wts_session_p...
remoting/host/wts_session_process_launcher_win.cc:42: kHostPermanentErrorMin =
remoting::kInvalidHostConfigurationExitCode,
maybe move this to constants.h to make sure we don't forget to update this range
when adding new error codes

http://codereview.chromium.org/10106013/diff/10001/remoting/host/url_request_...
File remoting/host/url_request_context.cc (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/url_request_...
remoting/host/url_request_context.cc:64: url_request_context_ =
nit: this should be wrapped in {}

[alexeypa (please no reviews), 2012-04-17 20:28:08]

http://codereview.chromium.org/10106013/diff/4003/remoting/host/wts_session_p...
File remoting/host/wts_session_process_launcher_win.cc (right):

http://codereview.chromium.org/10106013/diff/4003/remoting/host/wts_session_p...
remoting/host/wts_session_process_launcher_win.cc:42: kHostPermanentErrorMin =
remoting::kInvalidHostConfigurationExitCode,
On 2012/04/17 20:01:25, sergeyu wrote:
> maybe move this to constants.h to make sure we don't forget to update this
range
> when adding new error codes

Done.

http://codereview.chromium.org/10106013/diff/10001/remoting/host/url_request_...
File remoting/host/url_request_context.cc (right):

http://codereview.chromium.org/10106013/diff/10001/remoting/host/url_request_...
remoting/host/url_request_context.cc:64: url_request_context_ =
On 2012/04/17 20:01:25, sergeyu wrote:
> nit: this should be wrapped in {}

Done.

[commit-bot: I haz the power, 2012-04-17 20:28:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10106013/12005

[commit-bot: I haz the power, 2012-04-17 22:30:56]

Change committed as 132675