Integrate Surface requests with MediaPlayerRenderer

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 deec2d1d543e3663bd45ac08e04f55eda27e9c76..b964a6ce8c0d62ef78ef32b21248cfce23b2e4b2 100644
--- a/media/mojo/services/mojo_renderer_service.cc
+++ b/media/mojo/services/mojo_renderer_service.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/optional.h"
 #include "media/base/audio_renderer_sink.h"
 #include "media/base/media_keys.h"
 #include "media/base/media_url_demuxer.h"
@@ -24,13 +25,15 @@ MojoRendererService::MojoRendererService(
     base::WeakPtr<MojoCdmServiceContext> mojo_cdm_service_context,
     scoped_refptr<AudioRendererSink> audio_sink,
     std::unique_ptr<VideoRendererSink> video_sink,
-    std::unique_ptr<media::Renderer> renderer)
+    std::unique_ptr<media::Renderer> renderer,
+    InitiateSurfaceRequestCB initiate_surface_request_cb)
     : mojo_cdm_service_context_(mojo_cdm_service_context),
       state_(STATE_UNINITIALIZED),
       playback_rate_(0),
       audio_sink_(std::move(audio_sink)),
       video_sink_(std::move(video_sink)),
       renderer_(std::move(renderer)),
+      initiate_surface_request_cb_(initiate_surface_request_cb),
       weak_factory_(this) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(renderer_);
@@ -238,4 +241,15 @@ void MojoRendererService::OnCdmAttached(
   callback.Run(success);
 }
 
+void MojoRendererService::InitiateScopedSurfaceRequest(
+    const InitiateScopedSurfaceRequestCallback& callback) {
+  if (initiate_surface_request_cb_.is_null()) {
+    // |renderer_| is likely not of type MediaPlayerRenderer.
+    // This is an unexpected call, and the connection should be closed.

[dcheng, 2016/10/05 07:18:40]

Still might be nice to call mojo::ReportBadMessage here (since it will do a
process kill if we're in the browser. Outside the browser process, maybe we'll
histogram things?)

[tguilbert, 2016/10/05 19:20:53]

I'm not sure what the proper mojo::ReportBadMessage behavior is in a 'renderer
<=> GPU' or a 'renderer <=> utility' connection.

That being said, is there a security concern with only closing the connection
and not killing the process? I expect that the only time we will actually call
this function improperly is at development time, and, otherwise, I don't know if
this method increases our attack surface meaningfully.

[dcheng, 2016/10/05 20:04:13]

Right: if it's hooked up, it'll do something useful. And if it's not, it doesn't
hurt to call it, at least that's my understanding: it's easy enough to remove
calls later, whereas its hard to find and add calls that don't exist yet)

[tguilbert, 2016/10/05 22:21:45]

Ok, done!

+    delete this;

[dcheng, 2016/10/05 07:18:40]

delete this is problematic, since this is wrapped in StrongBinding in the unit
test (and presumably will be once it's actually used). I'm actually not sure
what a good way to close the associated message pipe would be.

[tguilbert, 2016/10/05 19:20:53]

I thought "delete this" was pretty much the only way to close a pipe from an
object that is in a strong binding. There is precedent here at least:
https://cs.chromium.org/chromium/src/device/battery/battery_monitor_impl.cc?s...

[dcheng, 2016/10/05 20:04:13]

I confirmed with rockot@ that this code is unsafe: the StrongBinding has unique
ownership of |this|. rockot@ mentioned that MakeStrongBinding returns a WeakPtr
to the strong binding, and we can use that instead to close the pipe (which
deletes this).

[tguilbert, 2016/10/05 22:21:45]

Done.

+    return;
+  }
+
+  callback.Run(initiate_surface_request_cb_.Run());
+}
 }  // namespace media