Retry requests that has timed out in BufferedDataSource

Retries request if the read request is not fulfilled in 5 seconds and at most 3 times. This will handle the case when
a user pause a movie and starts it in a later time when the
connection to the server has already timed out.

TEST=BufferedDataSourceTest.*
BUG=17479

done

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

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

3 files changed, 134 insertions, 1 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 76e6e9d..6b71ec9 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -49,7 +49,7 @@ const int kForwardWaitThreshold = 2 * kMegabyte;
 // timeout and start a new request.
 // TODO(hclam): Use this value when retry is implemented.
 // TODO(hclam): Set it to 5s, calibrate this value later.
-const int kDataTransferTimeoutSeconds = 5;
+const int kTimeoutMilliseconds = 5000;
 
 // Defines how many times we should try to read from a buffered resource loader
 // before we declare a read error. After each failure of read from a buffered
@@ -447,6 +447,13 @@ BufferedResourceLoader* BufferedDataSource::CreateLoader(
                                     last_byte_position);
 }
 
+// This method simply returns kTimeoutMilliseconds. The purpose of this
+// method is to be overidded so as to provide a different timeout value
+// for testing purpose.
+base::TimeDelta BufferedDataSource::GetTimeoutMilliseconds() {
+  return base::TimeDelta::FromMilliseconds(kTimeoutMilliseconds);
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // BufferedDataSource, media::MediaFilter implementation
 void BufferedDataSource::Initialize(const std::string& url,
@@ -510,6 +517,14 @@ void BufferedDataSource::InitializeTask() {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(!loader_.get());
 
+  // Kick starts the watch dog task that will handle connection timeout.
+  // We run the watch dog 2 times faster the actual timeout so as to catch
+  // the timeout more accurately.
+  watch_dog_timer_.Start(
+      GetTimeoutMilliseconds() / 2,
+      this,
+      &BufferedDataSource::WatchDogTask);
+
   // Creates a new resource loader with the full range.
   loader_.reset(CreateLoader(-1, -1));
 
@@ -530,6 +545,8 @@ void BufferedDataSource::ReadTask(
   read_size_ = read_size;
   read_callback_.reset(read_callback);
   read_buffer_ = buffer;
+  read_submitted_time_ = base::Time::Now();
+  read_attempts_ = 0;
 
   // Call to read internal to perform the actual read.
   ReadInternal();
@@ -538,11 +555,22 @@ void BufferedDataSource::ReadTask(
 void BufferedDataSource::StopTask() {
   DCHECK(MessageLoop::current() == render_loop_);
 
+  // Stop the watch dog.
+  watch_dog_timer_.Stop();
+
   // We just need to stop the loader, so it stops activity.
   if (loader_.get()) {
     loader_->Stop();
     loader_.reset();
   }
+
+  // Reset the parameters of the current read request.
+  read_callback_.reset();
+  read_position_ = 0;
+  read_size_ = 0;
+  read_buffer_ = 0;
+  read_submitted_time_ = base::Time();
+  read_attempts_ = 0;
 }
 
 void BufferedDataSource::SwapLoaderTask(BufferedResourceLoader* loader) {
@@ -554,6 +582,32 @@ void BufferedDataSource::SwapLoaderTask(BufferedResourceLoader* loader) {
                              &BufferedDataSource::PartialReadStartCallback));
 }
 
+void BufferedDataSource::WatchDogTask() {
+  DCHECK(MessageLoop::current() == render_loop_);
+
+  // We only care if there is an active read request.
+  if (!read_callback_.get())
+    return;
+
+  DCHECK(loader_.get());
+  base::TimeDelta delta = base::Time::Now() - read_submitted_time_;
+  if (delta < GetTimeoutMilliseconds())
+    return;
+  
+  // TODO(hclam): Maybe raise an error here. But if an error is reported
+  // the whole pipeline may get destroyed...
+  if (read_attempts_ >= kReadTrials)
+    return;
+
+  ++read_attempts_;
+  read_submitted_time_ = base::Time::Now();
+
+  // Stops the current loader and swap in a new resource loader and
+  // retry the request.
+  loader_->Stop();
+  SwapLoaderTask(CreateLoader(read_position_, -1));
+}
+
 // This method is the place where actual read happens, |loader_| must be valid
 // prior to make this method call.
 void BufferedDataSource::ReadInternal() {
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index 5d6f1c3..0e1df1c 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -9,6 +9,7 @@
 
 #include "base/lock.h"
 #include "base/scoped_ptr.h"
+#include "base/timer.h"
 #include "base/condition_variable.h"
 #include "googleurl/src/gurl.h"
 #include "media/base/factory.h"
@@ -183,6 +184,11 @@ class BufferedDataSource : public media::DataSource {
   virtual BufferedResourceLoader* CreateLoader(int64 first_byte_position,
                                                int64 last_byte_position);
 
+  // Gets the number of milliseconds to declare a request timeout since
+  // the request was made. This method is made virtual so as to inject a
+  // different number for testing purpose.
+  virtual base::TimeDelta GetTimeoutMilliseconds();
+
  private:
   friend class media::FilterFactoryImpl2<
       BufferedDataSource,
@@ -205,6 +211,11 @@ class BufferedDataSource : public media::DataSource {
   // callback method from the current buffered resource loader.
   void SwapLoaderTask(BufferedResourceLoader* loader);
 
+  // This task monitors the current active read request. If the current read
+  // request has timed out, this task will destroy the current loader and
+  // creates a new to accomodate the read request.
+  void WatchDogTask();
+
   // The method that performs actual read. This method can only be executed on
   // the render thread.
   void ReadInternal();
@@ -252,6 +263,8 @@ class BufferedDataSource : public media::DataSource {
   int64 read_position_;
   int read_size_;
   uint8* read_buffer_;
+  base::Time read_submitted_time_;
+  int read_attempts_;
 
   // This buffer is intermediate, we use it for BufferedResourceLoader to write
   // to. And when read in BufferedResourceLoader is done, we copy data from
@@ -278,6 +291,12 @@ class BufferedDataSource : public media::DataSource {
   // Stop signal to suppressing activities.
   bool stopped_;
 
+  // This timer is to run the WatchDogTask repeatedly. We use a timer instead
+  // of doing PostDelayedTask() reduce the extra reference held by the message
+  // loop. The RepeatingTimer does PostDelayedTask() internally, by using it
+  // the message loop doesn't hold a reference for the watch dog task.
+  base::RepeatingTimer<BufferedDataSource> watch_dog_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(BufferedDataSource);
 };
 
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index 40c0b41..d25ac72 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -16,9 +16,11 @@
 
 using ::testing::_;
 using ::testing::Assign;
+using ::testing::DeleteArg;
 using ::testing::DoAll;
 using ::testing::InSequence;
 using ::testing::Invoke;
+using ::testing::InvokeWithoutArgs;
 using ::testing::NotNull;
 using ::testing::Return;
 using ::testing::SetArgumentPointee;
@@ -347,6 +349,11 @@ class MockBufferedDataSource : public BufferedDataSource {
                                            bridge_factory);
   }
 
+  virtual base::TimeDelta GetTimeoutMilliseconds() {
+    // It is 100 ms because we don't want the test to run too long.
+    return base::TimeDelta::FromMilliseconds(100);
+  }
+
   MOCK_METHOD2(CreateLoader, BufferedResourceLoader*(int64 first_position,
                                                      int64 last_position));
 
@@ -384,6 +391,7 @@ class BufferedDataSourceTest : public testing::Test {
 
   ~BufferedDataSourceTest() {
     if (data_source_) {
+      // Release the bridge factory because we don't own it.
       // Expects bridge factory to be destroyed along with data source.
       EXPECT_CALL(*bridge_factory_, OnDestroy())
           .WillOnce(Invoke(this,
@@ -568,6 +576,52 @@ class BufferedDataSourceTest : public testing::Test {
     message_loop_->RunAllPending();
   }
 
+  void ReadDataSourceTimesOut(int64 position, int size) {
+    InSequence s;
+    // 1. Drop the request and let it times out.
+    EXPECT_CALL(*loader_, Read(position, size, NotNull(), NotNull()))
+        .WillOnce(DeleteArg<3>());
+    
+    // 2. Then the current loader will be stop and destroyed.
+    StrictMock<MockBufferedResourceLoader> *new_loader =
+        new StrictMock<MockBufferedResourceLoader>();
+    EXPECT_CALL(*loader_, Stop());
+    EXPECT_CALL(*data_source_, CreateLoader(position, -1))
+        .WillOnce(Return(new_loader));
+    EXPECT_CALL(*loader_, OnDestroy())
+        .WillOnce(Invoke(this, &BufferedDataSourceTest::ReleaseLoader));
+    
+    // 3. Then the new loader will be started.
+    EXPECT_CALL(*new_loader, Start(NotNull()))
+        .WillOnce(DoAll(Assign(&error_, net::OK),
+                        Invoke(this,
+                               &BufferedDataSourceTest::InvokeStartCallback)));
+    
+    // 4. Then again a read request is made to the new loader.
+    EXPECT_CALL(*new_loader, Read(position, size, NotNull(), NotNull()))
+        .WillOnce(DoAll(Assign(&error_, size),
+                        Invoke(this,
+                               &BufferedDataSourceTest::InvokeReadCallback),
+                        InvokeWithoutArgs(message_loop_.get(),
+                                          &MessageLoop::Quit)));
+
+    EXPECT_CALL(*this, ReadCallback(size));
+    
+    data_source_->Read(
+        position, size, buffer_,
+        NewCallback(this, &BufferedDataSourceTest::ReadCallback));
+
+    // This blocks the current thread until the watch task is executed and
+    // triggers a read callback to quit this message loop.
+    message_loop_->Run();
+
+    // Make sure data is correct.
+    EXPECT_EQ(0, memcmp(buffer_, data_ + static_cast<int>(position), size));
+    
+    EXPECT_TRUE(loader_.get() == NULL);
+    loader_.reset(new_loader);
+  }
+
   MOCK_METHOD1(ReadCallback, void(size_t size));
 
   scoped_ptr<StrictMock<MockMediaResourceLoaderBridgeFactory> >
@@ -624,4 +678,10 @@ TEST_F(BufferedDataSourceTest, ReadFailed) {
   StopDataSource();
 }
 
+TEST_F(BufferedDataSourceTest, ReadTimesOut) {
+  InitializeDataSource(kHttpUrl, net::OK, 1024);
+  ReadDataSourceTimesOut(20, 10);
+  StopDataSource();
+}
+
 }  // namespace webkit_glue