Preparing |SyncSocket|'s handle for the peer process is different for POSIX and Windows

which leads to code duplication, platform #ifdef checks on multiple levels and
general confusion on how to work with the SyncSocket.

Typical use case for |SyncSocket|:
1. Browser creates and connects the socket pair - one for browser and one for renderer.
2. Browser prepares the foreign socket (Windows duplicates, POSIX creates file descriptor).
3. Browser relays the foreign socket handle to the renderer via IPC.
4. Renderer receives the IPC and creates the socket using the handle provided.
5. Sockets are ready for send/receive on both ends.

Steps 1-4 get simplified since there is no need to check the platform in order to prepare the socket for transit.

What this CL brings:
1. Adds |SyncSocket::TransitDescriptor| type which wraps the socket handle and is cross-platform.
2. Adds |SyncSocket::PrepareTransitDescriptor| method which is implemented depending on the platform.
3. Adds |SyncSocket::UnwrapHandle| method which unwraps |SyncSocket::Handle| from |SyncSocket::TransitDescriptor|.
4. Removes #ifdefs for platform-checks in code using |SyncSocket| and simplifies preparing the SyncSocket.

Note:
- There is still some less evident duplication in the ppapi and pepper-broker code which should be addressed.
- SyncSocket unit test should also be reviewed.
- There is a similar pattern when using SharedMemory.

BUG=409656

Review URL: https://codereview.chromium.org/525313002

Cr-Commit-Position: refs/heads/master@{#293680}

base/sync_socket.h

@@ -17,17 +17,24 @@
 
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
+#include "base/process/process_handle.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/time/time.h"
 
+#if defined(OS_POSIX)
+#include "base/file_descriptor_posix.h"
+#endif
+
 namespace base {
 
 class BASE_EXPORT SyncSocket {
  public:
 #if defined(OS_WIN)
   typedef HANDLE Handle;
+  typedef Handle TransitDescriptor;
 #else
   typedef int Handle;
+  typedef FileDescriptor TransitDescriptor;
 #endif
   static const Handle kInvalidHandle;
 
@@ -42,6 +49,15 @@ class BASE_EXPORT SyncSocket {
   // return, the sockets will both be valid and connected.
   static bool CreatePair(SyncSocket* socket_a, SyncSocket* socket_b);
 
+  // Returns |Handle| wrapped in a |TransitDescriptor|.
+  static Handle UnwrapHandle(const TransitDescriptor& descriptor);
+
+  // Prepares a |TransitDescriptor| which wraps |Handle| used for transit.
+  // This is used to prepare the underlying shared resource before passing back
+  // the handle to be used by the peer process.
+  bool PrepareTransitDescriptor(ProcessHandle peer_process_handle,
+                                TransitDescriptor* descriptor);
+
   // Closes the SyncSocket.  Returns true on success, false on failure.
   virtual bool Close();
 

base/sync_socket_nacl.cc

@@ -27,6 +27,24 @@ bool SyncSocket::CreatePair(SyncSocket* socket_a, SyncSocket* socket_b) {
   return false;
 }
 
+// static
+SyncSocket::Handle SyncSocket::UnwrapHandle(
+    const SyncSocket::TransitDescriptor& descriptor) {
+  // TODO(xians): Still unclear how NaCl uses SyncSocket.
+  // See http://crbug.com/409656
+  NOTIMPLEMENTED();
+  return SyncSocket::kInvalidHandle;
+}
+
+bool SyncSocket::PrepareTransitDescriptor(
+    ProcessHandle peer_process_handle,
+    SyncSocket::TransitDescriptor* descriptor) {
+  // TODO(xians): Still unclear how NaCl uses SyncSocket.
+  // See http://crbug.com/409656
+  NOTIMPLEMENTED();
+  return false;
+}
+
 bool SyncSocket::Close() {
   if (handle_ != kInvalidHandle) {
     if (close(handle_) < 0)

base/sync_socket_posix.cc

@@ -96,6 +96,19 @@ bool SyncSocket::CreatePair(SyncSocket* socket_a, SyncSocket* socket_b) {
   return true;
 }
 
+// static
+SyncSocket::Handle SyncSocket::UnwrapHandle(
+    const TransitDescriptor& descriptor) {
+  return descriptor.fd;
+}
+
+bool SyncSocket::PrepareTransitDescriptor(ProcessHandle peer_process_handle,
+                                          TransitDescriptor* descriptor) {
+  descriptor->fd = handle();
+  descriptor->auto_close = false;
+  return descriptor->fd != kInvalidHandle;
+}
+
 bool SyncSocket::Close() {
   const bool retval = CloseHandle(handle_);
   handle_ = kInvalidHandle;

base/sync_socket_win.cc

@@ -207,6 +207,23 @@ bool SyncSocket::CreatePair(SyncSocket* socket_a, SyncSocket* socket_b) {
   return CreatePairImpl(&socket_a->handle_, &socket_b->handle_, false);
 }
 
+// static
+SyncSocket::Handle SyncSocket::UnwrapHandle(
+    const TransitDescriptor& descriptor) {
+  return descriptor;
+}
+
+bool SyncSocket::PrepareTransitDescriptor(ProcessHandle peer_process_handle,
+                                          TransitDescriptor* descriptor) {
+  DCHECK(descriptor);
+  if (!::DuplicateHandle(GetCurrentProcess(), handle(), peer_process_handle,
+                         descriptor, 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+    DPLOG(ERROR) << "Cannot duplicate socket handle for peer process.";
+    return false;
+  }
+  return true;
+}
+
 bool SyncSocket::Close() {
   if (handle_ == kInvalidHandle)
     return true;

content/browser/renderer_host/media/audio_input_renderer_host.cc

@@ -31,7 +31,7 @@ void LogMessage(int stream_id, const std::string& msg, bool add_prefix) {
   DVLOG(1) << oss.str();
 }
 
-}
+}  // namespace
 
 namespace content {
 
@@ -178,16 +178,11 @@ void AudioInputRendererHost::DoCompleteCreation(
   AudioInputSyncWriter* writer =
       static_cast<AudioInputSyncWriter*>(entry->writer.get());
 
-#if defined(OS_WIN)
-  base::SyncSocket::Handle foreign_socket_handle;
-#else
-  base::FileDescriptor foreign_socket_handle;
-#endif
+  base::SyncSocket::TransitDescriptor socket_transit_descriptor;
 
   // If we failed to prepare the sync socket for the renderer then we fail
   // the construction of audio input stream.
-  if (!writer->PrepareForeignSocketHandle(PeerHandle(),
-                                          &foreign_socket_handle)) {
+  if (!writer->PrepareForeignSocket(PeerHandle(), &socket_transit_descriptor)) {
     DeleteEntryOnError(entry, SYNC_SOCKET_ERROR);
     return;
   }
@@ -196,8 +191,8 @@ void AudioInputRendererHost::DoCompleteCreation(
              "DoCompleteCreation: IPC channel and stream are now open",
              true);
 
-  Send(new AudioInputMsg_NotifyStreamCreated(entry->stream_id,
-      foreign_memory_handle, foreign_socket_handle,
+  Send(new AudioInputMsg_NotifyStreamCreated(
+      entry->stream_id, foreign_memory_handle, socket_transit_descriptor,
       entry->shared_memory.requested_size(),
       entry->shared_memory_segment_count));
 }

content/browser/renderer_host/media/audio_input_sync_writer.cc

@@ -107,27 +107,11 @@ bool AudioInputSyncWriter::Init() {
                                                 foreign_socket_.get());
 }
 
-#if defined(OS_WIN)
-
-bool AudioInputSyncWriter::PrepareForeignSocketHandle(
-    base::ProcessHandle process_handle,
-    base::SyncSocket::Handle* foreign_handle) {
-  ::DuplicateHandle(GetCurrentProcess(), foreign_socket_->handle(),
-                    process_handle, foreign_handle,
-                    0, FALSE, DUPLICATE_SAME_ACCESS);
-  return (*foreign_handle != 0);
-}
-
-#else
-
-bool AudioInputSyncWriter::PrepareForeignSocketHandle(
+bool AudioInputSyncWriter::PrepareForeignSocket(
     base::ProcessHandle process_handle,
-    base::FileDescriptor* foreign_handle) {
-  foreign_handle->fd = foreign_socket_->handle();
-  foreign_handle->auto_close = false;
-  return (foreign_handle->fd != -1);
+    base::SyncSocket::TransitDescriptor* descriptor) {
+  return foreign_socket_->PrepareTransitDescriptor(process_handle, descriptor);
 }
 
-#endif
 
 }  // namespace content

content/browser/renderer_host/media/audio_input_sync_writer.h

@@ -42,12 +42,8 @@ class AudioInputSyncWriter : public media::AudioInputController::SyncWriter {
   virtual void Close() OVERRIDE;
 
   bool Init();
-  bool PrepareForeignSocketHandle(base::ProcessHandle process_handle,
-#if defined(OS_WIN)
-                                  base::SyncSocket::Handle* foreign_handle);
-#else
-                                  base::FileDescriptor* foreign_handle);
-#endif
+  bool PrepareForeignSocket(base::ProcessHandle process_handle,
+                            base::SyncSocket::TransitDescriptor* descriptor);
 
  private:
   base::SharedMemory* shared_memory_;

content/browser/renderer_host/media/audio_renderer_host.cc

@@ -238,24 +238,17 @@ void AudioRendererHost::DoCompleteCreation(int stream_id) {
 
   AudioSyncReader* reader = static_cast<AudioSyncReader*>(entry->reader());
 
-#if defined(OS_WIN)
-  base::SyncSocket::Handle foreign_socket_handle;
-#else
-  base::FileDescriptor foreign_socket_handle;
-#endif
+  base::SyncSocket::TransitDescriptor socket_descriptor;
 
   // If we failed to prepare the sync socket for the renderer then we fail
   // the construction of audio stream.
-  if (!reader->PrepareForeignSocketHandle(PeerHandle(),
-                                          &foreign_socket_handle)) {
+  if (!reader->PrepareForeignSocket(PeerHandle(), &socket_descriptor)) {
     ReportErrorAndClose(entry->stream_id());
     return;
   }
 
   Send(new AudioMsg_NotifyStreamCreated(
-      entry->stream_id(),
-      foreign_memory_handle,
-      foreign_socket_handle,
+      entry->stream_id(), foreign_memory_handle, socket_descriptor,
       entry->shared_memory()->requested_size()));
 }
 

content/browser/renderer_host/media/audio_renderer_host_unittest.cc

@@ -101,14 +101,9 @@ class MockAudioRendererHost : public AudioRendererHost {
     return true;
   }
 
-  void OnNotifyStreamCreated(int stream_id,
-                             base::SharedMemoryHandle handle,
-#if defined(OS_WIN)
-                             base::SyncSocket::Handle socket_handle,
-#else
-                             base::FileDescriptor socket_descriptor,
-#endif
-                             uint32 length) {
+  void OnNotifyStreamCreated(
+      int stream_id, base::SharedMemoryHandle handle,
+      base::SyncSocket::TransitDescriptor socket_descriptor, uint32 length) {
     // Maps the shared memory.
     shared_memory_.reset(new base::SharedMemory(handle, false));
     CHECK(shared_memory_->Map(length));
@@ -116,12 +111,8 @@ class MockAudioRendererHost : public AudioRendererHost {
     shared_memory_length_ = length;
 
     // Create the SyncSocket using the handle.
-    base::SyncSocket::Handle sync_socket_handle;
-#if defined(OS_WIN)
-    sync_socket_handle = socket_handle;
-#else
-    sync_socket_handle = socket_descriptor.fd;
-#endif
+    base::SyncSocket::Handle sync_socket_handle =
+        base::SyncSocket::UnwrapHandle(socket_descriptor);
     sync_socket_.reset(new base::SyncSocket(sync_socket_handle));
 
     // And then delegate the call to the mock method.

content/browser/renderer_host/media/audio_sync_reader.cc

@@ -115,24 +115,11 @@ bool AudioSyncReader::Init() {
                                                 foreign_socket_.get());
 }
 
-#if defined(OS_WIN)
-bool AudioSyncReader::PrepareForeignSocketHandle(
+bool AudioSyncReader::PrepareForeignSocket(
     base::ProcessHandle process_handle,
-    base::SyncSocket::Handle* foreign_handle) {
-  ::DuplicateHandle(GetCurrentProcess(), foreign_socket_->handle(),
-                    process_handle, foreign_handle,
-                    0, FALSE, DUPLICATE_SAME_ACCESS);
-  return (*foreign_handle != 0);
+    base::SyncSocket::TransitDescriptor* descriptor) {
+  return foreign_socket_->PrepareTransitDescriptor(process_handle, descriptor);
 }
-#else
-bool AudioSyncReader::PrepareForeignSocketHandle(
-    base::ProcessHandle process_handle,
-    base::FileDescriptor* foreign_handle) {
-  foreign_handle->fd = foreign_socket_->handle();
-  foreign_handle->auto_close = false;
-  return (foreign_handle->fd != -1);
-}
-#endif
 
 bool AudioSyncReader::WaitUntilDataIsReady() {
   base::TimeDelta timeout = maximum_wait_time_;

content/browser/renderer_host/media/audio_sync_reader.h

@@ -39,12 +39,8 @@ class AudioSyncReader : public media::AudioOutputController::SyncReader {
   virtual void Close() OVERRIDE;
 
   bool Init();
-  bool PrepareForeignSocketHandle(base::ProcessHandle process_handle,
-#if defined(OS_WIN)
-                                  base::SyncSocket::Handle* foreign_handle);
-#else
-                                  base::FileDescriptor* foreign_handle);
-#endif
+  bool PrepareForeignSocket(base::ProcessHandle process_handle,
+                            base::SyncSocket::TransitDescriptor* descriptor);
 
  private:
   // Blocks until data is ready for reading or a timeout expires.  Returns false

content/common/media/audio_messages.h

@@ -39,38 +39,23 @@ IPC_STRUCT_END()
 // buffer it shares with the browser process. It is also given a SyncSocket that
 // it uses to communicate with the browser process about the state of the
 // buffered audio data.
-#if defined(OS_WIN)
-IPC_MESSAGE_CONTROL4(AudioMsg_NotifyStreamCreated,
+IPC_MESSAGE_CONTROL4(
+   AudioMsg_NotifyStreamCreated,
    int /* stream id */,
    base::SharedMemoryHandle /* handle */,
-                     base::SyncSocket::Handle /* socket handle */,
+   base::SyncSocket::TransitDescriptor /* socket descriptor */,
    uint32 /* length */)
-#else
-IPC_MESSAGE_CONTROL4(AudioMsg_NotifyStreamCreated,
-                     int /* stream id */,
-                     base::SharedMemoryHandle /* handle */,
-                     base::FileDescriptor /* socket handle */,
-                     uint32 /* length */)
-#endif
 
 // Tell the renderer process that an audio input stream has been created.
 // The renderer process would be given a SyncSocket that it should read
 // from from then on. It is also given number of segments in shared memory.
-#if defined(OS_WIN)
-IPC_MESSAGE_CONTROL5(AudioInputMsg_NotifyStreamCreated,
-                     int /* stream id */,
-                     base::SharedMemoryHandle /* handle */,
-                     base::SyncSocket::Handle /* socket handle */,
-                     uint32 /* length */,
-                     uint32 /* segment count */)
-#else
-IPC_MESSAGE_CONTROL5(AudioInputMsg_NotifyStreamCreated,
+IPC_MESSAGE_CONTROL5(
+   AudioInputMsg_NotifyStreamCreated,
    int /* stream id */,
    base::SharedMemoryHandle /* handle */,
-                     base::FileDescriptor /* socket handle */,
+   base::SyncSocket::TransitDescriptor /* socket descriptor */,
    uint32 /* length */,
    uint32 /* segment count */)
-#endif
 
 // Notification message sent from AudioRendererHost to renderer after an output
 // device change has occurred.

content/renderer/media/audio_input_message_filter.cc

@@ -23,7 +23,7 @@ void LogMessage(int stream_id, const std::string& msg) {
   DVLOG(1) << oss.str();
 }
 
-}
+}  // namespace
 
 namespace content {
 
@@ -126,19 +126,14 @@ void AudioInputMessageFilter::OnChannelClosing() {
 void AudioInputMessageFilter::OnStreamCreated(
     int stream_id,
     base::SharedMemoryHandle handle,
-#if defined(OS_WIN)
-    base::SyncSocket::Handle socket_handle,
-#else
-    base::FileDescriptor socket_descriptor,
-#endif
+    base::SyncSocket::TransitDescriptor socket_descriptor,
     uint32 length,
     uint32 total_segments) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   LogMessage(stream_id, "OnStreamCreated");
 
-#if !defined(OS_WIN)
-  base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
-#endif
+  base::SyncSocket::Handle socket_handle =
+      base::SyncSocket::UnwrapHandle(socket_descriptor);
   media::AudioInputIPCDelegate* delegate = delegates_.Lookup(stream_id);
   if (!delegate) {
     DLOG(WARNING) << "Got audio stream event for a non-existent or removed"

content/renderer/media/audio_message_filter.cc

@@ -168,11 +168,7 @@ void AudioMessageFilter::OnChannelClosing() {
 void AudioMessageFilter::OnStreamCreated(
     int stream_id,
     base::SharedMemoryHandle handle,
-#if defined(OS_WIN)
-    base::SyncSocket::Handle socket_handle,
-#else
-    base::FileDescriptor socket_descriptor,
-#endif
+    base::SyncSocket::TransitDescriptor socket_descriptor,
     uint32 length) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
 
@@ -180,9 +176,8 @@ void AudioMessageFilter::OnStreamCreated(
       "AMF::OnStreamCreated. stream_id=%d",
       stream_id));
 
-#if !defined(OS_WIN)
-  base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
-#endif
+  base::SyncSocket::Handle socket_handle =
+      base::SyncSocket::UnwrapHandle(socket_descriptor);
 
   media::AudioOutputIPCDelegate* delegate = delegates_.Lookup(stream_id);
   if (!delegate) {

content/renderer/media/audio_message_filter.h

@@ -76,11 +76,7 @@ class CONTENT_EXPORT AudioMessageFilter : public IPC::MessageFilter {
 
   // Received when browser process has created an audio output stream.
   void OnStreamCreated(int stream_id, base::SharedMemoryHandle handle,
-#if defined(OS_WIN)
-                       base::SyncSocket::Handle socket_handle,
-#else
-                       base::FileDescriptor socket_descriptor,
-#endif
+                       base::SyncSocket::TransitDescriptor socket_descriptor,
                        uint32 length);
 
   // Received when internal state of browser process' audio output device has

content/renderer/media/audio_message_filter_unittest.cc

@@ -86,17 +86,11 @@ TEST(AudioMessageFilterTest, Basic) {
   EXPECT_EQ(&delegate, filter->delegates_.Lookup(kStreamId));
 
   // AudioMsg_NotifyStreamCreated
-#if defined(OS_WIN)
-  base::SyncSocket::Handle socket_handle;
-#else
-  base::FileDescriptor socket_handle;
-#endif
+  base::SyncSocket::TransitDescriptor socket_descriptor;
   const uint32 kLength = 1024;
   EXPECT_FALSE(delegate.created_received());
-  filter->OnMessageReceived(
-      AudioMsg_NotifyStreamCreated(
-          kStreamId, base::SharedMemory::NULLHandle(),
-          socket_handle, kLength));
+  filter->OnMessageReceived(AudioMsg_NotifyStreamCreated(
+      kStreamId, base::SharedMemory::NULLHandle(), socket_descriptor, kLength));
   EXPECT_TRUE(delegate.created_received());
   EXPECT_FALSE(base::SharedMemory::IsHandleValid(delegate.handle()));
   EXPECT_EQ(kLength, delegate.length());

media/audio/audio_output_device_unittest.cc

@@ -54,25 +54,6 @@ class MockAudioOutputIPC : public AudioOutputIPC {
   MOCK_METHOD1(SetVolume, void(double volume));
 };
 
-// Creates a copy of a SyncSocket handle that we can give to AudioOutputDevice.
-// On Windows this means duplicating the pipe handle so that AudioOutputDevice
-// can call CloseHandle() (since ownership has been transferred), but on other
-// platforms, we just copy the same socket handle since AudioOutputDevice on
-// those platforms won't actually own the socket (FileDescriptor.auto_close is
-// false).
-bool DuplicateSocketHandle(SyncSocket::Handle socket_handle,
-                           SyncSocket::Handle* copy) {
-#if defined(OS_WIN)
-  HANDLE process = GetCurrentProcess();
-  ::DuplicateHandle(process, socket_handle, process, copy,
-                    0, FALSE, DUPLICATE_SAME_ACCESS);
-  return *copy != NULL;
-#else
-  *copy = socket_handle;
-  return *copy != -1;
-#endif
-}
-
 ACTION_P2(SendPendingBytes, socket, pending_bytes) {
   socket->Send(&pending_bytes, sizeof(pending_bytes));
 }
@@ -163,15 +144,16 @@ void AudioOutputDeviceTest::CreateStream() {
   // Create duplicates of the handles we pass to AudioOutputDevice since
   // ownership will be transferred and AudioOutputDevice is responsible for
   // freeing.
-  SyncSocket::Handle audio_device_socket = SyncSocket::kInvalidHandle;
-  ASSERT_TRUE(DuplicateSocketHandle(renderer_socket_.handle(),
-                                    &audio_device_socket));
+  SyncSocket::TransitDescriptor audio_device_socket_descriptor;
+  ASSERT_TRUE(renderer_socket_.PrepareTransitDescriptor(
+      base::GetCurrentProcessHandle(), &audio_device_socket_descriptor));
   base::SharedMemoryHandle duplicated_memory_handle;
   ASSERT_TRUE(shared_memory_.ShareToProcess(base::GetCurrentProcessHandle(),
                                             &duplicated_memory_handle));
 
-  audio_device_->OnStreamCreated(duplicated_memory_handle, audio_device_socket,
-                                 kMemorySize);
+  audio_device_->OnStreamCreated(
+      duplicated_memory_handle,
+      SyncSocket::UnwrapHandle(audio_device_socket_descriptor), kMemorySize);
   io_loop_.RunUntilIdle();
 }