Fix crash when pipeline is stopped while video restart is pending

media/renderers/renderer_impl.cc

Index: media/renderers/renderer_impl.cc
diff --git a/media/renderers/renderer_impl.cc b/media/renderers/renderer_impl.cc
index 210e5cc574f2437b4368c498f56b02facf5c1e61..b8d9511140231b1e3c1570c879b9ce4af01153ec 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -111,6 +111,8 @@ RendererImpl::~RendererImpl() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
+  state_ = STATE_SHUTDOWN;

[xhwang, 2017/01/04 18:29:09]

Instead of adding a new state, does it make sense to invalid all weak pointers
so that RendererImpl::RestartVideoRenderer() (the callback) will never be called
during destruction?

Note that all callbacks are bound using weak pointers, e.g.
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=0&l...

[servolk, 2017/01/04 19:00:52]

Yes, I guess we could try invalidating RendererImpl weak pointers at the
beginning of ~RendererImpl destruction to ensure RestartA/VRenderer don't get
invoked, I'll give it a try.
Although I still think that removing the DCHECK_EQ(state_, STATE_PLAYING); in
the Restart* methods is a good idea too, since we might get track status
notifications even before playback is started. But I guess I can do that as a
separate change with a separate test case.

+
   // Tear down in opposite order of construction as |video_renderer_| can still
   // need |time_source_| (which can be |audio_renderer_|) to be alive.
   video_renderer_.reset();
@@ -246,8 +248,13 @@ void RendererImpl::RestartStreamPlayback(DemuxerStream* stream,
 void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
   DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
+  // We only need to actually restart video renderer if we are in the PLAYING
+  // state. In all other states we don't need to do anything, since either we
+  // are already shutting down, or the VideoRenderer::StartPlayingFrom will be
+  // called when RendererImpl transitions into PLAYING state.
+  if (state_ != STATE_PLAYING)
+    return;
   DCHECK(video_renderer_);
-  DCHECK_EQ(state_, STATE_PLAYING);
   video_ended_ = false;
   video_renderer_->StartPlayingFrom(time);
 }
@@ -255,7 +262,12 @@ void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
 void RendererImpl::RestartAudioRenderer(base::TimeDelta time) {
   DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, STATE_PLAYING);
+  // We only need to actually restart audio renderer if we are in the PLAYING
+  // state. In all other states we don't need to do anything, since either we
+  // are already shutting down, or the AudioRenderer::StartPlayingFrom will be
+  // called when RendererImpl transitions into PLAYING state.
+  if (state_ != STATE_PLAYING)
+    return;
   DCHECK(time_source_);
   DCHECK(audio_renderer_);
   audio_ended_ = false;
@@ -724,6 +736,7 @@ void RendererImpl::PausePlayback() {
     case STATE_UNINITIALIZED:
     case STATE_INIT_PENDING_CDM:
     case STATE_INITIALIZING:
+    case STATE_SHUTDOWN:
       NOTREACHED() << "Invalid state: " << state_;
       break;