Fixes use-after-free in MojoDemuxerStreamImpl.

media/mojo/clients/mojo_renderer_impl.cc

Index: media/mojo/clients/mojo_renderer_impl.cc
diff --git a/media/mojo/clients/mojo_renderer_impl.cc b/media/mojo/clients/mojo_renderer_impl.cc
index f407b89c57c7a98bcfa6b465b0911ad4d7631049..111e6fc8aaef305d30cbc482d47e8fe6a9badd90 100644
--- a/media/mojo/clients/mojo_renderer_impl.cc
+++ b/media/mojo/clients/mojo_renderer_impl.cc
@@ -44,24 +44,27 @@ void MojoRendererImpl::Initialize(
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(demuxer_stream_provider);
 
-  demuxer_stream_provider_ = demuxer_stream_provider;
   client_ = client;
   init_cb_ = init_cb;
 
   // Create audio and video mojom::DemuxerStream and bind its lifetime to
   // the pipe.
   DemuxerStream* const audio =
-      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO);
+      demuxer_stream_provider->GetStream(DemuxerStream::AUDIO);
   DemuxerStream* const video =
-      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
+      demuxer_stream_provider->GetStream(DemuxerStream::VIDEO);
 
   mojom::DemuxerStreamPtr audio_stream;
-  if (audio)
-    new MojoDemuxerStreamImpl(audio, GetProxy(&audio_stream));
+  if (audio) {
+    audio_stream_.reset(
+        new MojoDemuxerStreamImpl(audio, GetProxy(&audio_stream)));
+  }
 
   mojom::DemuxerStreamPtr video_stream;
-  if (video)
-    new MojoDemuxerStreamImpl(video, GetProxy(&video_stream));
+  if (video) {
+    video_stream_.reset(
+        new MojoDemuxerStreamImpl(video, GetProxy(&video_stream)));
+  }

[xhwang, 2016/06/17 18:47:00]

In theory, if the remote renderer doesn't care about either audio or video, it
can simply drop the interface request, causing a connection error on
audio_stream_ or video_stream_. In this case we can delete audio_stream_ or
video_stream_ to save some resource. But this will break HasAudio() and
HasVideo() in this CL.

[alokp, 2016/06/17 20:16:37]

If we decide to support this use case it would be more efficient to not create
audio/video demuxer streams to start with. I would change
mojom::Renderer::Initialize from:
Initialize(RendererClient, DemuxerStream?, DemuxerStream?) => (bool success);

to:
Initialize(RendererClient, AudioDecoderConfig?, VideoDecoderConfig?) => (
    bool success,
    handle<data_pipe_producer>? audio_pipe,
    handle<data_pipe_producer>? video_pipe);

That way Service can create whatever stream it is interested in rendering
instead of client unconditionally creating all streams. In addition it also
reduces one roundtrip currently required to initialize DemuxerStream. It might
also be efficient for mojo because IIRC creating a data pipe from a sandboxed
process requires a SYNC IPC? The new API will move the creation of data pipe to
privileged process.

WDYT?

[alokp, 2016/06/17 20:40:28]

Whoops I did not think this through. You would still need to create a
mojom::DemuxerStream and pass its proxy to the service somehow. Let me
incorporate your suggestions in the next patch.

 
   BindRemoteRendererIfNeeded();
 
@@ -136,13 +139,13 @@ bool MojoRendererImpl::HasAudio() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(remote_renderer_.get());  // We always bind the renderer.
-  return !!demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO);
+  return !!audio_stream_;
 }
 
 bool MojoRendererImpl::HasVideo() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  return !!demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
+  return !!video_stream_;
 }
 
 void MojoRendererImpl::OnTimeUpdate(int64_t time_usec, int64_t max_time_usec) {