Pipeline: Use WeakPtr for DemuxerHost Calls and Tasks.

There is a chance that the Demuxer calls host_->OnDemuxerError() right
before Stop() is called. The Pipeline always posts a ErrorChangedTask() for
OnDemuxerError() with base::Retained(this).

After the Demuxer fires the stop callback, the Pipeline could be destroyed
immediately. So If the media thread hasn't been destroyed we could end up with
running ErrorChangedTask() on null pipeline which causes a crash.

This CL uses a weak pointer for DemuxerHost calls so that no task will run
after the pipeline is destroyed.

BUG=397656, 399417, 365141
TEST=Updated unit tests to cover this case.
R=scherkus@chromium.org

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

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

2 files changed, 45 insertions, 42 deletions

diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 91ac2cf..c144682 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -72,12 +72,6 @@ Pipeline::~Pipeline() {
       media_log_->CreateEvent(MediaLogEvent::PIPELINE_DESTROYED));
 }
 
-// The base::Unretained(this) in these functions are safe because:
-// 1, No public methods (except for the dtor) should be called after Stop().
-// 2, |this| will not be destructed until the stop callback is fired.
-// 3, Stop() also posts StopTask(), hence all XxxTask() will be executed before
-//    StopTask(), and therefore before the dtor.
-
 void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const base::Closure& ended_cb,
                      const PipelineStatusCB& error_cb,
@@ -104,14 +98,14 @@ void Pipeline::Start(scoped_ptr<FilterCollection> collection,
   duration_change_cb_ = duration_change_cb;
 
   task_runner_->PostTask(
-      FROM_HERE, base::Bind(&Pipeline::StartTask, base::Unretained(this)));
+      FROM_HERE, base::Bind(&Pipeline::StartTask, weak_factory_.GetWeakPtr()));
 }
 
 void Pipeline::Stop(const base::Closure& stop_cb) {
   DVLOG(2) << __FUNCTION__;
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&Pipeline::StopTask, base::Unretained(this), stop_cb));
+      base::Bind(&Pipeline::StopTask, weak_factory_.GetWeakPtr(), stop_cb));
 }
 
 void Pipeline::Seek(TimeDelta time, const PipelineStatusCB& seek_cb) {
@@ -123,7 +117,8 @@ void Pipeline::Seek(TimeDelta time, const PipelineStatusCB& seek_cb) {
 
   task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&Pipeline::SeekTask, base::Unretained(this), time, seek_cb));
+      base::Bind(
+          &Pipeline::SeekTask, weak_factory_.GetWeakPtr(), time, seek_cb));
 }
 
 bool Pipeline::IsRunning() const {
@@ -145,7 +140,7 @@ void Pipeline::SetPlaybackRate(float playback_rate) {
   if (running_) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&Pipeline::PlaybackRateChangedTask,
-                                      base::Unretained(this),
+                                      weak_factory_.GetWeakPtr(),
                                       playback_rate));
   }
 }
@@ -165,7 +160,7 @@ void Pipeline::SetVolume(float volume) {
     task_runner_->PostTask(
         FROM_HERE,
         base::Bind(
-            &Pipeline::VolumeChangedTask, base::Unretained(this), volume));
+            &Pipeline::VolumeChangedTask, weak_factory_.GetWeakPtr(), volume));
   }
 }
 
@@ -269,16 +264,10 @@ Pipeline::State Pipeline::GetNextState() const {
   return state_;
 }
 
-// The use of base::Unretained(this) in the following 3 functions is safe
-// because these functions are called by the Demuxer directly, before the stop
-// callback is posted by the Demuxer. So the posted tasks will always be
-// executed before the stop callback is executed, and hence before the Pipeline
-// is destructed.
-
 void Pipeline::OnDemuxerError(PipelineStatus error) {
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&Pipeline::ErrorChangedTask,
-                                    base::Unretained(this),
+                                    weak_factory_.GetWeakPtr(),
                                     error));
 }
 
@@ -286,7 +275,7 @@ void Pipeline::AddTextStream(DemuxerStream* text_stream,
                              const TextTrackConfig& config) {
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&Pipeline::AddTextStreamTask,
-                                    base::Unretained(this),
+                                    weak_factory_.GetWeakPtr(),
                                     text_stream,
                                     config));
 }
@@ -294,7 +283,7 @@ void Pipeline::AddTextStream(DemuxerStream* text_stream,
 void Pipeline::RemoveTextStream(DemuxerStream* text_stream) {
   task_runner_->PostTask(FROM_HERE,
                          base::Bind(&Pipeline::RemoveTextStreamTask,
-                                    base::Unretained(this),
+                                    weak_factory_.GetWeakPtr(),
                                     text_stream));
 }
 
@@ -558,6 +547,11 @@ void Pipeline::OnStopCompleted(PipelineStatus status) {
   }
   if (!stop_cb_.is_null()) {
     error_cb_.Reset();
+
+    // Invalid all weak pointers so it's safe to destroy |this| on the render
+    // main thread.
+    weak_factory_.InvalidateWeakPtrs();
+
     base::ResetAndReturn(&stop_cb_).Run();
 
     // NOTE: pipeline may be deleted at this point in time as a result of
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index 70c35bb..412fd88 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -122,7 +122,7 @@ class PipelineTest : public ::testing::Test {
     if (!pipeline_ || !pipeline_->IsRunning())
       return;
 
-    ExpectStop();
+    ExpectDemuxerStop();
 
     // The mock demuxer doesn't stop the fake text track stream,
     // so just stop it manually.
@@ -132,7 +132,7 @@ class PipelineTest : public ::testing::Test {
     }
 
     // Expect a stop callback if we were started.
-    EXPECT_CALL(callbacks_, OnStop());
+    ExpectPipelineStopAndDestroyPipeline();
     pipeline_->Stop(base::Bind(&CallbackHelper::OnStop,
                                base::Unretained(&callbacks_)));
     message_loop_.RunUntilIdle();
@@ -314,11 +314,22 @@ class PipelineTest : public ::testing::Test {
     EXPECT_EQ(seek_time, pipeline_->GetMediaTime());
   }
 
-  void ExpectStop() {
+  void DestroyPipeline() {
+    pipeline_.reset();
+  }
+
+  void ExpectDemuxerStop() {
     if (demuxer_)
       EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
   }
 
+  void ExpectPipelineStopAndDestroyPipeline() {
+    // After the Pipeline is stopped, it could be destroyed any time. Always
+    // destroy the pipeline immediately after OnStop() to test this.
+    EXPECT_CALL(callbacks_, OnStop())
+        .WillOnce(Invoke(this, &PipelineTest::DestroyPipeline));
+  }
+
   MOCK_METHOD2(OnAddTextTrack, void(const TextTrackConfig&,
                                     const AddTextTrackDoneCB&));
 
@@ -409,7 +420,7 @@ TEST_F(PipelineTest, NeverInitializes) {
 }
 
 TEST_F(PipelineTest, StopWithoutStart) {
-  EXPECT_CALL(callbacks_, OnStop());
+  ExpectPipelineStopAndDestroyPipeline();
   pipeline_->Stop(
       base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_)));
   message_loop_.RunUntilIdle();
@@ -435,7 +446,7 @@ TEST_F(PipelineTest, StartThenStopImmediately) {
                  base::Unretained(&callbacks_)));
 
   // Expect a stop callback if we were started.
-  EXPECT_CALL(callbacks_, OnStop());
+  ExpectPipelineStopAndDestroyPipeline();
   pipeline_->Stop(
       base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_)));
   message_loop_.RunUntilIdle();
@@ -454,7 +465,7 @@ TEST_F(PipelineTest, DemuxerErrorDuringStop) {
   EXPECT_CALL(*demuxer_, Stop(_))
       .WillOnce(DoAll(InvokeWithoutArgs(this, &PipelineTest::OnDemuxerError),
                       RunClosure<0>()));
-  EXPECT_CALL(callbacks_, OnStop());
+  ExpectPipelineStopAndDestroyPipeline();
 
   pipeline_->Stop(
       base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_)));
@@ -891,11 +902,7 @@ TEST_F(PipelineTest, AudioTimeUpdateDuringSeek) {
   EXPECT_EQ(pipeline_->GetMediaTime(), new_time);
 }
 
-static void DeletePipeline(scoped_ptr<Pipeline> pipeline) {
-  // |pipeline| will go out of scope.
-}
-
-TEST_F(PipelineTest, DeleteAfterStop) {
+TEST_F(PipelineTest, DestroyAfterStop) {
   CreateAudioStream();
   MockDemuxerStreamVector streams;
   streams.push_back(audio_stream());
@@ -903,10 +910,11 @@ TEST_F(PipelineTest, DeleteAfterStop) {
   SetAudioRendererExpectations(audio_stream());
   StartPipeline(PIPELINE_OK);
 
-  ExpectStop();
+  ExpectDemuxerStop();
 
-  Pipeline* pipeline = pipeline_.get();
-  pipeline->Stop(base::Bind(&DeletePipeline, base::Passed(&pipeline_)));
+  ExpectPipelineStopAndDestroyPipeline();
+  pipeline_->Stop(
+      base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_)));
   message_loop_.RunUntilIdle();
 }
 
@@ -959,8 +967,9 @@ TEST_F(PipelineTest, TimeUpdateAfterStop) {
 
   EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
 
-  Pipeline* pipeline = pipeline_.get();
-  pipeline->Stop(base::Bind(&DeletePipeline, base::Passed(&pipeline_)));
+  ExpectPipelineStopAndDestroyPipeline();
+  pipeline_->Stop(
+      base::Bind(&CallbackHelper::OnStop, base::Unretained(&callbacks_)));
   message_loop_.RunUntilIdle();
 }
 
@@ -1038,7 +1047,7 @@ class PipelineTeardownTest : public PipelineTest {
         EXPECT_CALL(*demuxer_, Initialize(_, _, _))
             .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb),
                             RunCallback<1>(PIPELINE_OK)));
-        EXPECT_CALL(callbacks_, OnStop());
+        ExpectPipelineStopAndDestroyPipeline();
       } else {
         status = DEMUXER_ERROR_COULD_NOT_OPEN;
         EXPECT_CALL(*demuxer_, Initialize(_, _, _))
@@ -1061,7 +1070,7 @@ class PipelineTeardownTest : public PipelineTest {
         EXPECT_CALL(*audio_renderer_, Initialize(_, _, _, _, _, _, _))
             .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb),
                             RunCallback<1>(PIPELINE_OK)));
-        EXPECT_CALL(callbacks_, OnStop());
+        ExpectPipelineStopAndDestroyPipeline();
       } else {
         status = PIPELINE_ERROR_INITIALIZATION_FAILED;
         EXPECT_CALL(*audio_renderer_, Initialize(_, _, _, _, _, _, _))
@@ -1081,7 +1090,7 @@ class PipelineTeardownTest : public PipelineTest {
         EXPECT_CALL(*video_renderer_, Initialize(_, _, _, _, _, _, _, _, _, _))
             .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb),
                             RunCallback<2>(PIPELINE_OK)));
-        EXPECT_CALL(callbacks_, OnStop());
+        ExpectPipelineStopAndDestroyPipeline();
       } else {
         status = PIPELINE_ERROR_INITIALIZATION_FAILED;
         EXPECT_CALL(*video_renderer_, Initialize(_, _, _, _, _, _, _, _, _, _))
@@ -1126,7 +1135,7 @@ class PipelineTeardownTest : public PipelineTest {
     EXPECT_CALL(callbacks_, OnSeek(status));
 
     if (status == PIPELINE_OK) {
-      EXPECT_CALL(callbacks_, OnStop());
+      ExpectPipelineStopAndDestroyPipeline();
     }
 
     pipeline_->Seek(base::TimeDelta::FromSeconds(10), base::Bind(
@@ -1195,7 +1204,7 @@ class PipelineTeardownTest : public PipelineTest {
 
     switch (stop_or_error) {
       case kStop:
-        EXPECT_CALL(callbacks_, OnStop());
+        ExpectPipelineStopAndDestroyPipeline();
         pipeline_->Stop(base::Bind(
             &CallbackHelper::OnStop, base::Unretained(&callbacks_)));
         break;
@@ -1207,7 +1216,7 @@ class PipelineTeardownTest : public PipelineTest {
 
       case kErrorAndStop:
         EXPECT_CALL(callbacks_, OnError(PIPELINE_ERROR_READ));
-        EXPECT_CALL(callbacks_, OnStop());
+        ExpectPipelineStopAndDestroyPipeline();
         pipeline_->SetErrorForTesting(PIPELINE_ERROR_READ);
         pipeline_->Stop(base::Bind(
             &CallbackHelper::OnStop, base::Unretained(&callbacks_)));