Fix crash in renderer initialization when media pipeline is stopped

The crash happens because by the time we call Demuxer::GetStream again
the main thread might have proceeded with destroying SourceBuffer and
thus ChunkDemuxer no longer allows accessing demuxer streams. Luckily
demuxer streams themselves are still alive, just moved to
ChunkDemuxer::removed_streams_ (see ChunkDemuxer::RemoveId). So cached
pointers can still be used safely.

BUG=668604

Review-Url: https://codereview.chromium.org/2558213002
Cr-Commit-Position: refs/heads/master@{#438000}

media/base/demuxer_stream_provider.h

@@ -39,6 +39,13 @@ class MEDIA_EXPORT DemuxerStreamProvider {
   //   Returns the first stream of the given stream type (which is not allowed
   //   to be DemuxerStream::TEXT), or NULL if that type of stream is not
   //   present.
+  //   NOTE: Once a DemuxerStream pointer is returned from GetStream it is
+  //   guaranteed to stay valid for as long as the Demuxer/DemuxerStreamProvider
+  //   is alive. But make no assumption that once GetStream returned a non-null
+  //   pointer for some stream type then all subsequent calls will also return
+  //   non-null pointer for the same stream type. In MSE Javascript code can
+  //   remove SourceBuffer from a MediaSource at any point and this will make
+  //   some previously existing streams inaccessible/unavailable.
   // Other types:
   //   Should not be called.
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0;

media/renderers/renderer_impl.cc

@@ -131,24 +131,11 @@ void RendererImpl::Initialize(DemuxerStreamProvider* demuxer_stream_provider,
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
   DCHECK(!init_cb.is_null());
   DCHECK(client);
-  DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) ||
-         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)
-    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;
     return;
@@ -384,19 +371,23 @@ void RendererImpl::InitializeAudioRenderer() {
   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_stream->SetStreamStatusChangeCB(base::Bind(
+      &RendererImpl::RestartStreamPlayback, weak_this_, audio_stream));
+
   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) {
@@ -429,17 +420,21 @@ void RendererImpl::InitializeVideoRenderer() {
   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_stream->SetStreamStatusChangeCB(base::Bind(
+      &RendererImpl::RestartStreamPlayback, weak_this_, video_stream));
+
   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);
 }

media/renderers/renderer_impl_unittest.cc

@@ -150,12 +150,34 @@ class RendererImplTest : public ::testing::Test {
   void InitializeWithAudio() {
     CreateAudioStream();
     SetAudioRendererInitializeExpectations(PIPELINE_OK);
+    // There is a potential race between HTMLMediaElement/WMPI shutdown and
+    // renderers being initialized which might result in DemuxerStreamProvider
+    // GetStream suddenly returning NULL (see crbug.com/668604). So we are going
+    // to check here that GetStream will be invoked exactly 3 times during
+    // RendererImpl initialization to help catch potential issues. Currently the
+    // GetStream is invoked once directly from RendererImpl::Initialize, once
+    // indirectly from RendererImpl::Initialize via HasEncryptedStream and once
+    // from RendererImpl::InitializeAudioRenderer.
+    EXPECT_CALL(*demuxer_, GetStream(DemuxerStream::AUDIO))
+        .Times(2)
+        .WillRepeatedly(Return(audio_stream_.get()));
     InitializeAndExpect(PIPELINE_OK);
   }
 
   void InitializeWithVideo() {
     CreateVideoStream();
     SetVideoRendererInitializeExpectations(PIPELINE_OK);
+    // There is a potential race between HTMLMediaElement/WMPI shutdown and
+    // renderers being initialized which might result in DemuxerStreamProvider
+    // GetStream suddenly returning NULL (see crbug.com/668604). So we are going
+    // to check here that GetStream will be invoked exactly 3 times during
+    // RendererImpl initialization to help catch potential issues. Currently the
+    // GetStream is invoked once directly from RendererImpl::Initialize, once
+    // indirectly from RendererImpl::Initialize via HasEncryptedStream and once
+    // from RendererImpl::InitializeVideoRenderer.
+    EXPECT_CALL(*demuxer_, GetStream(DemuxerStream::VIDEO))
+        .Times(2)
+        .WillRepeatedly(Return(video_stream_.get()));
     InitializeAndExpect(PIPELINE_OK);
   }