Bug 1738734 - Directly pass around handles rather than using Transpor…

…tDescriptor, r=jld,media-playback-reviewers,alwu This simplifies the logic around descriptors significantly, which is especially useful considering how few places use the type. There is a small change required on Windows to create the NamedPipe directly and transfer around each end's handle, rather than connecting between processes after the fact. A named pipe has to be used, rather than an anonymous pipe, as bidirectional communication is required. Differential Revision: https://phabricator.services.mozilla.com/D130381
iaxat · Jan 31, 2022 · 4881e36 · 4881e36
1 parent 7357910
commit 4881e36
Show file tree

Hide file tree

Showing 39 changed files with 146 additions and 597 deletions.
diff --git a/dom/canvas/WebGLChild.h b/dom/canvas/WebGLChild.h
@@ -7,6 +7,7 @@
 #define WEBGLCHILD_H_
 
 #include "mozilla/dom/PWebGLChild.h"
+#include "mozilla/WeakPtr.h"
 
 #include <string>
 

diff --git a/dom/canvas/WebGLIpdl.h b/dom/canvas/WebGLIpdl.h
@@ -8,6 +8,7 @@
 
 #include "gfxTypes.h"
 #include "ipc/EnumSerializer.h"
+#include "ipc/IPCMessageUtils.h"
 #include "mozilla/GfxMessageUtils.h"
 #include "mozilla/ipc/IPDLParamTraits.h"
 #include "mozilla/ipc/Shmem.h"

diff --git a/dom/gamepad/windows/WindowsGamepad.cpp b/dom/gamepad/windows/WindowsGamepad.cpp
@@ -18,6 +18,7 @@
 #include "nsITimer.h"
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
+#include "nsWindowsHelpers.h"
 
 #include "mozilla/ArrayUtils.h"
 

diff --git a/dom/locks/LockManagerParent.h b/dom/locks/LockManagerParent.h
@@ -11,6 +11,7 @@
 #include "mozilla/dom/locks/PLockManagerParent.h"
 #include "mozilla/dom/locks/LockRequestParent.h"
 #include "mozilla/ipc/PBackgroundSharedTypes.h"
+#include "mozilla/WeakPtr.h"
 
 namespace mozilla::dom::locks {
 

diff --git a/dom/media/gmp/GMPServiceChild.h b/dom/media/gmp/GMPServiceChild.h
@@ -10,7 +10,6 @@
 #include "MediaResult.h"
 #include "base/process.h"
 #include "mozilla/dom/PContent.h"
-#include "mozilla/ipc/Transport.h"
 #include "mozilla/gmp/PGMPServiceChild.h"
 #include "mozilla/MozPromise.h"
 #include "nsIAsyncShutdown.h"

diff --git a/dom/media/gmp/GMPServiceParent.cpp b/dom/media/gmp/GMPServiceParent.cpp
@@ -47,8 +47,6 @@
 #  include "mozilla/dom/MediaKeys.h"  // MediaKeys::kMediaKeysRequestTopic
 #endif
 
-using mozilla::ipc::Transport;
-
 namespace mozilla::gmp {
 
 #ifdef __CLASS__

diff --git a/gfx/ipc/GPUProcessManager.h b/gfx/ipc/GPUProcessManager.h
@@ -16,7 +16,6 @@
 #include "mozilla/gfx/Point.h"
 #include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/ipc/TaskFactory.h"
-#include "mozilla/ipc/Transport.h"
 #include "mozilla/layers/LayersTypes.h"
 #include "mozilla/webrender/WebRenderTypes.h"
 #include "nsIObserver.h"

diff --git a/gfx/layers/ipc/CompositorBridgeParent.cpp b/gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -26,12 +26,10 @@
 #include "mozilla/StaticPrefs_layers.h"
 #include "mozilla/StaticPrefs_layout.h"
 #include "mozilla/dom/BrowserParent.h"
-#include "mozilla/gfx/2D.h"         // for DrawTarget
-#include "mozilla/gfx/Point.h"      // for IntSize
-#include "mozilla/gfx/Rect.h"       // for IntSize
-#include "mozilla/gfx/gfxVars.h"    // for gfxVars
-#include "mozilla/ipc/Transport.h"  // for Transport
-#include "mozilla/gfx/gfxVars.h"
+#include "mozilla/gfx/2D.h"       // for DrawTarget
+#include "mozilla/gfx/Point.h"    // for IntSize
+#include "mozilla/gfx/Rect.h"     // for IntSize
+#include "mozilla/gfx/gfxVars.h"  // for gfxVars
 #include "mozilla/gfx/GPUParent.h"
 #include "mozilla/layers/APZCTreeManagerParent.h"  // for APZCTreeManagerParent
 #include "mozilla/layers/APZSampler.h"             // for APZSampler

diff --git a/gfx/layers/ipc/ContentCompositorBridgeParent.cpp b/gfx/layers/ipc/ContentCompositorBridgeParent.cpp
@@ -14,7 +14,6 @@
 #  include "mozilla/gfx/DeviceManagerDx.h"  // for DeviceManagerDx
 #  include "mozilla/layers/ImageDataSerializer.h"
 #endif
-#include "mozilla/ipc/Transport.h"           // for Transport
 #include "mozilla/layers/AnimationHelper.h"  // for CompositorAnimationStorage
 #include "mozilla/layers/APZCTreeManagerParent.h"  // for APZCTreeManagerParent
 #include "mozilla/layers/APZUpdater.h"             // for APZUpdater

diff --git a/gfx/layers/ipc/ImageBridgeChild.cpp b/gfx/layers/ipc/ImageBridgeChild.cpp
@@ -22,7 +22,6 @@
 #include "mozilla/gfx/gfxVars.h"
 #include "mozilla/ipc/Endpoint.h"
 #include "mozilla/ipc/MessageChannel.h"         // for MessageChannel, etc
-#include "mozilla/ipc/Transport.h"              // for Transport
 #include "mozilla/layers/CompositableClient.h"  // for CompositableChild, etc
 #include "mozilla/layers/CompositorThread.h"
 #include "mozilla/layers/ISurfaceAllocator.h"  // for ISurfaceAllocator

diff --git a/gfx/layers/ipc/ImageBridgeParent.cpp b/gfx/layers/ipc/ImageBridgeParent.cpp
@@ -15,7 +15,6 @@
 #include "mozilla/HalTypes.h"   // for hal::THREAD_PRIORITY_COMPOSITOR
 #include "mozilla/ipc/Endpoint.h"
 #include "mozilla/ipc/MessageChannel.h"  // for MessageChannel, etc
-#include "mozilla/ipc/Transport.h"       // for Transport
 #include "mozilla/media/MediaSystemResourceManagerParent.h"  // for MediaSystemResourceManagerParent
 #include "mozilla/layers/BufferTexture.h"
 #include "mozilla/layers/CompositableTransactionParent.h"

diff --git a/gfx/vr/ipc/VRParent.cpp b/gfx/vr/ipc/VRParent.cpp
@@ -9,6 +9,7 @@
 #include "gfxConfig.h"
 #include "nsDebugImpl.h"
 #include "nsThreadManager.h"
+#include "nsPrintfCString.h"
 
 #include "mozilla/dom/MemoryReportRequest.h"
 #include "mozilla/gfx/gfxVars.h"

diff --git a/ipc/chromium/src/chrome/common/child_process_host.cc b/ipc/chromium/src/chrome/common/child_process_host.cc
@@ -13,7 +13,6 @@
 #include "base/waitable_event.h"
 #include "mozilla/ipc/ProcessChild.h"
 #include "mozilla/ipc/BrowserProcessSubThread.h"
-#include "mozilla/ipc/Transport.h"
 typedef mozilla::ipc::BrowserProcessSubThread ChromeThread;
 #include "chrome/common/process_watcher.h"
 

diff --git a/ipc/chromium/src/chrome/common/ipc_channel.h b/ipc/chromium/src/chrome/common/ipc_channel.h
@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "build/build_config.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/UniquePtrExtensions.h"
 #include "mozilla/WeakPtr.h"
 #include "chrome/common/ipc_message.h"
 
@@ -40,6 +41,10 @@ class Channel {
   struct ChannelId {};
 #endif
 
+  // For channels which are created after initialization, handles to the pipe
+  // endpoints may be passed around directly using IPC messages.
+  using ChannelHandle = mozilla::UniqueFileHandle;
+
   // Implemented by consumers of a Channel to receive messages.
   //
   // All listeners will only be called on the IO thread, and must be destroyed
@@ -97,17 +102,8 @@ class Channel {
   //
   Channel(const ChannelId& channel_id, Mode mode, Listener* listener);
 
-  // XXX it would nice not to have yet more platform-specific code in
-  // here but it's just not worth the trouble.
-#if defined(OS_POSIX)
-  // Connect to a pre-created channel |fd| as |mode|.
-  Channel(int fd, Mode mode, Listener* listener);
-#elif defined(OS_WIN)
-  // Connect to a pre-created channel as |mode|.  Clients connect to
-  // the pre-existing server pipe, and servers take over |server_pipe|.
-  Channel(const ChannelId& channel_id, void* server_pipe, Mode mode,
-          Listener* listener);
-#endif
+  // Initialize a pre-created channel |pipe| as |mode|.
+  Channel(ChannelHandle pipe, Mode mode, Listener* listener);
 
   ~Channel();
 
@@ -157,9 +153,6 @@ class Channel {
   // Return the file descriptor for communication with the peer.
   int GetFileDescriptor() const;
 
-  // Reset the file descriptor for communication with the peer.
-  void ResetFileDescriptor(int fd);
-
   // Close the client side of the socketpair.
   void CloseClientFileDescriptor();
 
@@ -174,9 +167,6 @@ class Channel {
 #  endif
 
 #elif defined(OS_WIN)
-  // Return the server pipe handle.
-  void* GetServerPipeHandle() const;
-
   // Tell this pipe to accept handles. Exactly one side of the IPC connection
   // must be set as `MODE_SERVER`, and that side will be responsible for calling
   // `DuplicateHandle` to transfer the handle between processes.
@@ -200,6 +190,10 @@ class Channel {
   static void SetClientChannelFd(int fd);
 #endif  // defined(MOZ_WIDGET_ANDROID)
 
+  // Create a new pair of pipe endpoints which can be used to establish a
+  // native IPC::Channel connection.
+  static bool CreateRawPipe(ChannelHandle* server, ChannelHandle* client);
+
  private:
   // PIMPL to which all channel calls are delegated.
   class ChannelImpl;

diff --git a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc b/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -97,16 +97,6 @@ static int gClientChannelFd =
 //------------------------------------------------------------------------------
 const size_t kMaxPipeNameLength = sizeof(((sockaddr_un*)0)->sun_path);
 
-bool SetCloseOnExec(int fd) {
-  int flags = fcntl(fd, F_GETFD);
-  if (flags == -1) return false;
-
-  flags |= FD_CLOEXEC;
-  if (fcntl(fd, F_SETFD, flags) == -1) return false;
-
-  return true;
-}
-
 bool ErrorIsBrokenPipe(int err) { return err == EPIPE || err == ECONNRESET; }
 
 // Some Android ARM64 devices appear to have a bug where sendmsg
@@ -168,10 +158,11 @@ Channel::ChannelImpl::ChannelImpl(const ChannelId& channel_id, Mode mode,
   EnqueueHelloMessage();
 }
 
-Channel::ChannelImpl::ChannelImpl(int fd, Mode mode, Listener* listener)
+Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
+                                  Listener* listener)
     : factory_(this) {
   Init(mode, listener);
-  SetPipe(fd);
+  SetPipe(pipe.release());
 
   EnqueueHelloMessage();
 }
@@ -230,33 +221,13 @@ bool Channel::ChannelImpl::CreatePipe(Mode mode) {
   DCHECK(server_listen_pipe_ == -1 && pipe_ == -1);
 
   if (mode == MODE_SERVER) {
-    // socketpair()
-    int pipe_fds[2];
-    if (socketpair(AF_UNIX, SOCK_STREAM, 0, pipe_fds) != 0) {
-      mozilla::ipc::AnnotateCrashReportWithErrno(
-          CrashReporter::Annotation::IpcCreatePipeSocketPairErrno, errno);
-      return false;
-    }
-    // Set both ends to be non-blocking.
-    if (fcntl(pipe_fds[0], F_SETFL, O_NONBLOCK) == -1 ||
-        fcntl(pipe_fds[1], F_SETFL, O_NONBLOCK) == -1) {
-      mozilla::ipc::AnnotateCrashReportWithErrno(
-          CrashReporter::Annotation::IpcCreatePipeFcntlErrno, errno);
-      IGNORE_EINTR(close(pipe_fds[0]));
-      IGNORE_EINTR(close(pipe_fds[1]));
-      return false;
-    }
-
-    if (!SetCloseOnExec(pipe_fds[0]) || !SetCloseOnExec(pipe_fds[1])) {
-      mozilla::ipc::AnnotateCrashReportWithErrno(
-          CrashReporter::Annotation::IpcCreatePipeCloExecErrno, errno);
-      IGNORE_EINTR(close(pipe_fds[0]));
-      IGNORE_EINTR(close(pipe_fds[1]));
+    ChannelHandle server, client;
+    if (!Channel::CreateRawPipe(&server, &client)) {
       return false;
     }
 
-    SetPipe(pipe_fds[0]);
-    client_pipe_ = pipe_fds[1];
+    SetPipe(server.release());
+    client_pipe_ = client.release();
   } else {
     static mozilla::Atomic<bool> consumed(false);
     CHECK(!consumed.exchange(true))
@@ -267,15 +238,6 @@ bool Channel::ChannelImpl::CreatePipe(Mode mode) {
   return true;
 }
 
-/**
- * Reset the file descriptor for communication with the peer.
- */
-void Channel::ChannelImpl::ResetFileDescriptor(int fd) {
-  NS_ASSERTION(fd > 0 && fd == pipe_, "Invalid file descriptor");
-
-  EnqueueHelloMessage();
-}
-
 bool Channel::ChannelImpl::EnqueueHelloMessage() {
   mozilla::UniquePtr<Message> msg(
       new Message(MSG_ROUTING_NONE, HELLO_MESSAGE_TYPE));
@@ -1215,8 +1177,8 @@ Channel::Channel(const ChannelId& channel_id, Mode mode, Listener* listener)
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
-Channel::Channel(int fd, Mode mode, Listener* listener)
-    : channel_impl_(new ChannelImpl(fd, mode, listener)) {
+Channel::Channel(ChannelHandle pipe, Mode mode, Listener* listener)
+    : channel_impl_(new ChannelImpl(std::move(pipe), mode, listener)) {
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
@@ -1241,10 +1203,6 @@ void Channel::GetClientFileDescriptorMapping(int* src_fd, int* dest_fd) const {
   return channel_impl_->GetClientFileDescriptorMapping(src_fd, dest_fd);
 }
 
-void Channel::ResetFileDescriptor(int fd) {
-  channel_impl_->ResetFileDescriptor(fd);
-}
-
 int Channel::GetFileDescriptor() const {
   return channel_impl_->GetFileDescriptor();
 }
@@ -1279,4 +1237,48 @@ Channel::ChannelId Channel::GenerateVerifiedChannelID() { return {}; }
 // static
 Channel::ChannelId Channel::ChannelIDForCurrentProcess() { return {}; }
 
+// static
+bool Channel::CreateRawPipe(ChannelHandle* server, ChannelHandle* client) {
+  int fds[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) < 0) {
+    mozilla::ipc::AnnotateCrashReportWithErrno(
+        CrashReporter::Annotation::IpcCreatePipeSocketPairErrno, errno);
+    return false;
+  }
+
+  auto configureFd = [](int fd) -> bool {
+    // Mark the endpoints as non-blocking
+    if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) {
+      mozilla::ipc::AnnotateCrashReportWithErrno(
+          CrashReporter::Annotation::IpcCreatePipeFcntlErrno, errno);
+      return false;
+    }
+
+    // Mark the pipes as FD_CLOEXEC
+    int flags = fcntl(fd, F_GETFD);
+    if (flags == -1) {
+      mozilla::ipc::AnnotateCrashReportWithErrno(
+          CrashReporter::Annotation::IpcCreatePipeCloExecErrno, errno);
+      return false;
+    }
+    flags |= FD_CLOEXEC;
+    if (fcntl(fd, F_SETFD, flags) == -1) {
+      mozilla::ipc::AnnotateCrashReportWithErrno(
+          CrashReporter::Annotation::IpcCreatePipeCloExecErrno, errno);
+      return false;
+    }
+    return true;
+  };
+
+  if (!configureFd(fds[0]) || !configureFd(fds[1])) {
+    IGNORE_EINTR(close(fds[0]));
+    IGNORE_EINTR(close(fds[1]));
+    return false;
+  }
+
+  server->reset(fds[0]);
+  client->reset(fds[1]);
+  return true;
+}
+
 }  // namespace IPC
diff --git a/ipc/chromium/src/chrome/common/ipc_channel_posix.h b/ipc/chromium/src/chrome/common/ipc_channel_posix.h
@@ -34,7 +34,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
 
   // Mirror methods of Channel, see ipc_channel.h for description.
   ChannelImpl(const ChannelId& channel_id, Mode mode, Listener* listener);
-  ChannelImpl(int fd, Mode mode, Listener* listener);
+  ChannelImpl(ChannelHandle pipe, Mode mode, Listener* listener);
   ~ChannelImpl() { Close(); }
   bool Connect();
   void Close();
@@ -46,8 +46,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   bool Send(mozilla::UniquePtr<Message> message);
   void GetClientFileDescriptorMapping(int* src_fd, int* dest_fd) const;
 
-  void ResetFileDescriptor(int fd);
-
   int GetFileDescriptor() const { return pipe_; }
   void CloseClientFileDescriptor();