Fix a memory leak that happen on some video files

The memory leak was introduced by BufferedDataSource, the cause of the leak:
1. Some video files have read patterns that BufferedResourceLoader cannot
   provide data buffer with, thus a large amount of BufferedResourceLoader
   is created and destroyed.
2. In the destruction of BufferedResourceLoader, it cancels the resource
   request and immediately deletes the ResourceLoaderBridge.
3. The request canceled in the renderer process isn't cancalled immediately
   in the browser process, at the mean time browser process still sends
   shared memory buffers to the renderer process.
4. When these shared memory buffers arrived at the renderer process, the
   ResourceLoaderBridge associated with the requst has already been destroyed,
   ResourceDispatcher can't find the associated ResourceLoaderBridge and simply
   delete the message. But the IPC message is not POD, it contains the handle
   to shared memory that should be closed.
5. End result is a lot of memory are shared to the renderer process but they
   are not closed.

Solutions:
1. Delete ResourceLoaderBridge only at OnCompletedResponse.
   Delete ResourceLoaderBridge at OnCompletedResponse, it is guranteed that
   no more shared memory buffer will arrive after this point, so it's safe
   to delete ResourceLoaderBridge. The downside is the lifetime of
   BufferedResourceLoader is prolonged until OnCompletedResponse is received,
   which may never is received in case of timeout for certain protocol..
2. Inside ResourceDispatcer, close the handles to shared memory even if
   ResourceLoaderBridge is not found.

I'm going for route 1. The problem I mentioned in 2 also sounds like a bug in
ResourceDispatcher, will talk to erickay or eroman to find a solution.

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

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

1 files changed, 25 insertions, 8 deletions

diff --git a/chrome/renderer/media/buffered_data_source.cc b/chrome/renderer/media/buffered_data_source.cc
index 939c76d..f6daf58 100644
--- a/chrome/renderer/media/buffered_data_source.cc
+++ b/chrome/renderer/media/buffered_data_source.cc
@@ -57,6 +57,7 @@ BufferedResourceLoader::BufferedResourceLoader(int route_id,
       first_byte_position_(first_byte_position),
       last_byte_position_(last_byte_position),
       render_loop_(RenderThread::current()->message_loop()) {
+  AddRef();
 }
 
 BufferedResourceLoader::~BufferedResourceLoader() {
@@ -369,22 +370,26 @@ void BufferedResourceLoader::OnReceivedData(const char* data, int len) {
 void BufferedResourceLoader::OnCompletedRequest(
     const URLRequestStatus& status, const std::string& security_info) {
   SignalComplete();
+
   // After the response has completed, we don't need the bridge any more.
   bridge_.reset();
 
   if (async_start_)
     InvokeAndResetStartCallback(status.os_error());
+  Release();
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // BufferedResourceLoader, private
 void BufferedResourceLoader::AppendToBuffer(const uint8* data, size_t size) {
-  Buffer* buffer = new Buffer(size);
+  scoped_ptr<Buffer> buffer(new Buffer(size));
   memcpy(buffer->data.get(), data, size);
   {
     AutoLock auto_lock(lock_);
-    buffers_.push_back(buffer);
-    buffered_bytes_ += size;
+    if (!stopped_) {
+      buffers_.push_back(buffer.release());
+      buffered_bytes_ += size;
+    }
   }
   buffer_event_.Signal();
 }
@@ -424,7 +429,9 @@ void BufferedResourceLoader::OnStart() {
 
 void BufferedResourceLoader::OnDestroy() {
   DCHECK(MessageLoop::current() == render_loop_);
-  bridge_.reset();
+  if (bridge_.get()) {
+    bridge_->Cancel();
+  }
 }
 
 void BufferedResourceLoader::OnEnableDeferLoading() {
@@ -547,15 +554,25 @@ size_t BufferedDataSource::Read(uint8* data, size_t size) {
         AutoLock auto_lock(lock_);
         if (stopped_)
           return DataSource::kReadError;
-        old_resource_loader = buffered_resource_loader_;
-        buffered_resource_loader_ =
+
+        // Save the reference to the old resource loader, if we have a local
+        // reference, use this local reference instead of creating a new one.
+        if (resource_loader)
+          old_resource_loader = resource_loader;
+        else
+          old_resource_loader = buffered_resource_loader_;
+
+        // Create a new resource loader.
+        resource_loader =
             new BufferedResourceLoader(delegate_->view()->routing_id(),
                                        url_, position_,
                                        kPositionNotSpecified);
-        resource_loader = buffered_resource_loader_;
+        buffered_resource_loader_ = resource_loader;
       }
-      if (old_resource_loader)
+      if (old_resource_loader) {
         old_resource_loader->Stop();
+        old_resource_loader = NULL;
+      }
       if (resource_loader) {
         if (net::OK != resource_loader->Start(NULL)) {
           // We have started a new request but failed, report that.