Ensure the Chromoting Host process eventually terminates when shut down.

Ensure the Chromoting Host process eventually terminates when shut down.

The host process is meant to shut down cleanly when receiving a SIGTERM,
or if there is a configuration error.

This CL implements a timed watchdog, running on a new thread, that
triggers a forced exit of the process, in case it fails to terminate
normally within a reasonable time (for example, if some thread is inside
a blocking call).

BUG=420090
TEST=The watchdog shouldn't normally trigger, but I manually added a Sleep() to some thread to verify that it triggers if a thread is blocked for too long.

Committed: https://crrev.com/df715881c00599155a76e3fae40ba904203e322f
Cr-Commit-Position: refs/heads/master@{#307819}

[Lambros, 2014-12-10 02:25:07]

lambroslambrou@chromium.org changed reviewers:
+ jamiewalch@chromium.org, lukasza@chromium.org

[Lambros, 2014-12-10 02:25:08]

jamiewalch: Please review, it's not a big CL :)
lukasza: More FYI, but need to think about how long the timeout should be, in
relation to your recent changes for sending host status.

We should maybe remove the Python script's SIGTERM/SIGKILL timeout logic, in
favor of just sending SIGTERM, or use a longer timeout in Python?

[Lambros, 2014-12-10 02:29:19]

lambroslambrou@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Lambros, 2014-12-10 02:29:19]

Added kelvinp, for Chrome OS - do we want this code on Chrome OS?

[Jamie, 2014-12-10 03:02:23]

lgtm

https://codereview.chromium.org/784243003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/784243003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:320: // Destroying the watchdog will disarm
it, so it should be destroyed last.
It should be the first member variable in that case.

[Łukasz Anforowicz, 2014-12-10 17:15:31]

I am not sure if it is a good idea to disarm/destroy the watchdog when
HostProcess instance is destroyed - this can mean that the watchdog won't
terminate the process in some cases (when HostProcess is destroyed, but the main
thread is not).

https://codereview.chromium.org/784243003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/784243003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:320: // Destroying the watchdog will disarm
it, so it should be destroyed last.
On 2014/12/10 03:02:23, Jamie wrote:
> It should be the first member variable in that case.

It is possible that HostProcess is destroyed, but the process is not -
similarily to how MinimuHeartbeatSender extends the lifetime of
network_task_runner and ui_task_runner, something else could also extend these
lifetimes.  In this case the ShutdownWatchdog would be destroyed together with
HostProcess and would not enforce process shutdown, right?

https://codereview.chromium.org/784243003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:365:
shutdown_watchdog_(base::TimeDelta::FromSeconds(15)),
Looks good.  I wonder if we should pull all the process shutdown timeouts closer
together into either constants or macro definitions.  I guess the same comment
applies to my changelist :->

[Lambros, 2014-12-10 22:48:23]

lambroslambrou@chromium.org changed reviewers:
- kelvinp@chromium.org

[Lambros, 2014-12-10 22:48:24]

ptal

[Łukasz Anforowicz, 2014-12-10 23:20:48]

lgtm

LGTM

https://codereview.chromium.org/784243003/diff/20001/remoting/host/remoting_m...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/784243003/diff/20001/remoting/host/remoting_m...
remoting/host/remoting_me2me_host.cc:140: const int kShutdownTimeoutSeconds =
15;
Thanks.  I'll make similar change to MinimumHeartbeatSupporter-related changes.

[Lambros, 2014-12-10 23:33:24]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-12-10 23:35:59]

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

[commit-bot: I haz the power, 2014-12-11 01:14:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-11 01:16:08]

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