Raise SIGKILL if cast_service doesn't Finalize() within timeout.

Raise SIGKILL if cast_service doesn't Finalize() within timeout.

If there is a hang-up in the Finalize() method for cast_service,
then the cast_shell process will never return and cannot be stopped
using SIGTERM. We should add a timeout so that the process is
forcibly killed if it refuses to exit properly.

BUG=

Committed: https://crrev.com/12d9b12eebb8c05d0b6316cdd374fd39a0b4a06f
Cr-Commit-Position: refs/heads/master@{#322038}

[maclellant, 2015-03-23 18:59:40]

maclellant@chromium.org changed reviewers:
+ byungchul@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[maclellant, 2015-03-23 19:00:25]

See b/19696756 for details.

[byungchul, 2015-03-23 20:00:35]

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:110: }
remove {}

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:326:
RegisterKillOnAlarm(kKillOnAlarmTimeoutSec);
Should it be before cast_browser_process_->cast_service()->Stop() ?

[gunsch, 2015-03-23 20:14:21]

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:326:
RegisterKillOnAlarm(kKillOnAlarmTimeoutSec);
On 2015/03/23 20:00:35, byungchul wrote:
> Should it be before cast_browser_process_->cast_service()->Stop() ?

+1

[maclellant, 2015-03-23 20:24:28]

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:110: }
On 2015/03/23 20:00:35, byungchul wrote:
> remove {}

Done.

https://codereview.chromium.org/1002603006/diff/1/chromecast/browser/cast_bro...
chromecast/browser/cast_browser_main_parts.cc:326:
RegisterKillOnAlarm(kKillOnAlarmTimeoutSec);
On 2015/03/23 20:00:35, byungchul wrote:
> Should it be before cast_browser_process_->cast_service()->Stop() ?

Done.

[byungchul, 2015-03-23 20:51:12]

https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:334: DeregisterKillOnAlarm();
Not sure if it is useful.

[byungchul, 2015-03-23 20:51:38]

lgtm for anything else

[maclellant, 2015-03-23 21:12:45]

https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:334: DeregisterKillOnAlarm();
On 2015/03/23 20:51:11, byungchul wrote:
> Not sure if it is useful.

It seems cleaner to me to remove any side effects once we are done. This alarm
is only meant to guard against hangs during tear down, so once we are done we
should remove everything and cancel the alarm so we don't randomly get a SIGKILL
later.

You are correct though that it doesn't really do anything useful if the process
is going to terminate soon anyways.

[gunsch, 2015-03-23 21:35:22]

lgtm

[maclellant, 2015-03-24 17:27:59]

On 2015/03/23 21:12:45, maclellant wrote:
>
https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
> File chromecast/browser/cast_browser_main_parts.cc (right):
> 
>
https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
> chromecast/browser/cast_browser_main_parts.cc:334: DeregisterKillOnAlarm();
> On 2015/03/23 20:51:11, byungchul wrote:
> > Not sure if it is useful.
> 
> It seems cleaner to me to remove any side effects once we are done. This alarm
> is only meant to guard against hangs during tear down, so once we are done we
> should remove everything and cancel the alarm so we don't randomly get a
SIGKILL
> later.
> 
> You are correct though that it doesn't really do anything useful if the
process
> is going to terminate soon anyways.

Byungchul, does this look good to you?

[byungchul, 2015-03-24 18:31:46]

On 2015/03/24 17:27:59, maclellant wrote:
> On 2015/03/23 21:12:45, maclellant wrote:
> >
>
https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
> > File chromecast/browser/cast_browser_main_parts.cc (right):
> > 
> >
>
https://codereview.chromium.org/1002603006/diff/40001/chromecast/browser/cast...
> > chromecast/browser/cast_browser_main_parts.cc:334: DeregisterKillOnAlarm();
> > On 2015/03/23 20:51:11, byungchul wrote:
> > > Not sure if it is useful.
> > 
> > It seems cleaner to me to remove any side effects once we are done. This
alarm
> > is only meant to guard against hangs during tear down, so once we are done
we
> > should remove everything and cancel the alarm so we don't randomly get a
> SIGKILL
> > later.
> > 
> > You are correct though that it doesn't really do anything useful if the
> process
> > is going to terminate soon anyways.
> 
> Byungchul, does this look good to you?

lgtm already.

[maclellant, 2015-03-24 18:32:29]

The CQ bit was checked by maclellant@chromium.org

[commit-bot: I haz the power, 2015-03-24 18:33:01]

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

[commit-bot: I haz the power, 2015-03-24 19:00:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-24 19:01:18]

Patchset 3 (id:??) landed as
https://crrev.com/12d9b12eebb8c05d0b6316cdd374fd39a0b4a06f
Cr-Commit-Position: refs/heads/master@{#322038}