Fix crash in VideoRendererBase::ThreadMain().

media/filters/video_renderer_base.cc

 // Copyright (c) 2012 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/filters/video_renderer_base.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/message_loop.h"
@@ skipping 27 matching lines @@
       drop_frames_(drop_frames),
       playback_rate_(0),
       paint_cb_(paint_cb),
       set_opaque_cb_(set_opaque_cb),
       last_timestamp_(kNoTimestamp()) {
   DCHECK(!paint_cb_.is_null());
 }
 
 VideoRendererBase::~VideoRendererBase() {
   base::AutoLock auto_lock(lock_);
-  CHECK(state_ == kUninitialized || state_ == kStopped) << state_;
+  CHECK_EQ(state_, kStopped);

[acolwell GONE FROM CHROMIUM, 2013/02/20 17:01:14]

Won't this crash if an error is encountered before the renderer is initialized.
For example, wouldn't a demuxer init error cause this to be destroyed in the
kUninitialized state w/o Stop() being called? Audio-only playback might be
another case.

[scherkus (not reviewing), 2013/02/21 20:43:54]

Indeed it will!

   CHECK_EQ(thread_, base::kNullThreadHandle);
 }
 
 void VideoRendererBase::Play(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPrerolled, state_);
   state_ = kPlaying;
   callback.Run();
 }
@@ skipping 26 matching lines @@
 void VideoRendererBase::ResetDecoder() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
   decoder_->Reset(base::Bind(
       &VideoRendererBase::OnDecoderResetDone, weak_this_));
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
-  if (state_ == kUninitialized || state_ == kStopped) {
+  if (state_ == kStopped) {

[scherkus (not reviewing), 2013/02/20 08:26:11]

note: state == kUninitialized actually runs the Stop() code path now (hence the
check for decoder_ below)

[acolwell GONE FROM CHROMIUM, 2013/02/20 17:01:14]

This seems a little weird. Shouldn't we transition to a kInitializing state
inside Initialize() somewhere? That seems to make a little more sense then
letting kUninitialized fall through here. It avoids the "If we are
uninitialized, why do we have to clean up state." train of thought when looking
at this code.

[scherkus (not reviewing), 2013/02/21 20:43:54]

Yeah that was on my mind. I fully admit that I was a bit rushed to get something
together before the flight attendant chastised me.

     callback.Run();
     return;
   }
 
   // TODO(scherkus): Consider invalidating |weak_factory_| and replacing
   // task-running guards that check |state_| with DCHECK().
 
   state_ = kStopped;
 
   statistics_cb_.Reset();
@@ skipping 13 matching lines @@
     base::AutoUnlock auto_unlock(lock_);
     base::PlatformThread::Join(thread_to_join);
   }
 
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
         &VideoRendererBase::StopDecoder, weak_this_, callback));
     return;
   }
 
-  decoder_->Stop(callback);
+  if (decoder_) {
+    decoder_->Stop(callback);
+    return;
+  }
+
+  callback.Run();
 }
 
 void VideoRendererBase::StopDecoder(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
   decoder_->Stop(callback);
 }
 
 void VideoRendererBase::SetPlaybackRate(float playback_rate) {
   DCHECK(message_loop_->BelongsToCurrentThread());
@@ skipping 380 matching lines @@
     case kFlushed:
     case kEnded:
     case kStopped:
     case kError:
       return;
   }
 }
 
 void VideoRendererBase::OnDecoderResetDone() {
   base::AutoLock auto_lock(lock_);
+  if (state_ == kStopped)
+    return;
+
   DCHECK_EQ(kFlushingDecoder, state_);
   DCHECK(!pending_read_);
 
   state_ = kFlushing;
   AttemptFlush_Locked();
 }
 
 void VideoRendererBase::AttemptFlush_Locked() {
   lock_.AssertAcquired();
   DCHECK_EQ(kFlushing, state_);
@@ skipping 27 matching lines @@
   last_timestamp_ = kNoTimestamp();
   ready_frames_.clear();
 }
 
 int VideoRendererBase::NumFrames_Locked() const {
   lock_.AssertAcquired();
   return ready_frames_.size();
 }
 
 }  // namespace media