Fix crash in renderer initialization when media pipeline is stopped

media/renderers/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/renderers/renderer_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 113 matching lines @@
 }
 
 void RendererImpl::Initialize(DemuxerStreamProvider* demuxer_stream_provider,
                               RendererClient* client,
                               const PipelineStatusCB& init_cb) {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
   DCHECK(!init_cb.is_null());
   DCHECK(client);
   DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) ||

[wolenetz, 2016/12/08 23:01:02]

It looks like this DCHECK might also fail if pipeline stops fast enough that we
lose the race here.

[servolk, 2016/12/09 00:26:31]

Yes, I believe this is possible. And we also have the same race in
RendererImpl::InitializeAudioRenderer, but I've checked out of curiousity and
haven't found any crashes in RendererImpl::InitializeAudioRenderer so far, only
in InitializeVideoRenderer. I believe that's because MediaSource destruction
takes some time to remove media tracks and sent track notification events, etc,
before notifying ChunkDemuxer about SourceBuffer removal. So video renderer
init, being the last step of RendererImpl init has a higher chance of losing
this race.

[wolenetz, 2016/12/09 23:23:00]

My point is DCHECKs are meant partially to document preconditions. If this
DCHECK *can* fail here, we should either:
1) Change the DCHECK condition (change the precondition), or
2) Prevent this DCHECK from failing in the first place.

Note that only debug builds (or Release built with dcheck_always_on) will even
evaluate DCHECKs, so I wouldn't expect to see many/any crashes for something
that already has a low Release-build crash rate.

tl;dr: Is this DCHECK still ok? I think maybe we should short-circuit
::Initialize() here if the condition in this DCHECK would fail, and drop the
DCHECK. WDYT?

[servolk, 2016/12/09 23:31:07]

After spending some more time thinking about this issue I think a better way to
completely avoid this race would be to postpone MediaSource destruction until
after WMPI/pipeline/renderer init is complete. If I can find a way to do that,
then this DCHECK would stay.

          demuxer_stream_provider->GetStream(DemuxerStream::VIDEO));
 
   client_ = client;
   demuxer_stream_provider_ = demuxer_stream_provider;
   init_cb_ = init_cb;
 
   DemuxerStream* audio_stream =
       demuxer_stream_provider->GetStream(DemuxerStream::AUDIO);
   if (audio_stream)

[wolenetz, 2016/12/08 23:01:02]

Does it matter that we're setting up these CB's here on a stream that we might
not actually use (since GetStream in Initialize{Audio,Video}Renderer later might
return null)?

[servolk, 2016/12/09 00:26:31]

It doesn't matter, since the race is only happening while the main thread is
shutting down the pipeline while the media thread haven't finished
initialization yet. So these demuxer streams and a/v renderers are going to be
destroyed soon afterwards anyway.

[wolenetz, 2016/12/09 23:23:00]

Acknowledged.

     audio_stream->SetStreamStatusChangeCB(base::Bind(
         &RendererImpl::RestartStreamPlayback, weak_this_, audio_stream));
   DemuxerStream* video_stream =
       demuxer_stream_provider->GetStream(DemuxerStream::VIDEO);
   if (video_stream)
     video_stream->SetStreamStatusChangeCB(base::Bind(
         &RendererImpl::RestartStreamPlayback, weak_this_, video_stream));
 
   if (HasEncryptedStream() && !cdm_context_) {
     state_ = STATE_INIT_PENDING_CDM;
@@ skipping 223 matching lines @@
 
 void RendererImpl::InitializeAudioRenderer() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_INITIALIZING);
   DCHECK(!init_cb_.is_null());
 
   PipelineStatusCB done_cb =
       base::Bind(&RendererImpl::OnAudioRendererInitializeDone, weak_this_);
 
-  if (!demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO)) {
+  DemuxerStream* audio_stream =
+      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO);
+  if (!audio_stream) {
     audio_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     return;
   }
 
   audio_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::AUDIO, this));
   // Note: After the initialization of a renderer, error events from it may
   // happen at any time and all future calls must guard against STATE_ERROR.
-  audio_renderer_->Initialize(
-      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO), cdm_context_,
-      audio_renderer_client_.get(), done_cb);
+  audio_renderer_->Initialize(audio_stream, cdm_context_,
+                              audio_renderer_client_.get(), done_cb);
 }
 
 void RendererImpl::OnAudioRendererInitializeDone(PipelineStatus status) {
   DVLOG(1) << __func__ << ": " << status;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // OnError() may be fired at any time by the renderers, even if they thought
   // they initialized successfully (due to delayed output device setup).
   if (state_ != STATE_INITIALIZING) {
     DCHECK(init_cb_.is_null());
@@ skipping 12 matching lines @@
 
 void RendererImpl::InitializeVideoRenderer() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_INITIALIZING);
   DCHECK(!init_cb_.is_null());
 
   PipelineStatusCB done_cb =
       base::Bind(&RendererImpl::OnVideoRendererInitializeDone, weak_this_);
 
-  if (!demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO)) {
+  DemuxerStream* video_stream =
+      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
+  if (!video_stream) {
     video_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     return;
   }
 
   video_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::VIDEO, this));
   video_renderer_->Initialize(
-      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO), cdm_context_,
-      video_renderer_client_.get(),
+      video_stream, cdm_context_, video_renderer_client_.get(),
       base::Bind(&RendererImpl::GetWallClockTimes, base::Unretained(this)),
       done_cb);
 }
 
 void RendererImpl::OnVideoRendererInitializeDone(PipelineStatus status) {
   DVLOG(1) << __func__ << ": " << status;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // OnError() may be fired at any time by the renderers, even if they thought
   // they initialized successfully (due to delayed output device setup).
@@ skipping 386 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media