Fixes use-after-free in MojoDemuxerStreamImpl.

media/mojo/clients/mojo_renderer_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/mojo/clients/mojo_renderer_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 26 matching lines @@
 }
 
 void MojoRendererImpl::Initialize(
     DemuxerStreamProvider* demuxer_stream_provider,
     media::RendererClient* client,
     const PipelineStatusCB& init_cb) {
   DVLOG(1) << __FUNCTION__;
   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();
 
   // Using base::Unretained(this) is safe because |this| owns
   // |remote_renderer_|, and the callback won't be dispatched if
   // |remote_renderer_| is destroyed.
   remote_renderer_->Initialize(
       binding_.CreateInterfacePtrAndBind(), std::move(audio_stream),
       std::move(video_stream),
       base::Bind(&MojoRendererImpl::OnInitialized, base::Unretained(this)));
@@ skipping 54 matching lines @@
 base::TimeDelta MojoRendererImpl::GetMediaTime() {
   base::AutoLock auto_lock(lock_);
   DVLOG(3) << __FUNCTION__ << ": " << time_.InMilliseconds() << " ms";
   return time_;
 }
 
 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) {
   DVLOG(3) << __FUNCTION__ << ": " << time_usec << ", " << max_time_usec;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
   time_ = base::TimeDelta::FromMicroseconds(time_usec);
 }
 
@@ skipping 68 matching lines @@
 void MojoRendererImpl::OnInitialized(bool success) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!init_cb_.is_null());
 
   base::ResetAndReturn(&init_cb_).Run(
       success ? PIPELINE_OK : PIPELINE_ERROR_INITIALIZATION_FAILED);
 }
 
 }  // namespace media