Remove reference counting from media::Demuxer and friends.

media/filters/ffmpeg_demuxer_unittest.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 <algorithm>
 #include <deque>
 #include <string>
 
 #include "base/bind.h"
 #include "base/file_util.h"
@@ skipping 48 matching lines @@
 
 
 // Fixture class to facilitate writing tests.  Takes care of setting up the
 // FFmpeg, pipeline and filter host mocks.
 class FFmpegDemuxerTest : public testing::Test {
  protected:
   FFmpegDemuxerTest() {}
 
   virtual ~FFmpegDemuxerTest() {
     if (demuxer_) {
-      demuxer_->Stop(MessageLoop::QuitWhenIdleClosure());
-      message_loop_.Run();
+      WaitableMessageLoopEvent event;
+      demuxer_->Stop(event.GetClosure());
+      event.RunAndWait();
     }
 
-    demuxer_ = NULL;
+    demuxer_.reset();
   }
 
   void CreateDemuxer(const std::string& name) {
     CHECK(!demuxer_);
 
     EXPECT_CALL(host_, SetTotalBytes(_)).Times(AnyNumber());
     EXPECT_CALL(host_, AddBufferedByteRange(_, _)).Times(AnyNumber());
     EXPECT_CALL(host_, AddBufferedTimeRange(_, _)).Times(AnyNumber());
 
     CreateDataSource(name);
 
     media::FFmpegNeedKeyCB need_key_cb =
         base::Bind(&FFmpegDemuxerTest::NeedKeyCB, base::Unretained(this));
-    demuxer_ = new FFmpegDemuxer(message_loop_.message_loop_proxy(),
-                                 data_source_,
-                                 need_key_cb);
+    demuxer_.reset(new FFmpegDemuxer(message_loop_.message_loop_proxy(),
+                                     data_source_,
+                                     need_key_cb));
   }
 
   MOCK_METHOD1(CheckPoint, void(int v));
 
   void InitializeDemuxer() {
     EXPECT_CALL(host_, SetDuration(_));
     WaitableMessageLoopEvent event;
     demuxer_->Initialize(&host_, event.GetPipelineStatusCB());
     event.RunAndWaitForStatus(PIPELINE_OK);
   }
@@ skipping 46 matching lines @@
   }
 
   bool IsStreamStopped(DemuxerStream::Type type) {
     DemuxerStream* stream = demuxer_->GetStream(type);
     CHECK(stream);
     return static_cast<FFmpegDemuxerStream*>(stream)->stopped_;
   }
 
   // Fixture members.
   scoped_refptr<FileDataSource> data_source_;
-  scoped_refptr<FFmpegDemuxer> demuxer_;
+  scoped_ptr<FFmpegDemuxer> demuxer_;
   StrictMock<MockDemuxerHost> host_;
   MessageLoop message_loop_;
 
   AVFormatContext* format_context() {
     return demuxer_->glue_->format_context();
   }
 
   void ReadUntilEndOfStream() {
     // We should expect an end of stream buffer.
-    scoped_refptr<DemuxerStream> audio =
-        demuxer_->GetStream(DemuxerStream::AUDIO);
+    DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
     bool got_eos_buffer = false;
     const int kMaxBuffers = 170;
     for (int i = 0; !got_eos_buffer && i < kMaxBuffers; i++) {
       audio->Read(base::Bind(&EosOnReadDone, &got_eos_buffer));
       message_loop_.Run();
     }
 
     EXPECT_TRUE(got_eos_buffer);
   }
@@ skipping 49 matching lines @@
   WaitableMessageLoopEvent event;
   demuxer_->Initialize(&host_, event.GetPipelineStatusCB());
   event.RunAndWaitForStatus(DEMUXER_ERROR_NO_SUPPORTED_STREAMS);
 }
 
 TEST_F(FFmpegDemuxerTest, Initialize_Successful) {
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Video stream should be present.
-  scoped_refptr<DemuxerStream> stream =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(stream);
   EXPECT_EQ(DemuxerStream::VIDEO, stream->type());
 
   const VideoDecoderConfig& video_config = stream->video_decoder_config();
   EXPECT_EQ(kCodecVP8, video_config.codec());
   EXPECT_EQ(VideoFrame::YV12, video_config.format());
   EXPECT_EQ(320, video_config.coded_size().width());
   EXPECT_EQ(240, video_config.coded_size().height());
   EXPECT_EQ(0, video_config.visible_rect().x());
   EXPECT_EQ(0, video_config.visible_rect().y());
@@ skipping 28 matching lines @@
   //   Stream #1: Audio (Vorbis)
   //   Stream #2: Subtitles (SRT)
   //   Stream #3: Video (Theora)
   //   Stream #4: Audio (16-bit signed little endian PCM)
   //
   // We should only pick the first audio/video streams we come across.
   CreateDemuxer("bear-320x240-multitrack.webm");
   InitializeDemuxer();
 
   // Video stream should be VP8.
-  scoped_refptr<DemuxerStream> stream =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(stream);
   EXPECT_EQ(DemuxerStream::VIDEO, stream->type());
   EXPECT_EQ(kCodecVP8, stream->video_decoder_config().codec());
 
   // Audio stream should be Vorbis.
   stream = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(stream);
   EXPECT_EQ(DemuxerStream::AUDIO, stream->type());
   EXPECT_EQ(kCodecVorbis, stream->audio_decoder_config().codec());
 
@@ skipping 11 matching lines @@
   CreateDemuxer("bear-320x240-av_enc-av.webm");
   InitializeDemuxer();
 }
 
 TEST_F(FFmpegDemuxerTest, Read_Audio) {
   // We test that on a successful audio packet read.
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Attempt a read from the audio stream and run the message loop until done.
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   audio->Read(NewReadCB(FROM_HERE, 29, 0));
   message_loop_.Run();
 
   audio->Read(NewReadCB(FROM_HERE, 27, 3000));
   message_loop_.Run();
 }
 
 TEST_F(FFmpegDemuxerTest, Read_Video) {
   // We test that on a successful video packet read.
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Attempt a read from the video stream and run the message loop until done.
-  scoped_refptr<DemuxerStream> video =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
 
   video->Read(NewReadCB(FROM_HERE, 22084, 0));
   message_loop_.Run();
 
   video->Read(NewReadCB(FROM_HERE, 1057, 33000));
   message_loop_.Run();
 }
 
 TEST_F(FFmpegDemuxerTest, Read_VideoNonZeroStart) {
   // Test the start time is the first timestamp of the video and audio stream.
   CreateDemuxer("nonzero-start-time.webm");
   InitializeDemuxer();
 
   // Attempt a read from the video stream and run the message loop until done.
-  scoped_refptr<DemuxerStream> video =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
 
   // Check first buffer in video stream.
   video->Read(NewReadCB(FROM_HERE, 5636, 400000));
   message_loop_.Run();
 
   // Check first buffer in audio stream.
   audio->Read(NewReadCB(FROM_HERE, 165, 396000));
   message_loop_.Run();
 
   // Verify that the start time is equal to the lowest timestamp (ie the audio).
@@ skipping 16 matching lines @@
   ReadUntilEndOfStream();
 }
 
 TEST_F(FFmpegDemuxerTest, Seek) {
   // We're testing that the demuxer frees all queued packets when it receives
   // a Seek().
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Get our streams.
-  scoped_refptr<DemuxerStream> video =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(video);
   ASSERT_TRUE(audio);
 
   // Read a video packet and release it.
   video->Read(NewReadCB(FROM_HERE, 22084, 0));
   message_loop_.Run();
 
   // Issue a simple forward seek, which should discard queued packets.
   WaitableMessageLoopEvent event;
   demuxer_->Seek(base::TimeDelta::FromMicroseconds(1000000),
@@ skipping 38 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(MockReadCB);
 };
 
 TEST_F(FFmpegDemuxerTest, Stop) {
   // Tests that calling Read() on a stopped demuxer stream immediately deletes
   // the callback.
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Get our stream.
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(audio);
 
   demuxer_->Stop(NewExpectedClosure());
 
   // Expect all calls in sequence.
   InSequence s;
 
   // Create our mocked callback. The Callback created by base::Bind() will take
   // ownership of this pointer.
   StrictMock<MockReadCB>* callback = new StrictMock<MockReadCB>();
 
   // The callback should be immediately deleted.  We'll use a checkpoint to
   // verify that it has indeed been deleted.
   EXPECT_CALL(*callback, Run(DemuxerStream::kOk, IsEndOfStreamBuffer()));
   EXPECT_CALL(*callback, OnDelete());
   EXPECT_CALL(*this, CheckPoint(1));
 
   // Attempt the read...
   audio->Read(base::Bind(&MockReadCB::Run, callback));
 
   message_loop_.RunUntilIdle();
 
   // ...and verify that |callback| was deleted.
   CheckPoint(1);
 }
 
-// The streams can outlive the demuxer because the streams may still be in use

[scherkus (not reviewing), 2013/04/17 17:21:59]

this is no longer possible as it would be a UAF to destroy the demuxers yet keep
running the decoders

[acolwell GONE FROM CHROMIUM, 2013/04/17 20:24:53]

What about just removing the demuxer_ = NULL line? I believe it is still
possible to get a Read() after Stop() in the small period between the "stop
done" callback and both decoders being stopped.

[scherkus (not reviewing), 2013/04/19 01:07:22]

test added back (it's even more critical now that streams are still
ref-counted!)

-// by the decoder when the demuxer is destroyed.
-// This test verifies that DemuxerStream::Read() does not use an invalid demuxer
-// pointer (no crash occurs) and calls the callback with an EndOfStream buffer.
-TEST_F(FFmpegDemuxerTest, StreamReadAfterStopAndDemuxerDestruction) {
-  CreateDemuxer("bear-320x240.webm");
-  InitializeDemuxer();
-
-  // Get our stream.
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
-  ASSERT_TRUE(audio);
-
-  demuxer_->Stop(MessageLoop::QuitWhenIdleClosure());
-  message_loop_.Run();
-
-  // Expect all calls in sequence.
-  InSequence s;
-
-  // Create our mocked callback. The Callback created by base::Bind() will take
-  // ownership of this pointer.
-  StrictMock<MockReadCB>* callback = new StrictMock<MockReadCB>();
-
-  // The callback should be immediately deleted.  We'll use a checkpoint to
-  // verify that it has indeed been deleted.
-  EXPECT_CALL(*callback, Run(DemuxerStream::kOk, IsEndOfStreamBuffer()));
-  EXPECT_CALL(*callback, OnDelete());
-  EXPECT_CALL(*this, CheckPoint(1));
-
-  // Release the reference to the demuxer. This should also destroy it.
-  demuxer_ = NULL;
-  // |audio| now has a demuxer_ pointer to invalid memory.
-
-  // Attempt the read...
-  audio->Read(base::Bind(&MockReadCB::Run, callback));
-
-  message_loop_.RunUntilIdle();
-
-  // ...and verify that |callback| was deleted.
-  CheckPoint(1);
-}
-
 TEST_F(FFmpegDemuxerTest, DisableAudioStream) {
   // We are doing the following things here:
   // 1. Initialize the demuxer with audio and video stream.
   // 2. Send a "disable audio stream" message to the demuxer.
   // 3. Demuxer will free audio packets even if audio stream was initialized.
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
   // Submit a "disable audio stream" message to the demuxer.
   demuxer_->OnAudioRendererDisabled();
   message_loop_.RunUntilIdle();
 
   // Get our streams.
-  scoped_refptr<DemuxerStream> video =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(video);
   ASSERT_TRUE(audio);
 
   // The audio stream should have been prematurely stopped.
   EXPECT_FALSE(IsStreamStopped(DemuxerStream::VIDEO));
   EXPECT_TRUE(IsStreamStopped(DemuxerStream::AUDIO));
 
   // Attempt a read from the video stream: it should return valid data.
   video->Read(NewReadCB(FROM_HERE, 22084, 0));
   message_loop_.Run();
 
   // Attempt a read from the audio stream: it should immediately return end of
   // stream without requiring the message loop to read data.
   bool got_eos_buffer = false;
   audio->Read(base::Bind(&EosOnReadDone, &got_eos_buffer));
   message_loop_.RunUntilIdle();
   EXPECT_TRUE(got_eos_buffer);
 }
 
 // Verify that seek works properly when the WebM cues data is at the start of
 // the file instead of at the end.
 TEST_F(FFmpegDemuxerTest, SeekWithCuesBeforeFirstCluster) {
   CreateDemuxer("bear-320x240-cues-in-front.webm");
   InitializeDemuxer();
 
   // Get our streams.
-  scoped_refptr<DemuxerStream> video =
-      demuxer_->GetStream(DemuxerStream::VIDEO);
-  scoped_refptr<DemuxerStream> audio =
-      demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
+  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
   ASSERT_TRUE(video);
   ASSERT_TRUE(audio);
 
   // Read a video packet and release it.
   video->Read(NewReadCB(FROM_HERE, 22084, 0));
   message_loop_.Run();
 
   // Issue a simple forward seek, which should discard queued packets.
   WaitableMessageLoopEvent event;
   demuxer_->Seek(base::TimeDelta::FromMicroseconds(2500000),
@@ skipping 70 matching lines @@
 TEST_F(FFmpegDemuxerTest, MP4_ZeroStszEntry) {
 #if !defined(USE_PROPRIETARY_CODECS)
   return;
 #endif
   CreateDemuxer("bear-1280x720-zero-stsz-entry.mp4");
   InitializeDemuxer();
   ReadUntilEndOfStream();
 }
 
 }  // namespace media