Recently I hit this as well. I think it makes sense for created services also hold a reference to the MojoMediaApplication.

Re #1: The issue I hit is actually not related to the original issue reported here.

That being said, the original issue is legit. Here are some issues that I can think of:

1. MediaService owns MojoMediaClient, and is passing raw pointers of MojoMediaClient to ServiceFactoryImpl. This is okay since ServiceFactoryImpl owns a ServiceContextRef to MediaService. However, it seems ServiceFactoryImpl can pass the MojoMediaClient raw pointer to other media interface implementations (MII, e.g. MojoVideoDecoderService) as well. Note that MojoVideoDecoderService is strongly bound to the pipe so in theory it can outlive ServiceFactoryImpl.

2. ServiceFactoryImpl can also pass some other raw pointers to MII, e.g. the remote interface provider (as in MojoCdmService).

In summary, MII should outlive both the MediaService, and the ServiceFactoryImpl.

Note that today, in the render process ServceFactory is owned by RenderFrameImpl, since "frame" should be destroyed after media element, we should be okay most of the time, but I guess there's no guarantee.

Note that passing the ServiceContextRef to MII (as suggested in #1) won't work, because that only guarantees that MediaService will be destructed after MII, but can't ensure ServiceFactoryImpl will be destructed after MII.

Maybe we should not use StrongBindings for MII, and instead, just let ServiceFactoryImpl own MII and use weak bindings for MII.

rockot: Any recommendations?

This sounds like a general object lifetime management issue, and you have a
number options for solving it. If none of these impls are useful without an
active MojoMediaClient object, and the only reason you'd keep a
ServiceFactoryImpl alive would be to keep a MojoMediaClient alive, maybe
SFI shouldn't own MMC.

But it's really your call. I don't think there are any special constraints
or exceptions as a result of using Mojo.

Treat StrongBinding usage as equivalent to self-owned global state. If
strongly-bound things need a reference to some shared state, that shared
state should itself probably be either leaky or ref-counted, or expose some
way to be notified when it's going away so you can forcibly cleanup its
dependents.

I have some thoughts, which comes with more questions :)

I posted my question at: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/K9UsRyKpfHQ

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a2db52064245983d71338e51d330edf256418e81

commit a2db52064245983d71338e51d330edf256418e81
Author: xhwang <xhwang@chromium.org>
Date: Tue Nov 29 22:06:46 2016

media: Add |bad_message_cb_| to MojoRendererService

This helps move the StrongBindingPtr out of MojoRendererService, so that
we can bind MojoRendererService using other type of bindings, e.g.
mojo::Binding.

When we do use MojoRendererService::Create(), which uses
mojo::StrongBinding, now the StrongBindingPtr isimplicitly owned by the
MojoRendererService through the |bad_message_cb_|.

BUG= 604912 ,669606
TEST=Covered by media_mojo_unittests

Review-Url: https://codereview.chromium.org/2539703002
Cr-Commit-Position: refs/heads/master@{#435088}

[modify] https://crrev.com/a2db52064245983d71338e51d330edf256418e81/media/mojo/clients/mojo_renderer_unittest.cc
[modify] https://crrev.com/a2db52064245983d71338e51d330edf256418e81/media/mojo/services/mojo_renderer_service.cc
[modify] https://crrev.com/a2db52064245983d71338e51d330edf256418e81/media/mojo/services/mojo_renderer_service.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e557531d68bbd31b40c2b5c8daa70638c60710c7

commit e557531d68bbd31b40c2b5c8daa70638c60710c7
Author: xhwang <xhwang@chromium.org>
Date: Wed Nov 30 07:51:30 2016

media: Fix lifetime of InterfaceFactoryImpl and created impls

Makes sure that when InterfaceFactoryImpl is destructed, all mojo
interface implemenations (e.g. MojoAudioDecoderService) are destructed.

To achieve this, a helper class StrongBindingSet is introduced such
that:
- When connection error happens, the entry will be removed from the
  binding set, and the bound impl object will be destructed.
- When StrongBindingSet is destructed, it'll destruct all outstanding
  impl objects in the binding set.

BUG= 604912 
TEST=Added a unittest for StrongBindingSet.

Review-Url: https://codereview.chromium.org/2530613003
Cr-Commit-Position: refs/heads/master@{#435141}

[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/BUILD.gn
[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/BUILD.gn
[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/interface_factory_impl.cc
[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/interface_factory_impl.h
[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/mojo_renderer_service.cc
[modify] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/mojo_renderer_service.h
[add] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/strong_binding_set.h
[add] https://crrev.com/e557531d68bbd31b40c2b5c8daa70638c60710c7/media/mojo/services/strong_binding_set_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0aa7ee95d17f6864aa9cb1e9bd8caf58baf286c4

commit 0aa7ee95d17f6864aa9cb1e9bd8caf58baf286c4
Author: xhwang <xhwang@chromium.org>
Date: Wed Nov 30 19:12:11 2016

media: Fix a typo in interface_factory_impl.h

This was introduced in
https://chromiumcodereview.appspot.com/2530613003/

TBR=sandersd@chromium.org
BUG= 604912 

Review-Url: https://codereview.chromium.org/2535303006
Cr-Commit-Position: refs/heads/master@{#435366}

[modify] https://crrev.com/0aa7ee95d17f6864aa9cb1e9bd8caf58baf286c4/media/mojo/services/interface_factory_impl.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/647234812da3104a6d8ae2d87a24ae8969191b6b

commit 647234812da3104a6d8ae2d87a24ae8969191b6b
Author: alokp <alokp@chromium.org>
Date: Mon Feb 27 22:38:58 2017

Replaces media::StrongBindingSet with mojo::StrongBindingSet.

BUG= 604912 

Review-Url: https://codereview.chromium.org/2716873003
Cr-Commit-Position: refs/heads/master@{#453362}

[modify] https://crrev.com/647234812da3104a6d8ae2d87a24ae8969191b6b/media/mojo/BUILD.gn
[modify] https://crrev.com/647234812da3104a6d8ae2d87a24ae8969191b6b/media/mojo/services/BUILD.gn
[modify] https://crrev.com/647234812da3104a6d8ae2d87a24ae8969191b6b/media/mojo/services/interface_factory_impl.cc
[modify] https://crrev.com/647234812da3104a6d8ae2d87a24ae8969191b6b/media/mojo/services/interface_factory_impl.h
[delete] https://crrev.com/fe1613f09089d4b570263393f7e7e2d9c88d23d0/media/mojo/services/strong_binding_set.h
[delete] https://crrev.com/fe1613f09089d4b570263393f7e7e2d9c88d23d0/media/mojo/services/strong_binding_set_unittest.cc