Fix crash in renderer initialization when media pipeline is stopped

media/renderers/renderer_impl_unittest.cc

Index: media/renderers/renderer_impl_unittest.cc
diff --git a/media/renderers/renderer_impl_unittest.cc b/media/renderers/renderer_impl_unittest.cc
index 1e3e70696a280bd0f42e259df4280908736aafbc..2309c6f343726336e1837ecf2527e3b08d2989f4 100644
--- a/media/renderers/renderer_impl_unittest.cc
+++ b/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))

[DaleCurtis, 2016/12/12 23:43:09]

For HasEncryptedStream() can we cache the value during InitializeAudioRenderer()
/ InitializeVideoRenderer() or does this change later? +xhwang for clarity.

[xhwang, 2016/12/13 00:07:28]

Not having the big picture of this CL... :)

Today HasEncryptedStream() is called before
InitializeAudioRenderer()/InitializeVideoRenderer(), and the value really
depends on whether we have any encrypted DemuxerStream.

But it's a little bit vague to me when the streams can change.

I assume the streams cannot change when we are pending Renderer::Initialize()?
Then after the initialization succeeded and we are playing, some streams can be
disabled in the form of EOS. Is this correct? If we disable the encrypted
stream, I assume HasEncryptedStream() will become false, but I suppose this
should not happen during Renderer::Initialize()?

FWIW, today RendererImpl::Initialize() will wait for a CDM to be set to start
initialize audio/video renderers if HasEncryptedStream() is true. I am thinking
that we probably should move this logic to DecoderSelector so that we can handle
more dynamic config changes, e.g. switch between clear and encrytped. Will that
do any help here?

+        .Times(3)
+        .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(3)
+        .WillRepeatedly(Return(video_stream_.get()));
     InitializeAndExpect(PIPELINE_OK);
   }