FileAPI: Fix memory leak in WebFileSystemImpl (won't commit)

content/child/fileapi/webfilesystem_impl.cc

 // Copyright 2013 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 "content/child/fileapi/webfilesystem_impl.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ skipping 24 matching lines @@
 
 namespace content {
 
 namespace {
 
 base::LazyInstance<base::ThreadLocalPointer<WebFileSystemImpl> >::Leaky
     g_webfilesystem_tls = LAZY_INSTANCE_INITIALIZER;
 
 class WaitableCallbackResults {
  public:
-  static WaitableCallbackResults* MaybeCreate(
+  static scoped_ptr<WaitableCallbackResults> MaybeCreate(
       const WebFileSystemCallbacks& callbacks) {
     if (callbacks.shouldBlockUntilCompletion())
-      return new WaitableCallbackResults;
-    return NULL;
+      return make_scoped_ptr(new WaitableCallbackResults);
+    return scoped_ptr<WaitableCallbackResults>();
   }
   ~WaitableCallbackResults() {}
 
   void SetResultsAndSignal(const base::Closure& results_closure) {
     results_closure_ = results_closure;
-    event_->Signal();
+    event_.Signal();
   }
 
   void WaitAndRun() {
     {
       blink::WebHeap::SafePointScope safe_point;
-      event_->Wait();
+      event_.Wait();
     }
     DCHECK(!results_closure_.is_null());
     results_closure_.Run();
   }
 
  private:
-  WaitableCallbackResults() : event_(new base::WaitableEvent(true, false)) {}

[kinuko, 2014/03/05 15:23:32]

oops.

+  WaitableCallbackResults() : event_(true, false) {}
 
-  base::WaitableEvent* event_;
+  base::WaitableEvent event_;
   base::Closure results_closure_;
   DISALLOW_COPY_AND_ASSIGN(WaitableCallbackResults);
 };
 
 void DidReceiveSnapshotFile(int request_id) {
   if (ChildThread::current())
     ChildThread::current()->Send(
         new FileSystemHostMsg_DidReceiveSnapshotFile(request_id));
 }
 
@@ skipping 244 matching lines @@
 
 void WebFileSystemImpl::OnWorkerRunLoopStopped() {
   delete this;
 }
 
 void WebFileSystemImpl::openFileSystem(
     const blink::WebURL& storage_partition,
     blink::WebFileSystemType type,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::OpenFileSystem,
       MakeTuple(GURL(storage_partition),
                 static_cast<fileapi::FileSystemType>(type),
                 base::Bind(&OpenFileSystemCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results)),
+                           waitable_results.get()),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());

[nhiroki, 2014/03/05 15:16:08]

Hmm... |waitable_results| can be passed before calling get(). Will revert...

[kinuko, 2014/03/05 15:23:32]

Was about to comment about it. You could keep a raw ptr before calling
CallDispatch... (but I'm not entirely sure how the original code leaks
waitable_results)

 }
 
 void WebFileSystemImpl::resolveURL(
     const blink::WebURL& filesystem_url,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::ResolveURL,
       MakeTuple(GURL(filesystem_url),
                 base::Bind(&ResolveURLCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results)),
+                           waitable_results.get()),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::deleteFileSystem(
     const blink::WebURL& storage_partition,
     blink::WebFileSystemType type,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::DeleteFileSystem,
       MakeTuple(GURL(storage_partition),
                 static_cast<fileapi::FileSystemType>(type),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::move(
     const blink::WebURL& src_path,
     const blink::WebURL& dest_path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Move,
       MakeTuple(GURL(src_path), GURL(dest_path),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::copy(
     const blink::WebURL& src_path,
     const blink::WebURL& dest_path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Copy,
       MakeTuple(GURL(src_path), GURL(dest_path),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::remove(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Remove,
       MakeTuple(GURL(path), false /* recursive */,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::removeRecursively(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Remove,
       MakeTuple(GURL(path), true /* recursive */,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::readMetadata(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::ReadMetadata,
       MakeTuple(GURL(path),
                 base::Bind(&ReadMetadataCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results)),
+                           waitable_results.get()),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::createFile(
     const blink::WebURL& path,
     bool exclusive,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::CreateFile,
       MakeTuple(GURL(path), exclusive,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::createDirectory(
     const blink::WebURL& path,
     bool exclusive,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::CreateDirectory,
       MakeTuple(GURL(path), exclusive, false /* recursive */,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::fileExists(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Exists,
       MakeTuple(GURL(path), false /* directory */,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::directoryExists(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::Exists,
       MakeTuple(GURL(path), true /* directory */,
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::readDirectory(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::ReadDirectory,
       MakeTuple(GURL(path),
                 base::Bind(&ReadDirectoryCallbackAdapater,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results)),
+                           waitable_results.get()),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::createFileWriter(
     const WebURL& path,
     blink::WebFileWriterClient* client,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::ReadMetadata,
       MakeTuple(GURL(path),
                 base::Bind(&CreateFileWriterCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results),
+                           waitable_results.get(),
                            main_thread_loop_, GURL(path), client),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 void WebFileSystemImpl::createSnapshotFileAndReadMetadata(
     const blink::WebURL& path,
     WebFileSystemCallbacks callbacks) {
   int callbacks_id = RegisterCallbacks(callbacks);
-  WaitableCallbackResults* waitable_results =
+  scoped_ptr<WaitableCallbackResults> waitable_results =
       WaitableCallbackResults::MaybeCreate(callbacks);
   CallDispatcherOnMainThread(
       main_thread_loop_.get(),
       &FileSystemDispatcher::CreateSnapshotFile,
       MakeTuple(GURL(path),
                 base::Bind(&CreateSnapshotFileCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results),
+                           waitable_results.get(),
                            main_thread_loop_),
                 base::Bind(&StatusCallbackAdapter,
                            CurrentWorkerId(), callbacks_id,
-                           base::Unretained(waitable_results))),
-      make_scoped_ptr(waitable_results));
+                           waitable_results.get())),
+      waitable_results.Pass());
 }
 
 int WebFileSystemImpl::RegisterCallbacks(
     const WebFileSystemCallbacks& callbacks) {
   DCHECK(CalledOnValidThread());
   int id = next_callbacks_id_++;
   callbacks_[id] = callbacks;
   return id;
 }
 
 WebFileSystemCallbacks WebFileSystemImpl::GetAndUnregisterCallbacks(
     int callbacks_id) {
   DCHECK(CalledOnValidThread());
   CallbacksMap::iterator found = callbacks_.find(callbacks_id);
   DCHECK(found != callbacks_.end());
   WebFileSystemCallbacks callbacks = found->second;
   callbacks_.erase(found);
   return callbacks;
 }
 
 }  // namespace content