Split a portion of BufferedResourceLoader into a separate class ActiveLoader.

ActiveLoader encapsulates an active WebURLLoader and takes care of maintaining deferred status, references to parent object, and automatic cancelation during teardown.

As a result of fixing the imbalanced reference counts to BufferedResourceLoader there were a few use-after-free bugs due to doing work after executing callbacks. The ordering has been updated to ensure that no more work is done after executing callbacks.

BUG=100914

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

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

7 files changed, 188 insertions, 131 deletions

diff --git a/webkit/media/active_loader.cc b/webkit/media/active_loader.cc
new file mode 100644
index 0000000..c713fe1
--- /dev/null
+++ b/webkit/media/active_loader.cc
@@ -0,0 +1,37 @@
+// Copyright (c) 2011 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 "webkit/media/active_loader.h"
+
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebURLLoader.h"
+#include "webkit/media/buffered_resource_loader.h"
+
+namespace webkit_media {
+
+ActiveLoader::ActiveLoader(
+    const scoped_refptr<BufferedResourceLoader>& parent,
+    WebKit::WebURLLoader* loader)
+    : parent_(parent),
+      loader_(loader),
+      deferred_(false) {
+}
+
+ActiveLoader::~ActiveLoader() {
+  if (parent_)
+    Cancel();
+}
+
+void ActiveLoader::SetDeferred(bool deferred) {
+  deferred_ = deferred;
+  loader_->setDefersLoading(deferred);
+}
+
+void ActiveLoader::Cancel() {
+  // We only need to maintain a reference to our parent while the loader is
+  // still active. Failing to do so can result in circular refcounts.
+  loader_->cancel();
+  parent_ = NULL;
+}
+
+}  // namespace webkit_media
diff --git a/webkit/media/active_loader.h b/webkit/media/active_loader.h
new file mode 100644
index 0000000..18c220c
--- /dev/null
+++ b/webkit/media/active_loader.h
@@ -0,0 +1,53 @@
+// Copyright (c) 2011 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.
+
+#ifndef WEBKIT_MEDIA_ACTIVE_LOADER_H_
+#define WEBKIT_MEDIA_ACTIVE_LOADER_H_
+
+#include "base/basictypes.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace WebKit {
+class WebURLLoader;
+}
+
+namespace webkit_media {
+
+class BufferedResourceLoader;
+
+// Wraps an active WebURLLoader with some additional state and maintains a
+// reference to its parent.
+//
+// Handles deferring and deletion of loaders.
+class ActiveLoader {
+ public:
+  // Creates an ActiveLoader with a reference to its parent. The initial state
+  // assumes that the loader has started loading and has not been deferred.
+  //
+  // ActiveLoader takes ownership of |loader|.
+  ActiveLoader(const scoped_refptr<BufferedResourceLoader>& parent,
+               WebKit::WebURLLoader* loader);
+  ~ActiveLoader();
+
+  // Starts or stops deferring the resource load.
+  void SetDeferred(bool deferred);
+  bool deferred() { return deferred_; }
+
+  // Cancels the URL loader associated with this object.
+  void Cancel();
+
+ private:
+  friend class BufferedDataSourceTest;
+
+  scoped_refptr<BufferedResourceLoader> parent_;
+  scoped_ptr<WebKit::WebURLLoader> loader_;
+  bool deferred_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ActiveLoader);
+};
+
+}  // namespace webkit_media
+
+#endif  // WEBKIT_MEDIA_ACTIVE_LOADER_H_
diff --git a/webkit/media/buffered_data_source_unittest.cc b/webkit/media/buffered_data_source_unittest.cc
index b32bf5c..42abe5d 100644
--- a/webkit/media/buffered_data_source_unittest.cc
+++ b/webkit/media/buffered_data_source_unittest.cc
@@ -17,7 +17,6 @@
 using ::testing::_;
 using ::testing::Assign;
 using ::testing::Invoke;
-using ::testing::Mock;
 using ::testing::StrictMock;
 using ::testing::NiceMock;
 
@@ -56,14 +55,6 @@ class MockBufferedDataSource : public BufferedDataSource {
     ON_CALL(*url_loader, cancel())
         .WillByDefault(Assign(&loading_, false));
 
-    // TODO(scherkus): this is a real leak detected by http://crbug.com/100914
-    // but the fix will have to wait for a more invasive follow up patch.
-    //
-    // If you're curious what the fix is, we no longer need the reference
-    // counting added to BufferedResourceLoader in r23274 since we started
-    // using WebURLLoader in r69429.
-    Mock::AllowLeak(url_loader);
-
     loader->SetURLLoaderForTest(url_loader);
     return loader;
   }
@@ -162,8 +153,12 @@ class BufferedDataSourceTest : public testing::Test {
   }
 
   // Accessors for private variables on |data_source_|.
-  BufferedResourceLoader* loader() { return data_source_->loader_.get(); }
-  WebURLLoader* url_loader() { return loader()->url_loader_.get(); }
+  BufferedResourceLoader* loader() {
+    return data_source_->loader_.get();
+  }
+  WebURLLoader* url_loader() {
+    return loader()->active_loader_->loader_.get();
+  }
 
   media::Preload preload() { return data_source_->preload_; }
   BufferedResourceLoader::DeferStrategy defer_strategy() {
@@ -405,6 +400,10 @@ TEST_F(BufferedDataSourceTest, SetBitrate) {
   EXPECT_NE(old_loader, loader());
   EXPECT_EQ(1234, loader_bitrate());
 
+  // During teardown we'll also report our final network status.
+  EXPECT_CALL(host_, SetNetworkActivity(true));
+  EXPECT_CALL(host_, SetBufferedBytes(4000000));
+
   EXPECT_TRUE(data_source_->loading());
   EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
   Stop();
@@ -427,6 +426,10 @@ TEST_F(BufferedDataSourceTest, SetPlaybackRate) {
   // Verify loader changed but still has same playback rate.
   EXPECT_NE(old_loader, loader());
 
+  // During teardown we'll also report our final network status.
+  EXPECT_CALL(host_, SetNetworkActivity(true));
+  EXPECT_CALL(host_, SetBufferedBytes(4000000));
+
   EXPECT_TRUE(data_source_->loading());
   EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
   Stop();
@@ -445,7 +448,7 @@ TEST_F(BufferedDataSourceTest, Read) {
 
   // During teardown we'll also report our final network status.
   EXPECT_CALL(host_, SetBufferedBytes(kDataSize));
-  EXPECT_CALL(host_, SetNetworkActivity(false));
+  //EXPECT_CALL(host_, SetNetworkActivity(false));
 
   EXPECT_TRUE(data_source_->loading());
   Stop();
diff --git a/webkit/media/buffered_resource_loader.cc b/webkit/media/buffered_resource_loader.cc
index 3d856f9..b9248d2 100644
--- a/webkit/media/buffered_resource_loader.cc
+++ b/webkit/media/buffered_resource_loader.cc
@@ -93,7 +93,6 @@ static void ComputeTargetBufferWindow(float playback_rate, int bitrate,
     std::swap(*out_forward_capacity, *out_backward_capacity);
 }
 
-
 BufferedResourceLoader::BufferedResourceLoader(
     const GURL& url,
     int64 first_byte_position,
@@ -102,9 +101,7 @@ BufferedResourceLoader::BufferedResourceLoader(
     int bitrate,
     float playback_rate,
     media::MediaLog* media_log)
-    : deferred_(false),
-      defer_strategy_(strategy),
-      completed_(false),
+    : defer_strategy_(strategy),
       range_requested_(false),
       range_supported_(false),
       saved_forward_capacity_(0),
@@ -120,7 +117,6 @@ BufferedResourceLoader::BufferedResourceLoader(
       read_buffer_(NULL),
       first_offset_(0),
       last_offset_(0),
-      keep_test_loader_(false),
       bitrate_(bitrate),
       playback_rate_(playback_rate),
       media_log_(media_log) {
@@ -132,10 +128,7 @@ BufferedResourceLoader::BufferedResourceLoader(
   buffer_.reset(new media::SeekableBuffer(backward_capacity, forward_capacity));
 }
 
-BufferedResourceLoader::~BufferedResourceLoader() {
-  if (!completed_ && url_loader_.get())
-    url_loader_->cancel();
-}
+BufferedResourceLoader::~BufferedResourceLoader() {}
 
 void BufferedResourceLoader::Start(
     const net::CompletionCallback& start_callback,
@@ -157,10 +150,6 @@ void BufferedResourceLoader::Start(
     offset_ = first_byte_position_;
   }
 
-  // Increment the reference count right before we start the request. This
-  // reference will be release when this request has ended.
-  AddRef();
-
   // Prepare the request.
   WebURLRequest request(url_);
   request.setTargetType(WebURLRequest::TargetIsMedia);
@@ -179,17 +168,21 @@ void BufferedResourceLoader::Start(
       WebString::fromUTF8(net::HttpRequestHeaders::kAcceptEncoding),
       WebString::fromUTF8("identity;q=1, *;q=0"));
 
-  // This flag is for unittests as we don't want to reset |url_loader|
-  if (!keep_test_loader_) {
+  // Check for our test WebURLLoader.
+  WebURLLoader* loader = NULL;
+  if (test_loader_.get()) {
+    loader = test_loader_.release();
+  } else {
     WebURLLoaderOptions options;
     options.allowCredentials = true;
     options.crossOriginRequestPolicy =
         WebURLLoaderOptions::CrossOriginRequestPolicyAllow;
-    url_loader_.reset(frame->createAssociatedURLLoader(options));
+    loader = frame->createAssociatedURLLoader(options);
   }
 
   // Start the resource loading.
-  url_loader_->loadAsynchronously(request, this);
+  loader->loadAsynchronously(request, this);
+  active_loader_.reset(new ActiveLoader(this, loader));
 }
 
 void BufferedResourceLoader::Stop() {
@@ -206,16 +199,8 @@ void BufferedResourceLoader::Stop() {
   // Destroy internal buffer.
   buffer_.reset();
 
-  if (url_loader_.get()) {
-    if (deferred_)
-      url_loader_->setDefersLoading(false);
-    deferred_ = false;
-
-    if (!completed_) {
-      url_loader_->cancel();
-      completed_ = true;
-    }
-  }
+  // Cancel and reset any active loaders.
+  active_loader_.reset();
 }
 
 void BufferedResourceLoader::Read(
@@ -223,9 +208,10 @@ void BufferedResourceLoader::Read(
     int read_size,
     uint8* buffer,
     const net::CompletionCallback& read_callback) {
+  DCHECK(start_callback_.is_null());
   DCHECK(read_callback_.is_null());
-  DCHECK(buffer_.get());
   DCHECK(!read_callback.is_null());
+  DCHECK(buffer_.get());
   DCHECK(buffer);
   DCHECK_GT(read_size, 0);
 
@@ -292,9 +278,7 @@ void BufferedResourceLoader::Read(
     }
 
     // Make sure we stop deferring now that there's additional capacity.
-    //
-    // XXX: can we DCHECK(url_loader_.get()) at this point in time?
-    if (deferred_)
+    if (active_loader_->deferred())
       SetDeferred(false);
 
     DCHECK(!ShouldEnableDefer())
@@ -326,16 +310,15 @@ bool BufferedResourceLoader::range_supported() {
 }
 
 bool BufferedResourceLoader::is_downloading_data() {
-  return !completed_ && !deferred_;
+  return active_loader_.get() && !active_loader_->deferred();
 }
 
 const GURL& BufferedResourceLoader::url() {
   return url_;
 }
 
-void BufferedResourceLoader::SetURLLoaderForTest(WebURLLoader* mock_loader) {
-  url_loader_.reset(mock_loader);
-  keep_test_loader_ = true;
+void BufferedResourceLoader::SetURLLoaderForTest(WebURLLoader* test_loader) {
+  test_loader_.reset(test_loader);
 }
 
 /////////////////////////////////////////////////////////////////////////////
@@ -371,6 +354,7 @@ void BufferedResourceLoader::didReceiveResponse(
     WebURLLoader* loader,
     const WebURLResponse& response) {
   VLOG(1) << "didReceiveResponse: " << response.httpStatusCode();
+  DCHECK(active_loader_.get());
 
   // The loader may have been stopped and |start_callback| is destroyed.
   // In this case we shouldn't do anything.
@@ -408,7 +392,6 @@ void BufferedResourceLoader::didReceiveResponse(
 
     if (error != net::OK) {
       DoneStart(error);
-      Stop();
       return;
     }
   } else {
@@ -436,8 +419,7 @@ void BufferedResourceLoader::didReceiveData(
     int data_length,
     int encoded_data_length) {
   VLOG(1) << "didReceiveData: " << data_length << " bytes";
-
-  DCHECK(!completed_);
+  DCHECK(active_loader_.get());
   DCHECK_GT(data_length, 0);
 
   // If this loader has been stopped, |buffer_| would be destroyed.
@@ -485,9 +467,11 @@ void BufferedResourceLoader::didFinishLoading(
     WebURLLoader* loader,
     double finishTime) {
   VLOG(1) << "didFinishLoading";
+  DCHECK(active_loader_.get());
 
-  DCHECK(!completed_);
-  completed_ = true;
+  // We're done with the loader.
+  active_loader_.reset();
+  NotifyNetworkEvent();
 
   // If we didn't know the |instance_size_| we do now.
   if (instance_size_ == kPositionNotSpecified) {
@@ -496,10 +480,13 @@ void BufferedResourceLoader::didFinishLoading(
 
   // If there is a start callback, run it.
   if (!start_callback_.is_null()) {
+    DCHECK(read_callback_.is_null())
+        << "Shouldn't have a read callback during start";
     DoneStart(net::OK);
+    return;
   }
 
-  // If there is a pending read but the request has ended, returns with what
+  // If there is a pending read but the request has ended, return with what
   // we have.
   if (HasPendingRead()) {
     // Make sure we have a valid buffer before we satisfy a read request.
@@ -514,38 +501,30 @@ void BufferedResourceLoader::didFinishLoading(
 
   // There must not be any outstanding read request.
   DCHECK(!HasPendingRead());
-
-  // Notify that network response is completed.
-  NotifyNetworkEvent();
-
-  url_loader_.reset();
-  Release();
 }
 
 void BufferedResourceLoader::didFail(
     WebURLLoader* loader,
     const WebURLError& error) {
   VLOG(1) << "didFail: " << error.reason;
+  DCHECK(active_loader_.get());
 
-  DCHECK(!completed_);
-  completed_ = true;
+  // We don't need to continue loading after failure.
+  active_loader_->Cancel();
+  NotifyNetworkEvent();
 
-  // If there is a start callback, run it.
+  // Don't leave start callbacks hanging around.
   if (!start_callback_.is_null()) {
+    DCHECK(read_callback_.is_null())
+        << "Shouldn't have a read callback during start";
     DoneStart(error.reason);
+    return;
   }
 
-  // If there is a pending read but the request failed, return with the
-  // reason for the error.
+  // Don't leave read callbacks hanging around.
   if (HasPendingRead()) {
     DoneRead(error.reason);
   }
-
-  // Notify that network response is completed.
-  NotifyNetworkEvent();
-
-  url_loader_.reset();
-  Release();
 }
 
 bool BufferedResourceLoader::HasSingleOrigin() const {
@@ -594,26 +573,23 @@ void BufferedResourceLoader::UpdateBufferWindow() {
 }
 
 void BufferedResourceLoader::UpdateDeferBehavior() {
-  if (!url_loader_.get() || !buffer_.get())
+  if (!active_loader_.get() || !buffer_.get())
     return;
 
   // If necessary, toggle defer state and continue/pause downloading data
   // accordingly.
   if (ShouldEnableDefer() || ShouldDisableDefer())
-    SetDeferred(!deferred_);
+    SetDeferred(!active_loader_->deferred());
 }
 
 void BufferedResourceLoader::SetDeferred(bool deferred) {
-  deferred_ = deferred;
-  if (url_loader_.get()) {
-    url_loader_->setDefersLoading(deferred);
-    NotifyNetworkEvent();
-  }
+  active_loader_->SetDeferred(deferred);
+  NotifyNetworkEvent();
 }
 
-bool BufferedResourceLoader::ShouldEnableDefer() {
+bool BufferedResourceLoader::ShouldEnableDefer() const {
   // If we're already deferring, then enabling makes no sense.
-  if (deferred_)
+  if (active_loader_->deferred())
     return false;
 
   switch(defer_strategy_) {
@@ -633,9 +609,9 @@ bool BufferedResourceLoader::ShouldEnableDefer() {
   return false;
 }
 
-bool BufferedResourceLoader::ShouldDisableDefer() {
+bool BufferedResourceLoader::ShouldDisableDefer() const {
   // If we're not deferring, then disabling makes no sense.
-  if (!deferred_)
+  if (!active_loader_->deferred())
     return false;
 
   switch(defer_strategy_) {
@@ -662,7 +638,7 @@ bool BufferedResourceLoader::ShouldDisableDefer() {
   return false;
 }
 
-bool BufferedResourceLoader::CanFulfillRead() {
+bool BufferedResourceLoader::CanFulfillRead() const {
   // If we are reading too far in the backward direction.
   if (first_offset_ < 0 &&
       first_offset_ + static_cast<int>(buffer_->backward_bytes()) < 0)
@@ -674,7 +650,7 @@ bool BufferedResourceLoader::CanFulfillRead() {
 
   // At the point, we verified that first byte requested is within the buffer.
   // If the request has completed, then just returns with what we have now.
-  if (completed_)
+  if (!active_loader_.get())
     return true;
 
   // If the resource request is still active, make sure the whole requested
@@ -685,7 +661,7 @@ bool BufferedResourceLoader::CanFulfillRead() {
   return true;
 }
 
-bool BufferedResourceLoader::WillFulfillRead() {
+bool BufferedResourceLoader::WillFulfillRead() const {
   // Trying to read too far behind.
   if (first_offset_ < 0 &&
       first_offset_ + static_cast<int>(buffer_->backward_bytes()) < 0)
@@ -698,7 +674,7 @@ bool BufferedResourceLoader::WillFulfillRead() {
 
   // The resource request has completed, there's no way we can fulfill the
   // read request.
-  if (completed_)
+  if (!active_loader_.get())
     return false;
 
   return true;
@@ -764,8 +740,6 @@ std::string BufferedResourceLoader::GenerateHeaders(
 }
 
 void BufferedResourceLoader::DoneRead(int error) {
-  read_callback_.Run(error);
-  read_callback_.Reset();
   if (buffer_.get() && saved_forward_capacity_) {
     buffer_->set_forward_capacity(saved_forward_capacity_);
     saved_forward_capacity_ = 0;
@@ -776,11 +750,16 @@ void BufferedResourceLoader::DoneRead(int error) {
   first_offset_ = 0;
   last_offset_ = 0;
   Log();
+
+  net::CompletionCallback read_callback;
+  std::swap(read_callback, read_callback_);
+  read_callback.Run(error);
 }
 
 void BufferedResourceLoader::DoneStart(int error) {
-  start_callback_.Run(error);
-  start_callback_.Reset();
+  net::CompletionCallback start_callback;
+  std::swap(start_callback, start_callback_);
+  start_callback.Run(error);
 }
 
 void BufferedResourceLoader::NotifyNetworkEvent() {
diff --git a/webkit/media/buffered_resource_loader.h b/webkit/media/buffered_resource_loader.h
index 4141c79..8dfde22 100644
--- a/webkit/media/buffered_resource_loader.h
+++ b/webkit/media/buffered_resource_loader.h
@@ -17,6 +17,7 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLLoader.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLLoaderClient.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLRequest.h"
+#include "webkit/media/active_loader.h"
 #include "webkit/media/web_data_source.h"
 #include "webkit/media/webmediaplayer_impl.h"
 
@@ -83,7 +84,10 @@ class BufferedResourceLoader
                      const base::Closure& event_callback,
                      WebKit::WebFrame* frame);
 
-  // Stop this loader, cancels and request and release internal buffer.
+  // Stops everything associated with this loader, including active URL loads
+  // and pending callbacks.
+  //
+  // It is safe to delete a BufferedResourceLoader after calling Stop().
   virtual void Stop();
 
   // Reads the specified |read_size| from |position| into |buffer| and when
@@ -122,8 +126,11 @@ class BufferedResourceLoader
   // Returns resulting URL.
   virtual const GURL& url();
 
-  // Used to inject a mock used for unittests.
-  virtual void SetURLLoaderForTest(WebKit::WebURLLoader* mock_loader);
+  // Transfer ownership of an existing WebURLLoader instance for
+  // testing purposes.
+  //
+  // |test_loader| will get used the next time Start() is called.
+  virtual void SetURLLoaderForTest(WebKit::WebURLLoader* test_loader);
 
   // WebKit::WebURLLoaderClient implementation.
   virtual void willSendRequest(
@@ -181,11 +188,11 @@ class BufferedResourceLoader
 
   // Returns true if we should defer resource loading, based
   // on current buffering scheme.
-  bool ShouldEnableDefer();
+  bool ShouldEnableDefer() const;
 
   // Returns true if we should enable resource loading, based
   // on current buffering scheme.
-  bool ShouldDisableDefer();
+  bool ShouldDisableDefer() const;
 
   // Updates deferring behavior based on current buffering scheme.
   void UpdateDeferBehavior();
@@ -196,10 +203,10 @@ class BufferedResourceLoader
 
   // Returns true if the current read request can be fulfilled by what is in
   // the buffer.
-  bool CanFulfillRead();
+  bool CanFulfillRead() const;
 
   // Returns true if the current read request will be fulfilled in the future.
-  bool WillFulfillRead();
+  bool WillFulfillRead() const;
 
   // Method that does the actual read and calls the |read_callback_|, assuming
   // the request range is in |buffer_|.
@@ -238,15 +245,12 @@ class BufferedResourceLoader
   // A sliding window of buffer.
   scoped_ptr<media::SeekableBuffer> buffer_;
 
-  // True if resource loading was deferred.
-  bool deferred_;
+  // Keeps track of an active WebURLLoader and associated state.
+  scoped_ptr<ActiveLoader> active_loader_;
 
   // Current buffering algorithm in place for resource loading.
   DeferStrategy defer_strategy_;
 
-  // True if resource loading has completed.
-  bool completed_;
-
   // True if a range request was made.
   bool range_requested_;
 
@@ -256,9 +260,6 @@ class BufferedResourceLoader
   // Forward capacity to reset to after an extension.
   size_t saved_forward_capacity_;
 
-  // Does the work of loading and sends data back to this client.
-  scoped_ptr<WebKit::WebURLLoader> url_loader_;
-
   GURL url_;
   int64 first_byte_position_;
   int64 last_byte_position_;
@@ -285,8 +286,8 @@ class BufferedResourceLoader
   int first_offset_;
   int last_offset_;
 
-  // Used to ensure mocks for unittests are used instead of reset in Start().
-  bool keep_test_loader_;
+  // Injected WebURLLoader instance for testing purposes.
+  scoped_ptr<WebKit::WebURLLoader> test_loader_;
 
   // Bitrate of the media. Set to 0 if unknown.
   int bitrate_;
diff --git a/webkit/media/buffered_resource_loader_unittest.cc b/webkit/media/buffered_resource_loader_unittest.cc
index b68b3da..92aa97e 100644
--- a/webkit/media/buffered_resource_loader_unittest.cc
+++ b/webkit/media/buffered_resource_loader_unittest.cc
@@ -53,15 +53,6 @@ enum NetworkState {
   LOADING
 };
 
-// Submit a request completed event to the resource loader due to request
-// being canceled. Pretending the event is from external.
-ACTION_P(RequestCanceled, loader) {
-  WebURLError error;
-  error.reason = net::ERR_ABORTED;
-  error.domain = WebString::fromUTF8(net::kErrorDomain);
-  loader->didFail(NULL, error);
-}
-
 // Predicate that tests that request disallows compressed data.
 static bool CorrectAcceptEncoding(const WebKit::WebURLRequest &request) {
   std::string value = request.httpHeaderField(
@@ -121,10 +112,6 @@ class BufferedResourceLoaderTest : public testing::Test {
 
   void FullResponse(int64 instance_size, int status) {
     EXPECT_CALL(*this, StartCallback(status));
-    if (status != net::OK) {
-      EXPECT_CALL(*url_loader_, cancel())
-          .WillOnce(RequestCanceled(loader_));
-    }
 
     WebURLResponse response(gurl_);
     response.setHTTPHeaderField(WebString::fromUTF8("Content-Length"),
@@ -202,8 +189,7 @@ class BufferedResourceLoaderTest : public testing::Test {
 
   void StopWhenLoad() {
     InSequence s;
-    EXPECT_CALL(*url_loader_, cancel())
-        .WillOnce(RequestCanceled(loader_));
+    EXPECT_CALL(*url_loader_, cancel());
     loader_->Stop();
     loader_ = NULL;
   }
@@ -276,7 +262,7 @@ class BufferedResourceLoaderTest : public testing::Test {
   }
 
   void ConfirmLoaderDeferredState(bool expectedVal) {
-    EXPECT_EQ(loader_->deferred_, expectedVal);
+    EXPECT_EQ(loader_->active_loader_->deferred(), expectedVal);
   }
 
   // Makes sure the |loader_| buffer window is in a reasonable range.
@@ -329,13 +315,12 @@ TEST_F(BufferedResourceLoaderTest, BadHttpResponse) {
   Start();
 
   EXPECT_CALL(*this, StartCallback(net::ERR_FAILED));
-  EXPECT_CALL(*url_loader_, cancel())
-      .WillOnce(RequestCanceled(loader_));
 
   WebURLResponse response(gurl_);
   response.setHTTPStatusCode(404);
   response.setHTTPStatusText("Not Found\n");
   loader_->didReceiveResponse(url_loader_, response);
+  StopWhenLoad();
 }
 
 // Tests that partial content is requested but not fulfilled.
@@ -343,6 +328,7 @@ TEST_F(BufferedResourceLoaderTest, NotPartialResponse) {
   Initialize(kHttpUrl, 100, -1);
   Start();
   FullResponse(1024, net::ERR_INVALID_RESPONSE);
+  StopWhenLoad();
 }
 
 // Tests that a 200 response is received.
@@ -388,8 +374,6 @@ TEST_F(BufferedResourceLoaderTest, InvalidPartialResponse) {
   Start();
 
   EXPECT_CALL(*this, StartCallback(net::ERR_INVALID_RESPONSE));
-  EXPECT_CALL(*url_loader_, cancel())
-      .WillOnce(RequestCanceled(loader_));
 
   WebURLResponse response(gurl_);
   response.setHTTPHeaderField(WebString::fromUTF8("Content-Range"),
@@ -398,6 +382,7 @@ TEST_F(BufferedResourceLoaderTest, InvalidPartialResponse) {
   response.setExpectedContentLength(10);
   response.setHTTPStatusCode(kHttpPartialContent);
   loader_->didReceiveResponse(url_loader_, response);
+  StopWhenLoad();
 }
 
 // Tests the logic of sliding window for data buffering and reading.
@@ -529,8 +514,8 @@ TEST_F(BufferedResourceLoaderTest, ReadOutsideBuffer) {
   // The following call cannot be fulfilled now.
   ReadLoader(25, 10, buffer);
 
-  EXPECT_CALL(*this, ReadCallback(5));
   EXPECT_CALL(*this, NetworkCallback());
+  EXPECT_CALL(*this, ReadCallback(5));
   loader_->didFinishLoading(url_loader_, 0);
 }
 
@@ -543,8 +528,8 @@ TEST_F(BufferedResourceLoaderTest, RequestFailedWhenRead) {
   InSequence s;
 
   ReadLoader(10, 10, buffer);
-  EXPECT_CALL(*this, ReadCallback(net::ERR_FAILED));
   EXPECT_CALL(*this, NetworkCallback());
+  EXPECT_CALL(*this, ReadCallback(net::ERR_FAILED));
   WebURLError error;
   error.reason = net::ERR_FAILED;
   loader_->didFail(url_loader_, error);
@@ -583,9 +568,6 @@ TEST_F(BufferedResourceLoaderTest, ReadThenDeferStrategy) {
   uint8 buffer[10];
 
   // Make an outstanding read request.
-  // We should disable deferring after the read request, so expect
-  // a network event.
-  EXPECT_CALL(*this, NetworkCallback());
   ReadLoader(10, 10, buffer);
 
   // Receive almost enough data to cover, shouldn't defer.
diff --git a/webkit/media/webkit_media.gypi b/webkit/media/webkit_media.gypi
index 23f35c9..2763b47 100644
--- a/webkit/media/webkit_media.gypi
+++ b/webkit/media/webkit_media.gypi
@@ -14,6 +14,8 @@
         '<(DEPTH)/skia/skia.gyp:skia',
       ],
       'sources': [
+        'active_loader.cc',
+        'active_loader.h',
         'audio_decoder.cc',
         'audio_decoder.h',
         'buffered_data_source.cc',