Avoid leaking file handles.

AsyncOpenFileCallback: fix the issue that file handle is leaked if the file handle arrives after the requester has died.

This change also fixes test_file_system.

BUG=79820
TEST=FileIO/FileRef/FileSystem tests in ppapi_tests: Reload the test page, the test still passes. The output files can be deleted when any of these tests finishes running, no matter the test page is still open or not.


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

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

8 files changed, 31 insertions, 22 deletions

diff --git a/content/browser/file_system/file_system_dispatcher_host.cc b/content/browser/file_system/file_system_dispatcher_host.cc
index 7f5be8c..e4065e1 100644
--- a/content/browser/file_system/file_system_dispatcher_host.cc
+++ b/content/browser/file_system/file_system_dispatcher_host.cc
@@ -77,19 +77,10 @@ class BrowserFileSystemCallbackDispatcher
       base::PlatformFile file,
       base::ProcessHandle peer_handle) {
     IPC::PlatformFileForTransit file_for_transit =
-        IPC::InvalidPlatformFileForTransit();
-    if (file != base::kInvalidPlatformFileValue) {
-#if defined(OS_WIN)
-      if (!::DuplicateHandle(::GetCurrentProcess(), file, peer_handle,
-                             &file_for_transit, 0, false,
-                             DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE))
-        file_for_transit = IPC::InvalidPlatformFileForTransit();
-#elif defined(OS_POSIX)
-      file_for_transit = base::FileDescriptor(file, true);
-#else
-  #error Not implemented.
-#endif
-    }
+        file != base::kInvalidPlatformFileValue ?
+            IPC::GetFileHandleForProcess(file, peer_handle, true) :
+            IPC::InvalidPlatformFileForTransit();
+
     dispatcher_host_->Send(new FileSystemMsg_DidOpenFile(
         request_id_, file_for_transit));
   }
diff --git a/content/renderer/pepper_plugin_delegate_impl.cc b/content/renderer/pepper_plugin_delegate_impl.cc
index 82968f6..325f3f8 100644
--- a/content/renderer/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper_plugin_delegate_impl.cc
@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
+#include "base/file_util_proxy.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_split.h"
@@ -914,7 +915,10 @@ void PepperPluginDelegateImpl::OnAsyncFileOpened(
       messages_waiting_replies_.Lookup(message_id);
   DCHECK(callback);
   messages_waiting_replies_.Remove(message_id);
-  callback->Run(error_code, file);
+  callback->Run(error_code, base::PassPlatformFile(&file));
+  // Make sure we won't leak file handle if the requester has died.
+  if (file != base::kInvalidPlatformFileValue)
+    base::FileUtilProxy::Close(GetFileThreadMessageLoopProxy(), file, NULL);
   delete callback;
 }
 
@@ -1024,7 +1028,8 @@ class AsyncOpenFileSystemURLCallbackTranslator
   }
 
   virtual void DidFail(base::PlatformFileError error_code) {
-    callback_->Run(error_code, base::kInvalidPlatformFileValue);
+    base::PlatformFile invalid_file = base::kInvalidPlatformFileValue;
+    callback_->Run(error_code, base::PassPlatformFile(&invalid_file));
   }
 
   virtual void DidWrite(int64 bytes, bool complete) {
@@ -1034,7 +1039,12 @@ class AsyncOpenFileSystemURLCallbackTranslator
   virtual void DidOpenFile(
       base::PlatformFile file,
       base::ProcessHandle unused) {
-    callback_->Run(base::PLATFORM_FILE_OK, file);
+    callback_->Run(base::PLATFORM_FILE_OK, base::PassPlatformFile(&file));
+    // Make sure we won't leak file handle if the requester has died.
+    if (file != base::kInvalidPlatformFileValue) {
+      base::FileUtilProxy::Close(
+          RenderThread::current()->GetFileThreadMessageLoopProxy(), file, NULL);
+    }
   }
 
 private:  // TODO(ericu): Delete this?
diff --git a/ppapi/tests/test_file_system.cc b/ppapi/tests/test_file_system.cc
index 9232385..cb37284 100644
--- a/ppapi/tests/test_file_system.cc
+++ b/ppapi/tests/test_file_system.cc
@@ -67,7 +67,9 @@ std::string TestFileSystem::TestMultipleOpens() {
   int32_t rv_2 = file_system.Open(1024, callback_2);
   if (force_async_ && rv_2 != PP_OK_COMPLETIONPENDING)
     return ReportError("FileSystem::Open2 force_async", rv_2);
-  if (rv_2 == PP_OK_COMPLETIONPENDING || rv_2 == PP_OK)
+  if (rv_2 == PP_OK_COMPLETIONPENDING)
+    rv_2 = callback_2.WaitForResult();
+  if (rv_2 == PP_OK)
     return "FileSystem::Open2 should not allow multiple opens.";
 
   if (rv_1 == PP_OK_COMPLETIONPENDING)
@@ -79,7 +81,9 @@ std::string TestFileSystem::TestMultipleOpens() {
   int32_t rv_3 = file_system.Open(1024, callback_3);
   if (force_async_ && rv_3 != PP_OK_COMPLETIONPENDING)
     return ReportError("FileSystem::Open3 force_async", rv_3);
-  if (rv_3 == PP_OK_COMPLETIONPENDING || rv_3 == PP_OK)
+  if (rv_3 == PP_OK_COMPLETIONPENDING)
+    rv_3 = callback_3.WaitForResult();
+  if (rv_3 == PP_OK)
     return "FileSystem::Open3 should not allow multiple opens.";
 
   PASS();
diff --git a/webkit/fileapi/file_system_callback_dispatcher.cc b/webkit/fileapi/file_system_callback_dispatcher.cc
index 1298979..08fdf55 100644
--- a/webkit/fileapi/file_system_callback_dispatcher.cc
+++ b/webkit/fileapi/file_system_callback_dispatcher.cc
@@ -15,6 +15,9 @@ void FileSystemCallbackDispatcher::DidOpenFile(
     base::PlatformFile file,
     base::ProcessHandle peer_handle) {
   NOTREACHED();
+
+  if (file != base::kInvalidPlatformFileValue)
+    base::ClosePlatformFile(file);
 }
 
 }  // namespace fileapi
diff --git a/webkit/fileapi/file_system_callback_dispatcher.h b/webkit/fileapi/file_system_callback_dispatcher.h
index e2d3caa..cf19d00 100644
--- a/webkit/fileapi/file_system_callback_dispatcher.h
+++ b/webkit/fileapi/file_system_callback_dispatcher.h
@@ -55,6 +55,7 @@ class FileSystemCallbackDispatcher {
 
   // Callback for OpenFile.  This isn't in WebFileSystemCallbacks, as it's just
   // for Pepper.
+  // The method will be responsible for closing |file|.
   virtual void DidOpenFile(
       base::PlatformFile file,
       base::ProcessHandle peer_handle);
diff --git a/webkit/plugins/ppapi/plugin_delegate.h b/webkit/plugins/ppapi/plugin_delegate.h
index 2b6e098..ebfda18 100644
--- a/webkit/plugins/ppapi/plugin_delegate.h
+++ b/webkit/plugins/ppapi/plugin_delegate.h
@@ -294,7 +294,7 @@ class PluginDelegate {
       WebKit::WebFileChooserCompletion* chooser_completion) = 0;
 
   // Sends an async IPC to open a file.
-  typedef Callback2<base::PlatformFileError, base::PlatformFile
+  typedef Callback2<base::PlatformFileError, base::PassPlatformFile
                     >::Type AsyncOpenFileCallback;
   virtual bool AsyncOpenFile(const FilePath& path,
                              int flags,
diff --git a/webkit/plugins/ppapi/ppb_file_io_impl.cc b/webkit/plugins/ppapi/ppb_file_io_impl.cc
index 8a3b3d1..309067c 100644
--- a/webkit/plugins/ppapi/ppb_file_io_impl.cc
+++ b/webkit/plugins/ppapi/ppb_file_io_impl.cc
@@ -304,14 +304,14 @@ void PPB_FileIO_Impl::StatusCallback(base::PlatformFileError error_code) {
 
 void PPB_FileIO_Impl::AsyncOpenFileCallback(
     base::PlatformFileError error_code,
-    base::PlatformFile file) {
+    base::PassPlatformFile file) {
   if (pending_op_ != OPERATION_EXCLUSIVE || callbacks_.empty()) {
     NOTREACHED();
     return;
   }
 
   DCHECK(file_ == base::kInvalidPlatformFileValue);
-  file_ = file;
+  file_ = file.ReleaseValue();
   RunAndRemoveFirstPendingCallback(PlatformFileErrorToPepperError(error_code));
 }
 
diff --git a/webkit/plugins/ppapi/ppb_file_io_impl.h b/webkit/plugins/ppapi/ppb_file_io_impl.h
index 0fc3507..327f713 100644
--- a/webkit/plugins/ppapi/ppb_file_io_impl.h
+++ b/webkit/plugins/ppapi/ppb_file_io_impl.h
@@ -115,7 +115,7 @@ class PPB_FileIO_Impl : public Resource,
 
   void StatusCallback(base::PlatformFileError error_code);
   void AsyncOpenFileCallback(base::PlatformFileError error_code,
-                             base::PlatformFile file);
+                             base::PassPlatformFile file);
   void QueryInfoCallback(base::PlatformFileError error_code,
                          const base::PlatformFileInfo& file_info);
   void ReadCallback(base::PlatformFileError error_code,