Refcounting BufferedResourceLoader

BUG=18677

We do range request for <video> and <audio>, we used
to destroy the ResourceLoaderBridge right after we
cancel the request. But it turns out that this would
leave an excessive amount of warnings in the renderer
and also leaking of URLRequest in the browser.

By refcounting BufferedResourceLoader (the container
of ResourceLoaderBridge) that does resource loading
for <video>, we can then destroy the
ResourceLoaderBridge after request has completed.

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

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

3 files changed, 88 insertions, 74 deletions

diff --git a/webkit/glue/media/buffered_data_source.cc b/webkit/glue/media/buffered_data_source.cc
index 4dd9287..5f739ba 100644
--- a/webkit/glue/media/buffered_data_source.cc
+++ b/webkit/glue/media/buffered_data_source.cc
@@ -122,6 +122,10 @@ void BufferedResourceLoader::Start(net::CompletionCallback* start_callback) {
                                               first_byte_position_,
                                               last_byte_position_));
 
+  // Increment the reference count right before we start the request. This
+  // reference will be release when this request has ended.
+  AddRef();
+
   // And start the resource loading.
   bridge_->Start(this);
 }
@@ -135,9 +139,13 @@ void BufferedResourceLoader::Stop() {
   buffer_.reset();
 
   if (bridge_.get()) {
-    // Cancel the resource request.
+    // Cancel the request. This method call will cancel the request
+    // asynchronously. We may still get data or messages until we receive
+    // a response completed message.
+    if (deferred_)
+      bridge_->SetDefersLoading(false);
+    deferred_ = false;
     bridge_->Cancel();
-    bridge_.reset();
   }
 }
 
@@ -198,11 +206,15 @@ bool BufferedResourceLoader::OnReceivedRedirect(
     const GURL& new_url,
     const webkit_glue::ResourceLoaderBridge::ResponseInfo& info) {
   DCHECK(bridge_.get());
-  DCHECK(start_callback_.get());
 
   // Saves the new URL.
   url_ = new_url;
 
+  // The load may have been stopped and |start_callback| is destroyed.
+  // In this case we shouldn't do anything.
+  if (!start_callback_.get())
+    return true;
+
   // If we got redirected to an unsupported protocol then stop.
   if (!IsSchemeSupported(new_url)) {
     DoneStart(net::ERR_ADDRESS_INVALID);
@@ -216,7 +228,11 @@ void BufferedResourceLoader::OnReceivedResponse(
     const webkit_glue::ResourceLoaderBridge::ResponseInfo& info,
     bool content_filtered) {
   DCHECK(bridge_.get());
-  DCHECK(start_callback_.get());
+
+  // The loader may have been stopped and |start_callback| is destroyed.
+  // In this case we shouldn't do anything.
+  if (!start_callback_.get())
+    return;
 
   int64 first_byte_position = -1;
   int64 last_byte_position = -1;
@@ -268,7 +284,11 @@ void BufferedResourceLoader::OnReceivedResponse(
 
 void BufferedResourceLoader::OnReceivedData(const char* data, int len) {
   DCHECK(bridge_.get());
-  DCHECK(buffer_.get());
+
+  // If this loader has been stopped, |buffer_| would be destroyed.
+  // In this case we shouldn't do anything.
+  if (!buffer_.get())
+    return;
 
   // Writes more data to |buffer_|.
   buffer_->Append(len, reinterpret_cast<const uint8*>(data));
@@ -285,37 +305,41 @@ void BufferedResourceLoader::OnReceivedData(const char* data, int len) {
 void BufferedResourceLoader::OnCompletedRequest(
     const URLRequestStatus& status, const std::string& security_info) {
   DCHECK(bridge_.get());
-  DCHECK(buffer_.get());
 
   // Saves the information that the request has completed.
   completed_ = true;
 
-  // After the response has completed, we don't need the bridge any more.
-  bridge_.reset();
-
   // If there is a start callback, calls it.
   if (start_callback_.get()) {
     DoneStart(status.os_error());
   }
 
-  // If there is a pending read but the request has ended, returns with what we
-  // have.
+  // If there is a pending read but the request has ended, returns with what
+  // we have.
   if (HasPendingRead()) {
-    // If the request has failed, then fail the read.
-    if (!status.is_success()) {
+    // Make sure we have a valid buffer before we satisfy a read request.
+    DCHECK(buffer_.get());
+
+    if (status.is_success()) {
+      // Try to fulfill with what is in the buffer.
+      if (CanFulfillRead())
+        ReadInternal();
+      else
+        DoneRead(net::ERR_CACHE_MISS);
+    } else {
+      // If the request has failed, then fail the read.
       DoneRead(net::ERR_FAILED);
-      return;
     }
-
-    // Otherwise try to fulfill with what is in the buffer.
-    if (CanFulfillRead())
-      ReadInternal();
-    else
-      DoneRead(net::ERR_CACHE_MISS);
   }
 
   // There must not be any outstanding read request.
-  DCHECK(!read_callback_.get());
+  DCHECK(!HasPendingRead());
+
+  // We incremented the reference count when the loader was started. We balance
+  // that reference here so that we get destroyed. This is also the only safe
+  // place to destroy the ResourceLoaderBridge.
+  bridge_.reset();
+  Release();
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -557,8 +581,8 @@ void BufferedDataSource::InitializeTask() {
   // TODO(hclam): Only request 1 byte for this probe request, it may be useful
   // that we perform a suffix range request and fetch the index. That way we
   // can minimize the number of requests made.
-  loader_.reset(CreateLoader(-1, -1));
-  probe_loader_.reset(CreateLoader(1, 1));
+  loader_ = CreateLoader(-1, -1);
+  probe_loader_ = CreateLoader(1, 1);
 
   loader_->Start(NewCallback(this, &BufferedDataSource::InitialStartCallback));
   probe_loader_->Start(
@@ -618,11 +642,12 @@ void BufferedDataSource::StopTask() {
   stop_task_finished_ = true;
 }
 
-void BufferedDataSource::SwapLoaderTask(BufferedResourceLoader* loader) {
+void BufferedDataSource::SwapLoaderTask(
+    scoped_refptr<BufferedResourceLoader> loader) {
   DCHECK(MessageLoop::current() == render_loop_);
   DCHECK(loader);
 
-  loader_.reset(loader);
+  loader_ = loader;
   loader_->Start(NewCallback(this,
                              &BufferedDataSource::PartialReadStartCallback));
 }
diff --git a/webkit/glue/media/buffered_data_source.h b/webkit/glue/media/buffered_data_source.h
index ea6736b..f3282cc 100644
--- a/webkit/glue/media/buffered_data_source.h
+++ b/webkit/glue/media/buffered_data_source.h
@@ -29,7 +29,9 @@ namespace webkit_glue {
 // resource loader bridge and does the actual resource loading. This object
 // does buffering internally, it defers the resource loading if buffer is
 // full and un-defers the resource loading if it is under buffered.
-class BufferedResourceLoader : public webkit_glue::ResourceLoaderBridge::Peer {
+class BufferedResourceLoader :
+    public base::RefCountedThreadSafe<BufferedResourceLoader>,
+    public webkit_glue::ResourceLoaderBridge::Peer {
  public:
   // |bridge_factory| - Factory to create a ResourceLoaderBridge.
   // |url| - URL for the resource to be loaded.
@@ -234,7 +236,7 @@ class BufferedDataSource : public media::DataSource {
 
   // Reset |loader_| with |loader| and starts it. This task is posted from
   // callback method from the current buffered resource loader.
-  void SwapLoaderTask(BufferedResourceLoader* loader);
+  void SwapLoaderTask(scoped_refptr<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
@@ -289,10 +291,10 @@ class BufferedDataSource : public media::DataSource {
   scoped_ptr<webkit_glue::MediaResourceLoaderBridgeFactory> bridge_factory_;
 
   // A resource loader for the media resource.
-  scoped_ptr<BufferedResourceLoader> loader_;
+  scoped_refptr<BufferedResourceLoader> loader_;
 
   // A resource loader that probes the server's ability to serve range requests.
-  scoped_ptr<BufferedResourceLoader> probe_loader_;
+  scoped_refptr<BufferedResourceLoader> probe_loader_;
 
   // Callback method from the pipeline for initialization.
   scoped_ptr<media::FilterCallback> initialize_callback_;
diff --git a/webkit/glue/media/buffered_data_source_unittest.cc b/webkit/glue/media/buffered_data_source_unittest.cc
index dbd40cf..4d9efe8 100644
--- a/webkit/glue/media/buffered_data_source_unittest.cc
+++ b/webkit/glue/media/buffered_data_source_unittest.cc
@@ -36,6 +36,14 @@ const int kDataSize = 1024;
 
 namespace webkit_glue {
 
+// Submit a request completed event to the resource loader due to request
+// being canceled. Pretending the event is from external.
+ACTION_P(RequestCanceled, loader) {
+  URLRequestStatus status;
+  status.set_status(URLRequestStatus::CANCELED);
+  loader->OnCompletedRequest(status, "");
+}
+
 class BufferedResourceLoaderTest : public testing::Test {
  public:
   BufferedResourceLoaderTest() {
@@ -56,8 +64,8 @@ class BufferedResourceLoaderTest : public testing::Test {
     first_position_ = first_position;
     last_position_ = last_position;
 
-    loader_.reset(new BufferedResourceLoader(&bridge_factory_, gurl_,
-                                             first_position_, last_position_));
+    loader_ = new BufferedResourceLoader(&bridge_factory_, gurl_,
+                                         first_position_, last_position_);
     EXPECT_EQ(gurl_.spec(), loader_->GetURLForDebugging());
   }
 
@@ -103,9 +111,10 @@ class BufferedResourceLoaderTest : public testing::Test {
 
   void StopWhenLoad() {
     InSequence s;
-    EXPECT_CALL(*bridge_, Cancel());
+    EXPECT_CALL(*bridge_, Cancel())
+        .WillOnce(RequestCanceled(loader_));
     EXPECT_CALL(*bridge_, OnDestroy())
-      .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
+        .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
     loader_->Stop();
   }
 
@@ -137,7 +146,7 @@ class BufferedResourceLoaderTest : public testing::Test {
   int64 first_position_;
   int64 last_position_;
 
-  scoped_ptr<BufferedResourceLoader> loader_;
+  scoped_refptr<BufferedResourceLoader> loader_;
   StrictMock<MockMediaResourceLoaderBridgeFactory> bridge_factory_;
   scoped_ptr<StrictMock<MockResourceLoaderBridge> > bridge_;
 
@@ -159,7 +168,8 @@ TEST_F(BufferedResourceLoaderTest, MissingHttpHeader) {
   Start();
 
   EXPECT_CALL(*this, StartCallback(net::ERR_INVALID_RESPONSE));
-  EXPECT_CALL(*bridge_, Cancel());
+  EXPECT_CALL(*bridge_, Cancel())
+      .WillOnce(RequestCanceled(loader_));
   EXPECT_CALL(*bridge_, OnDestroy())
       .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
 
@@ -173,7 +183,8 @@ TEST_F(BufferedResourceLoaderTest, BadHttpResponse) {
   Start();
 
   EXPECT_CALL(*this, StartCallback(net::ERR_FAILED));
-  EXPECT_CALL(*bridge_, Cancel());
+  EXPECT_CALL(*bridge_, Cancel())
+      .WillOnce(RequestCanceled(loader_));
   EXPECT_CALL(*bridge_, OnDestroy())
       .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
 
@@ -188,7 +199,8 @@ TEST_F(BufferedResourceLoaderTest, NotPartialRange) {
   Start();
 
   EXPECT_CALL(*this, StartCallback(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE));
-  EXPECT_CALL(*bridge_, Cancel());
+  EXPECT_CALL(*bridge_, Cancel())
+      .WillOnce(RequestCanceled(loader_));
   EXPECT_CALL(*bridge_, OnDestroy())
       .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
 
@@ -291,9 +303,9 @@ TEST_F(BufferedResourceLoaderTest, ReadOutsideBuffer) {
   // The following call cannot be fulfilled now.
   ReadLoader(25, 10, buffer);
 
+  EXPECT_CALL(*this, ReadCallback(5));
   EXPECT_CALL(*bridge_, OnDestroy())
       .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
-  EXPECT_CALL(*this, ReadCallback(5));
   URLRequestStatus status;
   status.set_status(URLRequestStatus::SUCCESS);
   loader_->OnCompletedRequest(status, "");
@@ -308,9 +320,9 @@ TEST_F(BufferedResourceLoaderTest, RequestFailedWhenRead) {
   InSequence s;
 
   ReadLoader(10, 10, buffer);
+  EXPECT_CALL(*this, ReadCallback(net::ERR_FAILED));
   EXPECT_CALL(*bridge_, OnDestroy())
       .WillOnce(Invoke(this, &BufferedResourceLoaderTest::ReleaseBridge));
-  EXPECT_CALL(*this, ReadCallback(net::ERR_FAILED));
   URLRequestStatus status;
   status.set_status(URLRequestStatus::FAILED);
   loader_->OnCompletedRequest(status, "");
@@ -323,17 +335,12 @@ class MockBufferedResourceLoader : public BufferedResourceLoader {
   MockBufferedResourceLoader() : BufferedResourceLoader() {
   }
 
-  ~MockBufferedResourceLoader() {
-    OnDestroy();
-  }
-
   MOCK_METHOD1(Start, void(net::CompletionCallback* read_callback));
   MOCK_METHOD0(Stop, void());
   MOCK_METHOD4(Read, void(int64 position, int read_size, uint8* buffer,
                           net::CompletionCallback* callback));
   MOCK_METHOD0(content_length, int64());
   MOCK_METHOD0(instance_size, int64());
-  MOCK_METHOD0(OnDestroy, void());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockBufferedResourceLoader);
@@ -384,7 +391,6 @@ class BufferedDataSourceTest : public testing::Test {
     message_loop_.reset(MessageLoop::current());
     bridge_factory_.reset(
         new StrictMock<MockMediaResourceLoaderBridgeFactory>());
-    ReleaseLoader();
     factory_ = MockBufferedDataSource::CreateFactory(message_loop_.get(),
                                                      bridge_factory_.get());
 
@@ -423,8 +429,8 @@ class BufferedDataSourceTest : public testing::Test {
     data_source_->set_host(&host_);
 
     // Creates the first mock loader to be injected.
-    loader_.reset(new StrictMock<MockBufferedResourceLoader>());
-    probe_loader_.reset(new StrictMock<MockBufferedResourceLoader>());
+    loader_ = new StrictMock<MockBufferedResourceLoader>();
+    probe_loader_ = new StrictMock<MockBufferedResourceLoader>();
 
     InSequence s;
     StrictMock<media::MockFilterCallback> callback;
@@ -503,17 +509,12 @@ class BufferedDataSourceTest : public testing::Test {
   }
 
   void StopDataSource() {
-    if (loader_.get()) {
+    if (loader_) {
       InSequence s;
       EXPECT_CALL(*loader_, Stop());
       EXPECT_CALL(*probe_loader_, Stop());
     }
 
-    EXPECT_CALL(*loader_, OnDestroy())
-       .WillOnce(Invoke(this, &BufferedDataSourceTest::ReleaseLoader));
-    EXPECT_CALL(*probe_loader_, OnDestroy())
-       .WillOnce(Invoke(this, &BufferedDataSourceTest::ReleaseProbeLoader));
-
     data_source_->Stop();
     message_loop_->RunAllPending();
   }
@@ -522,14 +523,6 @@ class BufferedDataSourceTest : public testing::Test {
     bridge_factory_.release();
   }
 
-  void ReleaseLoader() {
-    loader_.release();
-  }
-
-  void ReleaseProbeLoader() {
-    probe_loader_.release();
-  }
-
   void InvokeStartCallback(net::CompletionCallback* callback) {
     callback->RunWithParams(Tuple1<int>(error_));
     delete callback;
@@ -544,7 +537,7 @@ class BufferedDataSourceTest : public testing::Test {
   }
 
   void ReadDataSourceHit(int64 position, int size, int read_size) {
-    EXPECT_TRUE(loader_.get() != NULL);
+    EXPECT_TRUE(loader_);
 
     InSequence s;
     // Expect the read is delegated to the resource loader.
@@ -567,7 +560,7 @@ class BufferedDataSourceTest : public testing::Test {
   }
 
   void ReadDataSourceMiss(int64 position, int size) {
-    EXPECT_TRUE(loader_.get() != NULL);
+    EXPECT_TRUE(loader_);
 
     InSequence s;
     // 1. Reply with a cache miss for the read.
@@ -582,8 +575,6 @@ class BufferedDataSourceTest : public testing::Test {
     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()))
@@ -607,12 +598,11 @@ class BufferedDataSourceTest : public testing::Test {
     // 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);
+    loader_ = new_loader;
   }
 
   void ReadDataSourceFailed(int64 position, int size, int error) {
-    EXPECT_TRUE(loader_.get() != NULL);
+    EXPECT_TRUE(loader_);
 
     InSequence s;
     // 1. Expect the read is delegated to the resource loader.
@@ -646,8 +636,6 @@ class BufferedDataSourceTest : public testing::Test {
     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()))
@@ -676,16 +664,15 @@ class BufferedDataSourceTest : public testing::Test {
     // 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);
+    loader_ = new_loader;
   }
 
   MOCK_METHOD1(ReadCallback, void(size_t size));
 
   scoped_ptr<StrictMock<MockMediaResourceLoaderBridgeFactory> >
       bridge_factory_;
-  scoped_ptr<StrictMock<MockBufferedResourceLoader> > loader_;
-  scoped_ptr<StrictMock<MockBufferedResourceLoader> > probe_loader_;
+  scoped_refptr<StrictMock<MockBufferedResourceLoader> > loader_;
+  scoped_refptr<StrictMock<MockBufferedResourceLoader> > probe_loader_;
   scoped_refptr<MockBufferedDataSource > data_source_;
   scoped_refptr<media::FilterFactory> factory_;