Fold DecryptingDemuxerStream::Stop() into the dtor.

BUG=349211
TEST=Existing tests pass.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283886 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 71 insertions, 115 deletions

diff --git a/media/filters/audio_decoder_selector_unittest.cc b/media/filters/audio_decoder_selector_unittest.cc
index 2b4cce5..0e2ccfe 100644
--- a/media/filters/audio_decoder_selector_unittest.cc
+++ b/media/filters/audio_decoder_selector_unittest.cc
@@ -132,16 +132,21 @@ class AudioDecoderSelectorTest : public ::testing::Test {
     NOTREACHED();
   }
 
-  // Fixture members.
-  scoped_ptr<AudioDecoderSelector> decoder_selector_;
+  // Declare |decoder_selector_| after |demuxer_stream_| and |decryptor_| since
+  // |demuxer_stream_| and |decryptor_| should outlive |decoder_selector_|.
   scoped_ptr<StrictMock<MockDemuxerStream> > demuxer_stream_;
+
   // Use NiceMock since we don't care about most of calls on the decryptor, e.g.
   // RegisterNewKeyCB().
   scoped_ptr<NiceMock<MockDecryptor> > decryptor_;
+
+  scoped_ptr<AudioDecoderSelector> decoder_selector_;
+
   StrictMock<MockAudioDecoder>* decoder_1_;
   StrictMock<MockAudioDecoder>* decoder_2_;
   ScopedVector<AudioDecoder> all_decoders_;
   scoped_ptr<AudioDecoder> selected_decoder_;
+
   base::MessageLoop message_loop_;
 
  private:
diff --git a/media/filters/decoder_selector.cc b/media/filters/decoder_selector.cc
index 077aed0..908bb55 100644
--- a/media/filters/decoder_selector.cc
+++ b/media/filters/decoder_selector.cc
@@ -116,32 +116,14 @@ void DecoderSelector<StreamType>::Abort() {
   DVLOG(2) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  // This could happen when SelectDecoder() was not called or when
-  // |select_decoder_cb_| was already posted but not fired (e.g. in the
-  // message loop queue).
-  if (select_decoder_cb_.is_null())
-    return;
-
-  // We must be trying to initialize the |decoder_| or the
-  // |decrypted_stream_|. Invalid all weak pointers so that all initialization
-  // callbacks won't fire.
+  // Invalidate all weak pointers so that pending callbacks won't fire.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
-  if (decoder_) {
-    // |decrypted_stream_| is either NULL or already initialized. We don't
-    // need to Stop() |decrypted_stream_| in either case.
-    decoder_.reset();
-    ReturnNullDecoder();
-    return;
-  }
+  decoder_.reset();
+  decrypted_stream_.reset();
 
-  if (decrypted_stream_) {
-    decrypted_stream_->Stop();
+  if (!select_decoder_cb_.is_null())
     ReturnNullDecoder();
-    return;
-  }
-
-  NOTREACHED();
 }
 
 template <DemuxerStream::Type StreamType>
diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index 66c64d3..35b3617 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -74,9 +74,6 @@ DecoderStream<StreamType>::~DecoderStream() {
   if (!reset_cb_.is_null())
     task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
-  if (decrypting_demuxer_stream_)
-    decrypting_demuxer_stream_->Stop();
-
   stream_ = NULL;
   decoder_.reset();
   decrypting_demuxer_stream_.reset();
diff --git a/media/filters/decrypting_demuxer_stream.cc b/media/filters/decrypting_demuxer_stream.cc
index e7b0f67..4ec6c53 100644
--- a/media/filters/decrypting_demuxer_stream.cc
+++ b/media/filters/decrypting_demuxer_stream.cc
@@ -74,7 +74,6 @@ void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
   DVLOG(2) << __FUNCTION__ << " - state: " << state_;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ != kUninitialized) << state_;
-  DCHECK(state_ != kStopped) << state_;
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = BindToCurrentLoop(closure);
@@ -110,35 +109,6 @@ void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
   DoReset();
 }
 
-void DecryptingDemuxerStream::Stop() {
-  DVLOG(2) << __FUNCTION__ << " - state: " << state_;
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ != kUninitialized) << state_;
-
-  // Invalidate all weak pointers so that pending callbacks won't be fired into
-  // this object.
-  weak_factory_.InvalidateWeakPtrs();
-
-  // At this point the render thread is likely paused (in WebMediaPlayerImpl's
-  // Destroy()), so running |closure| can't wait for anything that requires the
-  // render thread to process messages to complete (such as PPAPI methods).
-  if (decryptor_) {
-    decryptor_->CancelDecrypt(GetDecryptorStreamType());
-    decryptor_ = NULL;
-  }
-  if (!set_decryptor_ready_cb_.is_null())
-    base::ResetAndReturn(&set_decryptor_ready_cb_).Run(DecryptorReadyCB());
-  if (!init_cb_.is_null())
-    base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
-  if (!read_cb_.is_null())
-    base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
-  if (!reset_cb_.is_null())
-    base::ResetAndReturn(&reset_cb_).Run();
-  pending_buffer_to_decrypt_ = NULL;
-
-  state_ = kStopped;
-}
-
 AudioDecoderConfig DecryptingDemuxerStream::audio_decoder_config() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
   CHECK_EQ(demuxer_stream_->type(), AUDIO);
@@ -170,6 +140,24 @@ VideoRotation DecryptingDemuxerStream::video_rotation() {
 
 DecryptingDemuxerStream::~DecryptingDemuxerStream() {
   DVLOG(2) << __FUNCTION__ << " : state_ = " << state_;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (state_ == kUninitialized)
+    return;
+
+  if (decryptor_) {
+    decryptor_->CancelDecrypt(GetDecryptorStreamType());
+    decryptor_ = NULL;
+  }
+  if (!set_decryptor_ready_cb_.is_null())
+    base::ResetAndReturn(&set_decryptor_ready_cb_).Run(DecryptorReadyCB());
+  if (!init_cb_.is_null())
+    base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
+  if (!read_cb_.is_null())
+    base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
+  if (!reset_cb_.is_null())
+    base::ResetAndReturn(&reset_cb_).Run();
+  pending_buffer_to_decrypt_ = NULL;
 }
 
 void DecryptingDemuxerStream::SetDecryptor(Decryptor* decryptor) {
diff --git a/media/filters/decrypting_demuxer_stream.h b/media/filters/decrypting_demuxer_stream.h
index b46ee3e..c65df17 100644
--- a/media/filters/decrypting_demuxer_stream.h
+++ b/media/filters/decrypting_demuxer_stream.h
@@ -31,6 +31,8 @@ class MEDIA_EXPORT DecryptingDemuxerStream : public DemuxerStream {
   DecryptingDemuxerStream(
       const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       const SetDecryptorReadyCB& set_decryptor_ready_cb);
+
+  // Cancels all pending operations immediately and fires all pending callbacks.
   virtual ~DecryptingDemuxerStream();
 
   void Initialize(DemuxerStream* stream,
@@ -42,13 +44,6 @@ class MEDIA_EXPORT DecryptingDemuxerStream : public DemuxerStream {
   // kUninitialized if |this| hasn't been initialized, or to kIdle otherwise.
   void Reset(const base::Closure& closure);
 
-  // Cancels all pending operations immediately and fires all pending callbacks
-  // and sets the state to kStopped. Does NOT wait for any pending operations.
-  // Note: During the teardown process, media pipeline will be waiting on the
-  // render main thread. If a Decryptor depends on the render main thread
-  // (e.g. PpapiDecryptor), the pending DecryptCB would not be satisfied.
-  void Stop();
-
   // DemuxerStream implementation.
   virtual void Read(const ReadCB& read_cb) OVERRIDE;
   virtual AudioDecoderConfig audio_decoder_config() OVERRIDE;
@@ -69,8 +64,7 @@ class MEDIA_EXPORT DecryptingDemuxerStream : public DemuxerStream {
     kIdle,
     kPendingDemuxerRead,
     kPendingDecrypt,
-    kWaitingForKey,
-    kStopped
+    kWaitingForKey
   };
 
   // Callback for DecryptorHost::RequestDecryptor().
diff --git a/media/filters/decrypting_demuxer_stream_unittest.cc b/media/filters/decrypting_demuxer_stream_unittest.cc
index 27d1f33..fec5624 100644
--- a/media/filters/decrypting_demuxer_stream_unittest.cc
+++ b/media/filters/decrypting_demuxer_stream_unittest.cc
@@ -89,6 +89,13 @@ class DecryptingDemuxerStreamTest : public testing::Test {
         decrypted_buffer_(new DecoderBuffer(kFakeBufferSize)) {
   }
 
+  virtual ~DecryptingDemuxerStreamTest() {
+    if (is_decryptor_set_)
+      EXPECT_CALL(*decryptor_, CancelDecrypt(_));
+    demuxer_stream_.reset();
+    message_loop_.RunUntilIdle();
+  }
+
   void InitializeAudioAndExpectStatus(const AudioDecoderConfig& config,
                                       PipelineStatus status) {
     input_audio_stream_->set_audio_decoder_config(config);
@@ -237,15 +244,6 @@ class DecryptingDemuxerStreamTest : public testing::Test {
     message_loop_.RunUntilIdle();
   }
 
-  // Stops the |demuxer_stream_| without satisfying/aborting any pending
-  // operations.
-  void Stop() {
-    if (is_decryptor_set_)
-      EXPECT_CALL(*decryptor_, CancelDecrypt(Decryptor::kAudio));
-    demuxer_stream_->Stop();
-    message_loop_.RunUntilIdle();
-  }
-
   MOCK_METHOD1(RequestDecryptorNotification, void(const DecryptorReadyCB&));
 
   MOCK_METHOD2(BufferReady, void(DemuxerStream::Status,
@@ -379,8 +377,7 @@ TEST_F(DecryptingDemuxerStreamTest, KeyAdded_DruingPendingDecrypt) {
   message_loop_.RunUntilIdle();
 }
 
-// Test resetting when the DecryptingDemuxerStream is in kDecryptorRequested
-// state.
+// Test resetting in kDecryptorRequested state.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringDecryptorRequested) {
   // One for decryptor request, one for canceling request during Reset().
   EXPECT_CALL(*this, RequestDecryptorNotification(_))
@@ -392,22 +389,20 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringDecryptorRequested) {
   Reset();
 }
 
-// Test resetting when the DecryptingDemuxerStream is in kIdle state but has
-// not returned any buffer.
+// Test resetting in kIdle state but has not returned any buffer.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringIdleAfterInitialization) {
   Initialize();
   Reset();
 }
 
-// Test resetting when the DecryptingDemuxerStream is in kIdle state after it
-// has returned one buffer.
+// Test resetting in kIdle state after having returned one buffer.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringIdleAfterReadOneBuffer) {
   Initialize();
   EnterNormalReadingState();
   Reset();
 }
 
-// Test resetting when DecryptingDemuxerStream is in kPendingDemuxerRead state.
+// Test resetting in kPendingDemuxerRead state.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringPendingDemuxerRead) {
   Initialize();
   EnterPendingReadState();
@@ -419,7 +414,7 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringPendingDemuxerRead) {
   message_loop_.RunUntilIdle();
 }
 
-// Test resetting when the DecryptingDemuxerStream is in kPendingDecrypt state.
+// Test resetting in kPendingDecrypt state.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringPendingDecrypt) {
   Initialize();
   EnterPendingDecryptState();
@@ -429,7 +424,7 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringPendingDecrypt) {
   Reset();
 }
 
-// Test resetting when the DecryptingDemuxerStream is in kWaitingForKey state.
+// Test resetting in kWaitingForKey state.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringWaitingForKey) {
   Initialize();
   EnterWaitingForKeyState();
@@ -439,7 +434,7 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringWaitingForKey) {
   Reset();
 }
 
-// Test resetting after the DecryptingDemuxerStream has been reset.
+// Test resetting after reset.
 TEST_F(DecryptingDemuxerStreamTest, Reset_AfterReset) {
   Initialize();
   EnterNormalReadingState();
@@ -458,7 +453,7 @@ TEST_F(DecryptingDemuxerStreamTest, DemuxerRead_Aborted) {
   ReadAndExpectBufferReadyWith(DemuxerStream::kAborted, NULL);
 }
 
-// Test resetting when DecryptingDemuxerStream is waiting for an aborted read.
+// Test resetting when waiting for an aborted read.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringAbortedDemuxerRead) {
   Initialize();
   EnterPendingReadState();
@@ -487,8 +482,7 @@ TEST_F(DecryptingDemuxerStreamTest, DemuxerRead_ConfigChanged) {
   ReadAndExpectBufferReadyWith(DemuxerStream::kConfigChanged, NULL);
 }
 
-// Test resetting when DecryptingDemuxerStream is waiting for a config changed
-// read.
+// Test resetting when waiting for a config changed read.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringConfigChangedDemuxerRead) {
   Initialize();
   EnterPendingReadState();
@@ -501,9 +495,11 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringConfigChangedDemuxerRead) {
   message_loop_.RunUntilIdle();
 }
 
-// Test stopping when the DecryptingDemuxerStream is in kDecryptorRequested
-// state.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringDecryptorRequested) {
+// The following tests test destruction in various scenarios. The destruction
+// happens in DecryptingDemuxerStreamTest's dtor.
+
+// Test destruction in kDecryptorRequested state.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringDecryptorRequested) {
   // One for decryptor request, one for canceling request during Reset().
   EXPECT_CALL(*this, RequestDecryptorNotification(_))
       .Times(2);
@@ -511,57 +507,48 @@ TEST_F(DecryptingDemuxerStreamTest, Stop_DuringDecryptorRequested) {
       kCodecVorbis, kSampleFormatPlanarF32, CHANNEL_LAYOUT_STEREO, 44100,
       NULL, 0, true);
   InitializeAudioAndExpectStatus(input_config, PIPELINE_ERROR_ABORT);
-  Stop();
 }
 
-// Test stopping when the DecryptingDemuxerStream is in kIdle state but has
-// not returned any buffer.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringIdleAfterInitialization) {
+// Test destruction in kIdle state but has not returned any buffer.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringIdleAfterInitialization) {
   Initialize();
-  Stop();
 }
 
-// Test stopping when the DecryptingDemuxerStream is in kIdle state after it
-// has returned one buffer.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringIdleAfterReadOneBuffer) {
+// Test destruction in kIdle state after having returned one buffer.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringIdleAfterReadOneBuffer) {
   Initialize();
   EnterNormalReadingState();
-  Stop();
 }
 
-// Test stopping when DecryptingDemuxerStream is in kPendingDemuxerRead state.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringPendingDemuxerRead) {
+// Test destruction in kPendingDemuxerRead state.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringPendingDemuxerRead) {
   Initialize();
   EnterPendingReadState();
 
   EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
-  Stop();
 }
 
-// Test stopping when the DecryptingDemuxerStream is in kPendingDecrypt state.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringPendingDecrypt) {
+// Test destruction in kPendingDecrypt state.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringPendingDecrypt) {
   Initialize();
   EnterPendingDecryptState();
 
   EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
-  Stop();
 }
 
-// Test stopping when the DecryptingDemuxerStream is in kWaitingForKey state.
-TEST_F(DecryptingDemuxerStreamTest, Stop_DuringWaitingForKey) {
+// Test destruction in kWaitingForKey state.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_DuringWaitingForKey) {
   Initialize();
   EnterWaitingForKeyState();
 
   EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
-  Stop();
 }
 
-// Test stopping after the DecryptingDemuxerStream has been reset.
-TEST_F(DecryptingDemuxerStreamTest, Stop_AfterReset) {
+// Test destruction after reset.
+TEST_F(DecryptingDemuxerStreamTest, Destroy_AfterReset) {
   Initialize();
   EnterNormalReadingState();
   Reset();
-  Stop();
 }
 
 }  // namespace media
diff --git a/media/filters/video_decoder_selector_unittest.cc b/media/filters/video_decoder_selector_unittest.cc
index 7a9580e..6f89185 100644
--- a/media/filters/video_decoder_selector_unittest.cc
+++ b/media/filters/video_decoder_selector_unittest.cc
@@ -128,16 +128,19 @@ class VideoDecoderSelectorTest : public ::testing::Test {
     NOTREACHED();
   }
 
-  // Fixture members.
-  scoped_ptr<VideoDecoderSelector> decoder_selector_;
+  // Declare |decoder_selector_| after |demuxer_stream_| and |decryptor_| since
+  // |demuxer_stream_| and |decryptor_| should outlive |decoder_selector_|.
   scoped_ptr<StrictMock<MockDemuxerStream> > demuxer_stream_;
+
   // Use NiceMock since we don't care about most of calls on the decryptor, e.g.
   // RegisterNewKeyCB().
   scoped_ptr<NiceMock<MockDecryptor> > decryptor_;
+
+  scoped_ptr<VideoDecoderSelector> decoder_selector_;
+
   StrictMock<MockVideoDecoder>* decoder_1_;
   StrictMock<MockVideoDecoder>* decoder_2_;
   ScopedVector<VideoDecoder> all_decoders_;
-
   scoped_ptr<VideoDecoder> selected_decoder_;
 
   base::MessageLoop message_loop_;