Fix a teardown hang caused by an Abort() call while there is a pending read.

BUG=64754
TEST=BufferedDataSourceTest.StopDoesNotUseMessageLoopForCallback , BufferedDataSourceTest.AbortDuringPendingRead

Review URL: http://codereview.chromium.org/6342018

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

3 files changed, 118 insertions, 22 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index d7a2f47..4449100 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -137,9 +137,25 @@ void BufferedDataSource::SetPlaybackRate(float playback_rate) {
 // media::DataSource implementation.
 void BufferedDataSource::Read(int64 position, size_t size, uint8* data,
                               media::DataSource::ReadCallback* read_callback) {
+  DCHECK(read_callback);
+
+  {
+    base::AutoLock auto_lock(lock_);
+    DCHECK(!read_callback_.get());
+
+    if (stop_signal_received_ || stopped_on_render_loop_) {
+      read_callback->RunWithParams(
+          Tuple1<size_t>(static_cast<size_t>(media::DataSource::kReadError)));
+      delete read_callback;
+      return;
+    }
+
+    read_callback_.reset(read_callback);
+  }
+
   render_loop_->PostTask(FROM_HERE,
       NewRunnableMethod(this, &BufferedDataSource::ReadTask,
-                        position, static_cast<int>(size), data, read_callback));
+                        position, static_cast<int>(size), data));
 }
 
 bool BufferedDataSource::GetSize(int64* size_out) {
@@ -163,10 +179,12 @@ bool BufferedDataSource::HasSingleOrigin() {
 void BufferedDataSource::Abort() {
   DCHECK(MessageLoop::current() == render_loop_);
 
-  // If we are told to abort, immediately return from any pending read
-  // with an error.
-  if (read_callback_.get()) {
-      base::AutoLock auto_lock(lock_);
+  {
+    base::AutoLock auto_lock(lock_);
+
+    // If we are told to abort, immediately return from any pending read
+    // with an error.
+    if (read_callback_.get())
       DoneRead_Locked(net::ERR_FAILED);
   }
 
@@ -212,19 +230,21 @@ void BufferedDataSource::InitializeTask() {
 }
 
 void BufferedDataSource::ReadTask(
-     int64 position, int read_size, uint8* buffer,
-     media::DataSource::ReadCallback* read_callback) {
+    int64 position,
+    int read_size,
+    uint8* buffer) {
   DCHECK(MessageLoop::current() == render_loop_);
-  if (stopped_on_render_loop_)
-    return;
+  {
+    base::AutoLock auto_lock(lock_);
+    if (stopped_on_render_loop_)
+      return;
 
-  DCHECK(!read_callback_.get());
-  DCHECK(read_callback);
+    DCHECK(read_callback_.get());
+  }
 
   // Saves the read parameters.
   read_position_ = position;
   read_size_ = read_size;
-  read_callback_.reset(read_callback);
   read_buffer_ = buffer;
   read_submitted_time_ = base::Time::Now();
   read_attempts_ = 0;
@@ -235,8 +255,14 @@ void BufferedDataSource::ReadTask(
 
 void BufferedDataSource::CleanupTask() {
   DCHECK(MessageLoop::current() == render_loop_);
-  if (stopped_on_render_loop_)
-    return;
+
+  {
+    base::AutoLock auto_lock(lock_);
+    if (stopped_on_render_loop_)
+      return;
+
+    read_callback_.reset();
+  }
 
   // Stop the watch dog.
   watch_dog_timer_.Stop();
@@ -246,7 +272,6 @@ void BufferedDataSource::CleanupTask() {
     loader_->Stop();
 
   // Reset the parameters of the current read request.
-  read_callback_.reset();
   read_position_ = 0;
   read_size_ = 0;
   read_buffer_ = 0;
@@ -262,9 +287,12 @@ void BufferedDataSource::RestartLoadingTask() {
   if (stopped_on_render_loop_)
     return;
 
-  // If there's no outstanding read then return early.
-  if (!read_callback_.get())
-    return;
+  {
+    // If there's no outstanding read then return early.
+    base::AutoLock auto_lock(lock_);
+    if (!read_callback_.get())
+      return;
+  }
 
   loader_ = CreateResourceLoader(read_position_, kPositionNotSpecified);
   loader_->SetAllowDefer(!media_is_paused_);
@@ -280,8 +308,11 @@ void BufferedDataSource::WatchDogTask() {
     return;
 
   // We only care if there is an active read request.
-  if (!read_callback_.get())
-    return;
+  {
+    base::AutoLock auto_lock(lock_);
+    if (!read_callback_.get())
+      return;
+  }
 
   DCHECK(loader_.get());
   base::TimeDelta delta = base::Time::Now() - read_submitted_time_;
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index c40b480..25a565a 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -62,8 +62,7 @@ class BufferedDataSource : public WebDataSource {
   void InitializeTask();
 
   // Task posted to perform actual reading on the render thread.
-  void ReadTask(int64 position, int read_size, uint8* read_buffer,
-                media::DataSource::ReadCallback* read_callback);
+  void ReadTask(int64 position, int read_size, uint8* read_buffer);
 
   // Task posted when Stop() is called. Stops |watch_dog_timer_| and
   // |loader_|, reset Read() variables, and set |stopped_on_render_loop_|
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index 6e9a87d..f370803 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -492,4 +492,70 @@ TEST_F(BufferedDataSourceTest, FileHasLoadedState) {
   StopDataSource();
 }
 
+// This test makes sure that Stop() does not require a task to run on
+// |message_loop_| before it calls its callback. This prevents accidental
+// introduction of a pipeline teardown deadlock. The pipeline owner blocks
+// the render message loop while waiting for Stop() to complete. Since this
+// object runs on the render message loop, Stop() will not complete if it
+// requires a task to run on the the message loop that is being blocked.
+TEST_F(BufferedDataSourceTest, StopDoesNotUseMessageLoopForCallback) {
+  InitializeDataSource(kFileUrl, net::OK, true, 1024, LOADED);
+
+  // Create a callback that lets us verify that it was called before
+  // Stop() returns. This is to make sure that the callback does not
+  // require |message_loop_| to execute tasks before being called.
+  media::MockCallback* stop_callback = media::NewExpectedCallback();
+  bool stop_done_called = false;
+  ON_CALL(*stop_callback, RunWithParams(_))
+      .WillByDefault(Assign(&stop_done_called, true));
+
+  // Stop() the data source like normal.
+  data_source_->Stop(stop_callback);
+
+  // Verify that the callback was called inside the Stop() call.
+  EXPECT_TRUE(stop_done_called);
+
+  message_loop_->RunAllPending();
+}
+
+TEST_F(BufferedDataSourceTest, AbortDuringPendingRead) {
+  InitializeDataSource(kFileUrl, net::OK, true, 1024, LOADED);
+
+  // Setup a way to verify that Read() is not called on the loader.
+  // We are doing this to make sure that the ReadTask() is still on
+  // the message loop queue when Abort() is called.
+  bool read_called = false;
+  ON_CALL(*loader_, Read(_, _, _ , _))
+      .WillByDefault(DoAll(Assign(&read_called, true),
+                           DeleteArg<3>()));
+
+  // Initiate a Read() on the data source, but don't allow the
+  // message loop to run.
+  data_source_->Read(
+      0, 10, buffer_,
+      NewCallback(static_cast<BufferedDataSourceTest*>(this),
+                  &BufferedDataSourceTest::ReadCallback));
+
+  // Call Abort() with the read pending.
+  EXPECT_CALL(*this, ReadCallback(-1));
+  EXPECT_CALL(*loader_, Stop());
+  data_source_->Abort();
+
+  // Verify that Read()'s after the Abort() issue callback with an error.
+  EXPECT_CALL(*this, ReadCallback(-1));
+  data_source_->Read(
+      0, 10, buffer_,
+      NewCallback(static_cast<BufferedDataSourceTest*>(this),
+                  &BufferedDataSourceTest::ReadCallback));
+
+  // Stop() the data source like normal.
+  data_source_->Stop(media::NewExpectedCallback());
+
+  // Allow cleanup task to run.
+  message_loop_->RunAllPending();
+
+  // Verify that Read() was not called on the loader.
+  EXPECT_FALSE(read_called);
+}
+
 }  // namespace webkit_glue