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;
+
   // 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();

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

Hm, I'd rather have the callbacks owner know somehow the object is destroyed and
not call its methods at all. Otherwise it seems like all the current and new
public methods of RendererImpl should at least DCHECK for STATE_SHUTDOWN or
actively take it into account like you do below to avoid similar crashes in the
future.

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

Wait, are you talking about RendererImpl::flush_cb_ here? But I'm not changing
any logic related to RendererImpl::flush_cb_ at all in this CL. And yes, every
callback owner of an async operation (flush_cb_ in this case) might need to
check that the object is still in valid state, but again that's not changing
with my CL. As you can see from the implementation of ~RendererImpl both
video_renderer_ and audio_renderer_ will be null if the final flush_cb_ was
called due to async Flush pending while the RendererImpl was destroyed. I'm not
aware of any crashes happening in the methods bound to flush_cb_ here, so I
guess this means callback owners are already sufficiently careful and take all
of this into account.

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

Wait, aren't you fixing a crash where flush_cb_ is RestartVideoRenderer and
called from the dtor and crashes because it uses video_renderer_ (so it's a
counter example to your assumption in the last sentence)?

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

No, the flush_cb_ in RendererImpl is probably null, the RestartVideoRenderer is
invoked via VideoRendererImpl::flush_cb_. I agree that the pattern is very
similar (Flush is an async operation in both cases, and the flush_cb_ is invoked
from the destructor in both cases if the object is destroyed while the Flush is
pending). But I think that flush_cb_ is called from the destructor
intentionally, so that it's easier for clients to reason about async Flush (this
guarantees that Flush callback will be always called eventually, even if the
object is destroyed). And yes, this means that clients need to be more careful,
but it's probably a better tradeoff than needing to be careful about whether the
Flush callback will get invoked or not.

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

Hm, the dtor checks if flush_cb_ is null before invoking it. The crash happens
on the line invoking video_renderer_ so I'd suspect video_renderer_ is the null
pointer that causes SIGSEGV:
https://chromium.googlesource.com/chromium/src/+/57.0.2931.0/media/renderers/...

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

Yes, that's correct and doesn't contradict what I've said above. Again:
RendererImpl::flush_cb_ is null. VideoRendererImpl::flush_cb_ is non-null and is
used to invoke RendererImpl::RestartVideoPlayback. Note that VideoRendererImpl
is not RendererImpl.
The crash indeed happens because RendererImpl::video_renderer_ is null, while
the video_renderer_.reset (line 118 above) is executing.

The full crash call stack can be seen here, hopefully this will clarify things:
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.ptype%3D%...

   }
 }
 
 void RendererImpl::Initialize(DemuxerStreamProvider* demuxer_stream_provider,
                               RendererClient* client,
                               const PipelineStatusCB& init_cb) {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
   DCHECK(!init_cb.is_null());
@@ skipping 106 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());
-  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) {
   DVLOG(1) << __func__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Playback rate changes are only carried out while playing.
   if (state_ != STATE_PLAYING)
     return;
 
@@ skipping 444 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