arc-bridge: Restart ARC instance on crash

arc-bridge: Restart ARC instance on crash

There have been cases where the instance crashes. This change makes it
so that if there is a crash and the connection gets lost, the instance
will be cleanly shut down and restarted afterwards.

BUG=569446
TEST=components_unittests

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

[Luis Héctor Chávez, 2015-12-22 23:31:23]

lhchavez@chromium.org changed reviewers:
+ hidehiko@chromium.org

[Luis Héctor Chávez, 2015-12-22 23:31:24]

PTAL

[hidehiko, 2015-12-29 13:27:12]

Sorry for being late. I'm on YE vacation...

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_b...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:60: // When the instance crashes, it will
be stopped and restarted.
Clarification: In this case, the state would be changed to

(any) -> STOPPING -> STOPPED

then, -> SOCKET_CREATING -> ... -> READY, right?
Could you kindly briefly note that the state will be changed during the restart,
too?

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.cc:183: void
ArcBridgeService::CloseAllChannels() {
This method looks not actually close the channels?

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:105:
instance_ptr_.set_connection_error_handler(base::Bind(
This should be called before Init() invocation? (I believe at L101 or so?)

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:122: void
ArcBridgeServiceImpl::OnChannelClosed() {
In the following scenario:

- the channel is accidentally closed. (then reconnect_ is set to true).
- Then, host tries to StopInstance() intentionally.

I believe we'd like to stop the instance, but in the current implementation, it
restarts. WDYT?

[Luis Héctor Chávez, 2015-12-29 17:17:36]

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_b...
File components/arc/arc_bridge_bootstrap.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_b...
components/arc/arc_bridge_bootstrap.cc:60: // When the instance crashes, it will
be stopped and restarted.
On 2015/12/29 13:27:11, hidehiko wrote:
> Clarification: In this case, the state would be changed to
> 
> (any) -> STOPPING -> STOPPED
> 
> then, -> SOCKET_CREATING -> ... -> READY, right?
> Could you kindly briefly note that the state will be changed during the
restart,
> too?

Done.

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.cc:183: void
ArcBridgeService::CloseAllChannels() {
On 2015/12/29 13:27:12, hidehiko wrote:
> This method looks not actually close the channels?

It does, but let me add a comment to make it more clear.

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:105:
instance_ptr_.set_connection_error_handler(base::Bind(
On 2015/12/29 13:27:12, hidehiko wrote:
> This should be called before Init() invocation? (I believe at L101 or so?)

Done.

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:122: void
ArcBridgeServiceImpl::OnChannelClosed() {
On 2015/12/29 13:27:12, hidehiko wrote:
> In the following scenario:
> 
> - the channel is accidentally closed. (then reconnect_ is set to true).
> - Then, host tries to StopInstance() intentionally.
> 
> I believe we'd like to stop the instance, but in the current implementation,
it
> restarts. WDYT?

In those cases StopInstance() is not called directly. Rather one of the
prerequisites change (either logoff or opt-out), in which case the instance
would not restart.

[hidehiko, 2016-01-05 08:17:13]

LGTM with a renaming nitpick.

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.cc:183: void
ArcBridgeService::CloseAllChannels() {
On 2015/12/29 17:17:36, Luis Héctor Chávez wrote:
> On 2015/12/29 13:27:12, hidehiko wrote:
> > This method looks not actually close the channels?
> 
> It does, but let me add a comment to make it more clear.

Oh, I was confused by the name.
So, let's rename the methods, e.g. OnAppChannelClosed -> CloseAppChannel, etc.
Actually, even on error, there seem cases that the channel is still kept open,
so passing the CloseAppChannel as a error-handling callback should make sense,
IMHO. WDYT?

[Luis Héctor Chávez, 2016-01-05 18:59:21]

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1548833002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.cc:183: void
ArcBridgeService::CloseAllChannels() {
On 2016/01/05 08:17:13, hidehiko wrote:
> On 2015/12/29 17:17:36, Luis Héctor Chávez wrote:
> > On 2015/12/29 13:27:12, hidehiko wrote:
> > > This method looks not actually close the channels?
> > 
> > It does, but let me add a comment to make it more clear.
> 
> Oh, I was confused by the name.
> So, let's rename the methods, e.g. OnAppChannelClosed -> CloseAppChannel, etc.
> Actually, even on error, there seem cases that the channel is still kept open,
> so passing the CloseAppChannel as a error-handling callback should make sense,
> IMHO. WDYT?

When there is an error, Mojo closes the channel first, and then calls the
callback (if any). There should be no cases where there is an error (on an
interface) and the channel (for that interface) is still kept open.

But I'm not opposed to renaming, so Done.

[Luis Héctor Chávez, 2016-01-05 18:59:55]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-01-05 18:59:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/1548833002/#ps40001
(title: "Addressed more feedback")

[commit-bot: I haz the power, 2016-01-05 19:00:27]

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

[commit-bot: I haz the power, 2016-01-05 19:43:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-05 19:44:19]

Description was changed from

==========
arc-bridge: Restart ARC instance on crash

There have been cases where the instance crashes. This change makes it
so that if there is a crash and the connection gets lost, the instance
will be cleanly shut down and restarted afterwards.

BUG=569446
TEST=components_unittests
==========

to

==========
arc-bridge: Restart ARC instance on crash

There have been cases where the instance crashes. This change makes it
so that if there is a crash and the connection gets lost, the instance
will be cleanly shut down and restarted afterwards.

BUG=569446
TEST=components_unittests

Committed: https://crrev.com/aff66ee04b14fb3295c87684135e577eeff2b3b2
Cr-Commit-Position: refs/heads/master@{#367613}
==========

[commit-bot: I haz the power, 2016-01-05 19:44:20]

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