Destroy MojoMediaClient in MojoMediaApplication::OnStop().

Destroy MojoMediaClient in MojoMediaApplication::OnStop().

Service::OnStop is not called when EmbeddedApplicationRunner is
deleted instead of stopping due to connection error. Deleting
MojoMediaClient in MojoMediaApplication::OnStop() allows
MojoMediaApplication to share cleanup code between the two
shutdown paths.

BUG=641559
Committed: https://crrev.com/8f75a9ee58edd4a3039d0fcc55f7325842afc448
Cr-Commit-Position: refs/heads/master@{#415842}

[alokp, 2016-08-31 21:22:54]

alokp@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-08-31 21:28:49]

LGTM as non-owner, but I think you should also change
MojoMediaApplication::CreateServiceFactory to no-op if mojo_media_client_ is
null.

[alokp, 2016-08-31 21:36:06]

On 2016/08/31 21:28:49, Ken Rockot wrote:
> LGTM as non-owner, but I think you should also change
> MojoMediaApplication::CreateServiceFactory to no-op if mojo_media_client_ is
> null.

Is it possible for MojoMediaApplication::CreateServiceFactory to be called once
MojoMediaApplication::OnStop has been called? IIUC the documentation of
shell::Service::OnStop correctly, it should not be possible?

[alokp, 2016-08-31 21:36:22]

alokp@chromium.org changed reviewers:
+ sandersd@chromium.org

[Ken Rockot(use gerrit already), 2016-08-31 21:59:18]

On 2016/08/31 at 21:36:06, alokp wrote:
> On 2016/08/31 21:28:49, Ken Rockot wrote:
> > LGTM as non-owner, but I think you should also change
> > MojoMediaApplication::CreateServiceFactory to no-op if mojo_media_client_ is
> > null.
> 
> Is it possible for MojoMediaApplication::CreateServiceFactory to be called
once MojoMediaApplication::OnStop has been called? IIUC the documentation of
shell::Service::OnStop correctly, it should not be possible?

Yes it's possible. Events on the Service interface only regard the service
manager's tracking of the app instance lifetime. After receiving OnStop you
won't get any more calls to OnConnect, but previously established connections
may be kept alive as long as the ServiceContext is alive.

In general it is never safe to assume that incoming connections from other
services won't outlive your own connection to the service manager, which is
really the assumption made here.

In the case of EmbeddedApplicationRunner, we do explicitly tear down
ServiceContexts upon receiving OnStop, but there are no guarantees that
MojoMediaApplication will always be run via EmbeddedApplicationRunner.

[alokp, 2016-08-31 22:08:07]

On 2016/08/31 21:59:18, Ken Rockot wrote:

> Yes it's possible. Events on the Service interface only regard the service
> manager's tracking of the app instance lifetime. After receiving OnStop you
> won't get any more calls to OnConnect, but previously established connections
> may be kept alive as long as the ServiceContext is alive.
> 
> In general it is never safe to assume that incoming connections from other
> services won't outlive your own connection to the service manager, which is
> really the assumption made here.

Ah.. thanks for explaining - very insightful. DONE.

[sandersd (OOO until July 31), 2016-08-31 23:30:49]

lgtm

[alokp, 2016-08-31 23:31:49]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-08-31 23:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2295403002/#ps20001
(title: "ignore request if stopped")

[commit-bot: I haz the power, 2016-08-31 23:32:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 01:47:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-01 01:51:25]

Description was changed from

==========
Destroy MojoMediaClient in MojoMediaApplication::OnStop().

Service::OnStop is not called when EmbeddedApplicationRunner is
deleted instead of stopping due to connection error. Deleting
MojoMediaClient in MojoMediaApplication::OnStop() allows
MojoMediaApplication to share cleanup code between the two
shutdown paths.

BUG=641559
==========

to

==========
Destroy MojoMediaClient in MojoMediaApplication::OnStop().

Service::OnStop is not called when EmbeddedApplicationRunner is
deleted instead of stopping due to connection error. Deleting
MojoMediaClient in MojoMediaApplication::OnStop() allows
MojoMediaApplication to share cleanup code between the two
shutdown paths.

BUG=641559

Committed: https://crrev.com/8f75a9ee58edd4a3039d0fcc55f7325842afc448
Cr-Commit-Position: refs/heads/master@{#415842}
==========

[commit-bot: I haz the power, 2016-09-01 01:51:26]

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