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..3382179fe779654d095b52589b66031e234740b8 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;
+
   // 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,20 +248,22 @@ void RendererImpl::RestartStreamPlayback(DemuxerStream* stream,
 void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
   DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(video_renderer_);
-  DCHECK_EQ(state_, STATE_PLAYING);
-  video_ended_ = false;
-  video_renderer_->StartPlayingFrom(time);
+  if (state_ == STATE_PLAYING) {

[whywhat, 2016/12/13 05:08:41]

nit: maybe you should rather check for STATE_SHUTDOWN before the state_ DCHECK
and exit if it is?

[servolk, 2016/12/13 19:39:47]

Yes, I've considered doing that, but I've decided that doing it this way is
better. Initially I assumed that RestartVideoRenderer will be called only in the
STATE_PLAYING. But now that I've been proven wrong I've thought a bit more about
this and realized that we only need to call video_renderer_->StartPlayingFrom if
we are in STATE_PLAYING. In all other states this can be a no-op. Since if we
are in a STATE_SHUTDOWN, we don't need to restart the video renderer. And if
RendererImpl::RestartVideoRenderer was somehow called in some other state, e.g.
STATE_INITIALIZING or STATE_FLUSHING, then we don't need to call
video_renderer_->StartPlayingFrom here, since it will be called later from
RendererImpl::StartPlayingFrom.

[whywhat, 2016/12/13 20:10:54]

Calling this method in any other state than PLAYING (and now SHUTDOWN) was
supposed to be a logical mistake, hence the DCHECK. Now this method can be
called in any state and will just do nothing if the state is wrong. Meaning that
if it's called by mistake in the wrong state, noone will notice.

[servolk, 2016/12/13 20:57:10]

I want to make two notes here:
1. Calling this method in any other state beside PLAYING doesn't have to be a
logical mistake. After this CL I believe it would be valid to call it in any
state.
2. What's the alternative? The RestartVideoRenderer is used as a flush callback
in RestartStreamPlayback. Flush is an async operation, meaning RendererImpl or
VideoRendererImpl might get destroyed while an async operation is pending. As
I've explained in the comment above I believe this is an intentional design
choice to guarantee that the Flush callback will always get called, even if the
object is destroyed. I'm not changing that design choice in this CL, since it
potentially might have other destabilizing consequences (we'd need to go and
ensure that all code that used Flush API is going to clean up resources
correctly if a renderer gets destroyed while an async flush is pending). So I've
decided to do what I've done with this CL - just make RestartVideo/AudioRenderer
more careful and allow them to be called in any object state. I believe this is
a better and a more robust solution than other alternatives.

[whywhat, 2016/12/13 21:43:07]

I agree that fixing this specific crash by making RestartVideoDecoder tolerable
to being called from dtor via flash_cb_ is fine.

I argue about two points:
- if flush_cb_ wasn't called as is from the dtor, it would prevent this crash as
well as any future attempts to use the renderer object while it's being
destroyed; it's probably not worth fixing in this CL; at least Flush() method in
the header could have a comment saying that flush_cb_ needs to check the state
and exit early if it's SHUTDOWN
- you don't have to allow calling RestartVideoRenderer in any state, only
SHUTDOWN and PLAYING, to fix the crash; do you expect flush_cb_ being called
from other states? again, a comment saying that RVR does nothing unless the
object is in PLAYING state would be helpful

[servolk, 2016/12/13 22:18:33]

I see your points. But my counterarguments are:
1. If we didn't call flush_cb_ from dtor then indeed we would avoid this crash,
but we would also break the Flush contract by not guaranteeing the flush_cb is
going to be always invoked no matter what. I'm arguing that it's much easier to
introduce some new bug via changing the Flush contract like that. I believe that
the fix proposed in this CL is safer.
I'm also curious to hear Dale/Xiaohan opinion on this, hopefully they'll chime
in soon.

2. We don't have to allow calling RestartVideoRenderer in any state besides
PLAYING or SHUTDOWN for now. But it's easy for us to do, so why not? I don't
expect flush_cb_ to be called from other states yet, but I don't also see any
good reasons to prohibit RVR from being called in states other than PLAYING or
SHUTDOWN. And yes, I agree, adding a comment in RVR explaining why nothing need
to be done in state other than PLAYING is reasonable, I'll do that.

+    DCHECK(video_renderer_);
+    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);
-  DCHECK(time_source_);
-  DCHECK(audio_renderer_);
-  audio_ended_ = false;
-  audio_renderer_->StartPlaying();
+  if (state_ == STATE_PLAYING) {

[whywhat, 2016/12/13 21:43:07]

nit: you could still do

if (state_ != STATE_PLAYING)
  return;

reducing the indentation of the code :)

[servolk, 2016/12/13 22:18:33]

Done.

+    DCHECK(time_source_);
+    DCHECK(audio_renderer_);
+    audio_ended_ = false;
+    audio_renderer_->StartPlaying();
+  }
 }
 
 void RendererImpl::SetPlaybackRate(double playback_rate) {
@@ -724,6 +728,7 @@ void RendererImpl::PausePlayback() {
     case STATE_UNINITIALIZED:
     case STATE_INIT_PENDING_CDM:
     case STATE_INITIALIZING:
+    case STATE_SHUTDOWN:
       NOTREACHED() << "Invalid state: " << state_;
       break;