Factor fd sharing code in proxy and fix fd issues once and for all.

BUG=none
TEST=use flapper, go to youtube, make plugin crash and check no warning about close() failing.

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

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

5 files changed, 83 insertions, 72 deletions

diff --git a/ppapi/proxy/dispatcher.cc b/ppapi/proxy/dispatcher.cc
index 53b0baf..ad0094d 100644
--- a/ppapi/proxy/dispatcher.cc
+++ b/ppapi/proxy/dispatcher.cc
@@ -252,6 +252,33 @@ const void* Dispatcher::GetLocalInterface(const char* interface) {
   return local_get_interface_(interface);
 }
 
+IPC::PlatformFileForTransit Dispatcher::ShareHandleWithRemote(
+      base::PlatformFile handle,
+      bool should_close_source) {
+  IPC::PlatformFileForTransit out_handle;
+#if defined(OS_WIN)
+  DWORD options = DUPLICATE_SAME_ACCESS;
+  if (should_close_source)
+    options |= DUPLICATE_CLOSE_SOURCE;
+  if (!::DuplicateHandle(::GetCurrentProcess(),
+                         handle,
+                         remote_process_handle_,
+                         &out_handle,
+                         0,
+                         FALSE,
+                         options))
+    out_handle = IPC::InvalidPlatformFileForTransit();
+#elif defined(OS_POSIX)
+  // If asked to close the source, we can simply re-use the source fd instead of
+  // dup()ing and close()ing.
+  int fd = should_close_source ? handle : ::dup(handle);
+  out_handle = base::FileDescriptor(fd, true);
+#else
+  #error Not implemented.
+#endif
+  return out_handle;
+}
+
 bool Dispatcher::Send(IPC::Message* msg) {
   if (test_sink_)
     return test_sink_->Send(msg);
diff --git a/ppapi/proxy/dispatcher.h b/ppapi/proxy/dispatcher.h
index 82c0f31..161aa54 100644
--- a/ppapi/proxy/dispatcher.h
+++ b/ppapi/proxy/dispatcher.h
@@ -15,6 +15,7 @@
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_channel_handle.h"
 #include "ipc/ipc_message.h"
+#include "ipc/ipc_platform_file.h"
 #include "ppapi/c/pp_module.h"
 #include "ppapi/proxy/callback_tracker.h"
 #include "ppapi/proxy/interface_id.h"
@@ -83,13 +84,15 @@ class Dispatcher : public IPC::Channel::Listener,
   // Wrapper for calling the local GetInterface function.
   const void* GetLocalInterface(const char* interface);
 
-  // Returns the remote process' handle. For the host dispatcher, this will be
-  // the plugin process, and for the plugin dispatcher, this will be the
-  // renderer process. This is used for sharing memory and such and is
-  // guaranteed valid (unless the remote process has suddenly died).
-  base::ProcessHandle remote_process_handle() const {
-    return remote_process_handle_;
-  }
+  // Shares a file handle (HANDLE / file descriptor) with the remote side. It
+  // returns a handle that should be sent in exactly one IPC message. Upon
+  // receipt, the remote side then owns that handle. Note: if sending the
+  // message fails, the returned handle is properly closed by the IPC system. If
+  // should_close_source is set to true, the original handle is closed by this
+  // operation and should not be used again.
+  IPC::PlatformFileForTransit ShareHandleWithRemote(
+      base::PlatformFile handle,
+      bool should_close_source);
 
   // Called if the remote side is declaring to us which interfaces it supports
   // so we don't have to query for each one. We'll pre-create proxies for
diff --git a/ppapi/proxy/ppb_audio_proxy.cc b/ppapi/proxy/ppb_audio_proxy.cc
index 3e7350b..65f4b34 100644
--- a/ppapi/proxy/ppb_audio_proxy.cc
+++ b/ppapi/proxy/ppb_audio_proxy.cc
@@ -129,6 +129,18 @@ InterfaceProxy* CreateAudioProxy(Dispatcher* dispatcher,
   return new PPB_Audio_Proxy(dispatcher, target_interface);
 }
 
+base::PlatformFile IntToPlatformFile(int32_t handle) {
+  // TODO(piman/brettw): Change trusted interface to return a PP_FileHandle,
+  // those casts are ugly.
+#if defined(OS_WIN)
+  return reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
+#elif defined(OS_POSIX)
+  return handle;
+#else
+  #error Not implemented.
+#endif
+}
+
 }  // namespace
 
 PPB_Audio_Proxy::PPB_Audio_Proxy(Dispatcher* dispatcher,
@@ -226,13 +238,7 @@ void PPB_Audio_Proxy::AudioChannelConnected(
     const HostResource& resource) {
   IPC::PlatformFileForTransit socket_handle =
       IPC::InvalidPlatformFileForTransit();
-#if defined(OS_WIN)
-  base::SharedMemoryHandle shared_memory = NULL;
-#elif defined(OS_POSIX)
-  base::SharedMemoryHandle shared_memory(-1, false);
-#else
-  #error Not implemented.
-#endif
+  base::SharedMemoryHandle shared_memory = IPC::InvalidPlatformFileForTransit();
   uint32_t shared_memory_length = 0;
 
   int32_t result_code = result;
@@ -271,23 +277,13 @@ int32_t PPB_Audio_Proxy::GetAudioConnectedHandles(
   if (result != PP_OK)
     return result;
 
-#if defined(OS_WIN)
-  // On Windows, duplicate the socket into the plugin process, this will
-  // automatically close the source handle.
-  ::DuplicateHandle(
-      GetCurrentProcess(),
-      reinterpret_cast<HANDLE>(static_cast<intptr_t>(socket_handle)),
-      dispatcher()->remote_process_handle(), foreign_socket_handle,
-      STANDARD_RIGHTS_REQUIRED | FILE_MAP_READ | FILE_MAP_WRITE,
-      FALSE, DUPLICATE_CLOSE_SOURCE);
-#else
-  // On Posix, the socket handle will be auto-duplicated when we send the
-  // FileDescriptor. Don't set AutoClose since this is not our handle.
-  *foreign_socket_handle = base::FileDescriptor(socket_handle, false);
-#endif
+  // socket_handle doesn't belong to us: don't close it.
+  *foreign_socket_handle = dispatcher()->ShareHandleWithRemote(
+      IntToPlatformFile(socket_handle), false);
+  if (*foreign_socket_handle == IPC::InvalidPlatformFileForTransit())
+    return PP_ERROR_FAILED;
 
   // Get the shared memory for the buffer.
-  // TODO(brettw) remove the reinterpret cast when the interface is updated.
   int shared_memory_handle;
   result = audio_trusted->GetSharedMemory(resource.host_resource(),
                                           &shared_memory_handle,
@@ -295,18 +291,10 @@ int32_t PPB_Audio_Proxy::GetAudioConnectedHandles(
   if (result != PP_OK)
     return result;
 
-  base::SharedMemory shared_memory(
-#if defined(OS_WIN)
-      reinterpret_cast<HANDLE>(static_cast<intptr_t>(shared_memory_handle)),
-#else
-      base::FileDescriptor(shared_memory_handle, false),
-#endif
-      false);
-
-  // Duplicate the shared memory to the plugin process. This will automatically
-  // close the source handle.
-  if (!shared_memory.GiveToProcess(dispatcher()->remote_process_handle(),
-                                   foreign_shared_memory_handle))
+  // shared_memory_handle doesn't belong to us: don't close it.
+  *foreign_shared_memory_handle = dispatcher()->ShareHandleWithRemote(
+      IntToPlatformFile(shared_memory_handle), false);
+  if (*foreign_shared_memory_handle == IPC::InvalidPlatformFileForTransit())
     return PP_ERROR_FAILED;
 
   return PP_OK;
diff --git a/ppapi/proxy/ppb_context_3d_proxy.cc b/ppapi/proxy/ppb_context_3d_proxy.cc
index d36ed2a..55df27b 100644
--- a/ppapi/proxy/ppb_context_3d_proxy.cc
+++ b/ppapi/proxy/ppb_context_3d_proxy.cc
@@ -131,17 +131,20 @@ const PPB_Context3D_Dev context_3d_interface = {
   &GetBoundSurfaces,
 };
 
-base::SharedMemoryHandle SHMHandleFromInt(int shm_handle) {
-#if defined(OS_POSIX)
-  // The handle isn't ours to close, but we want to keep a reference to the
-  // handle until it is actually sent, so duplicate it, and mark auto-close.
-  return base::FileDescriptor(dup(shm_handle), true);
-#elif defined(OS_WIN)
-  // TODO(piman): DuplicateHandle to the plugin process.
-  return reinterpret_cast<HANDLE>(shm_handle);
+base::SharedMemoryHandle TransportSHMHandleFromInt(Dispatcher* dispatcher,
+                                                   int shm_handle) {
+  // TODO(piman): Change trusted interface to return a PP_FileHandle, those
+  // casts are ugly.
+  base::PlatformFile source =
+#if defined(OS_WIN)
+      reinterpret_cast<HANDLE>(static_cast<intptr_t>(shm_handle));
+#elif defined(OS_POSIX)
+      shm_handle;
 #else
-#error "Platform not supported."
+  #error Not implemented.
 #endif
+  // Don't close the handle, it doesn't belong to us.
+  return dispatcher->ShareHandleWithRemote(source, false);
 }
 
 gpu::CommandBuffer::State GPUStateFromPPState(
@@ -547,7 +550,7 @@ void PPB_Context3D_Proxy::OnMsgInitialize(
     return;
   }
 
-  *ring_buffer = SHMHandleFromInt(shm_handle);
+  *ring_buffer = TransportSHMHandleFromInt(dispatcher(), shm_handle);
 }
 
 void PPB_Context3D_Proxy::OnMsgGetState(const HostResource& context,
@@ -598,7 +601,7 @@ void PPB_Context3D_Proxy::OnMsgGetTransferBuffer(
                                                    &shm_size)) {
     return;
   }
-  *transfer_buffer = SHMHandleFromInt(shm_handle);
+  *transfer_buffer = TransportSHMHandleFromInt(dispatcher(), shm_handle);
   *size = shm_size;
 }
 
diff --git a/ppapi/proxy/ppb_flash_file_proxy.cc b/ppapi/proxy/ppb_flash_file_proxy.cc
index 5b83fa5..0c0869a 100644
--- a/ppapi/proxy/ppb_flash_file_proxy.cc
+++ b/ppapi/proxy/ppb_flash_file_proxy.cc
@@ -18,31 +18,20 @@ namespace proxy {
 
 namespace {
 
-// Given an error code and a handle result from a Pepper API call, converts
-// to a PlatformFileForTransit, possibly also updating the error value if
-// an error occurred.
+// Given an error code and a handle result from a Pepper API call, converts to a
+// PlatformFileForTransit by sharing with the other side, closing the original
+// handle, possibly also updating the error value if an error occurred.
 IPC::PlatformFileForTransit PlatformFileToPlatformFileForTransit(
+    Dispatcher* dispatcher,
     int32_t* error,
     base::PlatformFile file) {
   if (*error != PP_OK)
     return IPC::InvalidPlatformFileForTransit();
-#if defined(OS_WIN)
-/* TODO(brettw): figure out how to get the target process handle.
-  HANDLE result;
-  if (!::DuplicateHandle(::GetCurrentProcess(), file,
-                         target_process, &result, 0, false,
-                         DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
+  IPC::PlatformFileForTransit out_handle =
+      dispatcher->ShareHandleWithRemote(file, true);
+  if (out_handle == IPC::InvalidPlatformFileForTransit())
     *error = PP_ERROR_NOACCESS;
-    return INVALID_HANDLE_VALUE;
-  }
-  return result;
-*/
-  NOTIMPLEMENTED();
-  *error = PP_ERROR_NOACCESS;
-  return INVALID_HANDLE_VALUE;
-#elif defined(OS_POSIX)
-  return base::FileDescriptor(file, true);
-#endif
+  return out_handle;
 }
 
 void FreeDirContents(PP_Instance /* instance */,
@@ -224,7 +213,8 @@ void PPB_Flash_File_ModuleLocal_Proxy::OnMsgOpenFile(
   base::PlatformFile file;
   *result = ppb_flash_file_module_local_target()->
       OpenFile(instance, path.c_str(), mode, &file);
-  *file_handle = PlatformFileToPlatformFileForTransit(result, file);
+  *file_handle = PlatformFileToPlatformFileForTransit(
+      dispatcher(), result, file);
 }
 
 void PPB_Flash_File_ModuleLocal_Proxy::OnMsgRenameFile(