Fix crash when pipeline is stopped while video restart is pending

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 93 matching lines @@
   if (base::StringToInt(threshold_ms_str, &threshold_ms) && threshold_ms > 0) {
     video_underflow_threshold_ =
         base::TimeDelta::FromMilliseconds(threshold_ms);
   }
 }
 
 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();
   audio_renderer_.reset();
 
   if (!init_cb_.is_null()) {
     FinishInitialization(PIPELINE_ERROR_ABORT);
   } else if (!flush_cb_.is_null()) {
     base::ResetAndReturn(&flush_cb_).Run();
   }
@@ skipping 115 matching lines @@
       time_source_->StopTicking();
     }
     audio_renderer_->Flush(
         base::Bind(&RendererImpl::RestartAudioRenderer, weak_this_, time));
   }
 }
 
 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);
 }
 
 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;
   audio_renderer_->StartPlaying();
 }
 
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __func__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
 
@@ skipping 448 matching lines @@
 
       break;
 
     case STATE_FLUSHING:
       // It's OK to pause playback when flushing.
       break;
 
     case STATE_UNINITIALIZED:
     case STATE_INIT_PENDING_CDM:
     case STATE_INITIALIZING:
+    case STATE_SHUTDOWN:
       NOTREACHED() << "Invalid state: " << state_;
       break;
 
     case STATE_ERROR:
       // An error state may occur at any time.
       break;
   }
 
   if (time_ticking_) {
     time_ticking_ = false;
@@ skipping 97 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media