Fix race condition when accessing time_progressing_.

The |time_progressing_| variable should only be accessed via the
media thread, it can't be accessed under lock since it is set
during a call which may be reentrant to locked methods.

The fix is to use a proxy for |time_progressing_| when called from
other threads; luckily the inverse of |render_first_frame_and_stop_|
will suffice.

BUG=512371
TEST=media_unittests

Review URL: https://codereview.chromium.org/1247973002

Cr-Commit-Position: refs/heads/master@{#339817}

media/renderers/video_renderer_impl.cc

@@ -213,7 +213,18 @@ scoped_refptr<VideoFrame> VideoRendererImpl::Render(
   // end of stream, or have frames available.  We also don't want to do this in
   // background rendering mode unless this isn't the first background render
   // tick and we haven't seen any decoded frames since the last one.
-  const size_t effective_frames = MaybeFireEndedCallback();
+  //
+  // We use the inverse of |render_first_frame_and_stop_| as a proxy for the
+  // value of |time_progressing_| here since we can't access it from the
+  // compositor thread.  If we're here (in Render()) the sink must have been
+  // started -- but if it was started only to render the first frame and stop,
+  // then |time_progressing_| is likely false.  If we're still in Render() when
+  // |render_first_frame_and_stop_| is false, then |time_progressing_| is true.
+  // If |time_progressing_| is actually true when |render_first_frame_and_stop_|
+  // is also true, then the ended callback will be harmlessly delayed until
+  // MaybeStopSinkAfterFirstPaint() runs and the next Render() call comes in.
+  const size_t effective_frames =
+      MaybeFireEndedCallback_Locked(!render_first_frame_and_stop_);
   if (buffering_state_ == BUFFERING_HAVE_ENOUGH && !received_end_of_stream_ &&
       !effective_frames && (!background_rendering ||
                             (!frames_decoded_ && was_background_rendering_))) {
@@ -506,7 +517,7 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
 
       // See if we can fire EOS immediately instead of waiting for Render().
       if (use_new_video_renderering_path_)
-        MaybeFireEndedCallback();
+        MaybeFireEndedCallback_Locked(time_progressing_);
     } else {
       // Maintain the latest frame decoded so the correct frame is displayed
       // after prerolling has completed.
@@ -718,13 +729,11 @@ void VideoRendererImpl::MaybeStopSinkAfterFirstPaint() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(use_new_video_renderering_path_);
 
-  {
-    base::AutoLock auto_lock(lock_);
-    render_first_frame_and_stop_ = false;
-  }
-
   if (!time_progressing_ && sink_started_)
     StopSink();
+
+  base::AutoLock auto_lock(lock_);
+  render_first_frame_and_stop_ = false;
 }
 
 bool VideoRendererImpl::HaveReachedBufferingCap() {
@@ -758,7 +767,9 @@ void VideoRendererImpl::StopSink() {
   was_background_rendering_ = false;
 }
 
-size_t VideoRendererImpl::MaybeFireEndedCallback() {
+size_t VideoRendererImpl::MaybeFireEndedCallback_Locked(bool time_progressing) {
+  lock_.AssertAcquired();
+
   // If there's only one frame in the video or Render() was never called, the
   // algorithm will have one frame linger indefinitely.  So in cases where the
   // frame duration is unknown and we've received EOS, fire it once we get down
@@ -770,7 +781,7 @@ size_t VideoRendererImpl::MaybeFireEndedCallback() {
     return effective_frames;
 
   // Don't fire ended if time isn't moving and we have frames.
-  if (!time_progressing_ && algorithm_->frames_queued())
+  if (!time_progressing && algorithm_->frames_queued())
     return effective_frames;
 
   // Fire ended if we have no more effective frames or only ever had one frame.

media/renderers/video_renderer_impl.h

@@ -147,8 +147,15 @@ class MEDIA_EXPORT VideoRendererImpl
 
   // Fires |ended_cb_| if there are no remaining usable frames and
   // |received_end_of_stream_| is true.  Sets |rendered_end_of_stream_| if it
-  // does so.  Returns algorithm_->EffectiveFramesQueued().
-  size_t MaybeFireEndedCallback();
+  // does so.
+  //
+  // When called from the media thread, |time_progressing| should reflect the
+  // value of |time_progressing_|.  When called from Render() on the sink
+  // callback thread, the inverse of |render_first_frame_and_stop_| should be
+  // used as a proxy for |time_progressing_|.
+  //
+  // Returns algorithm_->EffectiveFramesQueued().
+  size_t MaybeFireEndedCallback_Locked(bool time_progressing);
 
   // Helper method for converting a single media timestamp to wall clock time.
   base::TimeTicks ConvertMediaTimestamp(base::TimeDelta media_timestamp);
@@ -163,7 +170,8 @@ class MEDIA_EXPORT VideoRendererImpl
   // Sink which calls into VideoRendererImpl via Render() for video frames.  Do
   // not call any methods on the sink while |lock_| is held or the two threads
   // might deadlock. Do not call Start() or Stop() on the sink directly, use
-  // StartSink() and StopSink() to ensure background rendering is started.
+  // StartSink() and StopSink() to ensure background rendering is started.  Only
+  // access these values on |task_runner_|.
   VideoRendererSink* const sink_;
   bool sink_started_;
 
@@ -271,7 +279,8 @@ class MEDIA_EXPORT VideoRendererImpl
   // counted.  Must be accessed under |lock_| once |sink_| is started.
   bool was_background_rendering_;
 
-  // Indicates whether or not media time is currently progressing or not.
+  // Indicates whether or not media time is currently progressing or not.  Must
+  // only be accessed from |task_runner_|.
   bool time_progressing_;
 
   // Indicates that Render() should only render the first frame and then request