media: Fix lifetime of InterfaceFactoryImpl and created impls

media/mojo/services/mojo_renderer_service.cc

Index: media/mojo/services/mojo_renderer_service.cc
diff --git a/media/mojo/services/mojo_renderer_service.cc b/media/mojo/services/mojo_renderer_service.cc
index 22c9a833377e2c420ca172243b351f1f9eb849af..581db6f27793cfa424475bac8090e3f3c553a00d 100644
--- a/media/mojo/services/mojo_renderer_service.cc
+++ b/media/mojo/services/mojo_renderer_service.cc
@@ -271,7 +271,10 @@ void MojoRendererService::InitiateScopedSurfaceRequest(
     // |renderer_| is likely not of type MediaPlayerRenderer.
     // This is an unexpected call, and the connection should be closed.
     mojo::ReportBadMessage("Unexpected call to InitiateScopedSurfaceRequest.");
-    binding_->Close();
+
+    if (binding_)
+      binding_->Close();

[xhwang, 2016/11/23 22:27:09]

tguilbert:

Today the |binding_| is a StrongBindingPtr which means that MojoRendererService
can only be bound with StrongBinding. I want to change MojoRendererService so it
can be used with both mojo::Binding or mojo::StrongBinding (see the rest of this
CL).

The check here would "work", but I feel it totally defies the original purpose
of this code. After this change, only when using MediaPlayerRenderer would we
set the |binding_| :)

To fix it, what do you think of returning an error to signal "failure" so that
the client side can drive the destruction process?

[tguilbert, 2016/11/23 23:45:06]

I originally used a base::Optional<UnguessableToken>, and sent back a nullopt
when this failed. However, at review time, I was asked about how likely this
scenario was to happen; the conclusion was that only a misbehaving/compromised
client would ever call InitiateScopedSurfaceRequest when
|initiate_surface_request_cb_| is null. The guidance I received was then to
assume that any unexpected call to this method was from a compromised client
(and should be treated as a security issue).

In that regards, I don't think that sending a failure flag to the client would
accomplish much, since we couldn't trust a compromised client to destroy itself.

I am not sure what would be the right thing to do here then. I guess, if we are
using a strong binding, we close the binding (because it owns |this|, and it's
the only way to safely delete |this|) and if we use a weak binding, we delete
|this|.

WDYT?

[xhwang, 2016/11/29 10:20:57]

tguilbert:

I have a solution to use a bad_message_cb instead of binding:
https://chromiumcodereview.appspot.com/2539703002/

It works, but that makes me feel that we might have some layering/design issues.
ISTM that InitiateScopedSurfaceRequest() doesn't belong to the mojom::Renderer
interface. For example, it doesn't exist in the C++ media::Renderer interface.
It also look very special that in MojoRendererService we don't call into the
|renderer_| to do the work, but will call a callback.

MediaPlayerRenderer is a Renderer, but it also has some additional
functionality, for example, to handle this surface request. In C++ layer, this
is done by binding additional features in the constructor, but in mojo, we don't
have this ability. One idea I have is to introduce an additional mojom
interface, e.g.

interface MediaPlayerRenderer {
  InitiateScopedSurfaceRequest() => ...
  GetRenderer(Renderer& renderer);
}

Then at RenderFrameHostImpl, we register a MediaPlayerRenderer service, and
connect to it from MediaPlayerRendererClient.

At the service side, we need a simple impl of mojom::MediaPlayerRenderer, which
delegate all work to C++ media::MediaPlayerRenderer. In GetRenderer(), we can
create a MojoRendererService to bind the RendererRequest. You can see similar
model where we create a MojoDecryptorService in MojoCdmService:

https://cs.chromium.org/chromium/src/media/mojo/services/mojo_cdm_service.cc?...

I feel overall this will be a cleaner design and it will eliminate the bad
message handling here. I am sad that this means we'll have some special code for
MediaPlayerRenderer and it'll be different from other mojo based Renderers. In
the future, if "requesting a surface" becomes a common pattern in
media::Renderer, maybe we should add native support throughout the full Renderer
stack.

WDYT?

[tguilbert, 2016/11/29 19:05:33]

We discussed offline. I agree with everything that was said. I am opening a bug
for future work and will add it to this CL.

+
     return;
   }