Bug 1838906 - Part 4: Don't require a listener in IPC::Channel until Connect, r=ipc-reviewers,jld

Depends on D181803

Differential Revision: https://phabricator.services.mozilla.com/D182225

ipc/chromium/src/chrome/common/child_thread.cc

@@ -32,8 +32,8 @@ void ChildThread::Init() {
   // to start the initial IPC connection to the parent process.
   IPC::Channel::ChannelHandle client_handle(
       IPC::Channel::GetClientChannelHandle());
-  auto channel = mozilla::MakeUnique<IPC::Channel>(
-      std::move(client_handle), IPC::Channel::MODE_CLIENT, nullptr);
+  auto channel = mozilla::MakeUnique<IPC::Channel>(std::move(client_handle),
+                                                   IPC::Channel::MODE_CLIENT);
 #if defined(XP_WIN)
   channel->StartAcceptingHandles(IPC::Channel::MODE_CLIENT);
 #elif defined(XP_DARWIN)

ipc/chromium/src/chrome/common/ipc_channel.h

@@ -82,13 +82,11 @@ class Channel {
   // |mode| specifies whether this channel is operating in server mode or client
   // mode. One side of the connection should be the client, and the other should
   // be the server.
-  // |listener| receives a callback on the current thread for each newly
-  // received message.
   //
   // The Channel must be created and destroyed on the IO thread, and all
   // methods, unless otherwise noted, are only safe to call on the I/O thread.
   //
-  Channel(ChannelHandle pipe, Mode mode, Listener* listener);
+  Channel(ChannelHandle pipe, Mode mode);
 
   ~Channel();
 
@@ -97,14 +95,14 @@ class Channel {
   // connect to a pre-existing pipe.  Note, calling Connect()
   // will not block the calling thread and may complete
   // asynchronously.
-  bool Connect();
+  //
+  // |listener| will receive a callback on the current thread for each newly
+  // received message.
+  bool Connect(Listener* listener);
 
   // Close this Channel explicitly.  May be called multiple times.
   void Close();
 
-  // Modify the Channel's listener.
-  Listener* set_listener(Listener* listener);
-
   // Send a message over the Channel to the listener on the other end.
   //
   // This method may be called from any thread, so long as the `Channel` is not

ipc/chromium/src/chrome/common/ipc_channel_posix.cc

@@ -140,11 +140,10 @@ void Channel::SetClientChannelFd(int fd) { gClientChannelFd = fd; }
 
 int Channel::GetClientChannelHandle() { return gClientChannelFd; }
 
-Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
-                                  Listener* listener)
+Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode)
     : chan_cap_("ChannelImpl::SendMutex",
                 MessageLoopForIO::current()->SerialEventTarget()) {
-  Init(mode, listener);
+  Init(mode);
   SetPipe(pipe.release());
 
   EnqueueHelloMessage();
@@ -176,7 +175,7 @@ bool Channel::ChannelImpl::PipeBufHasSpaceAfter(size_t already_written) {
          static_cast<size_t>(pipe_buf_len_) > already_written;
 }
 
-void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
+void Channel::ChannelImpl::Init(Mode mode) {
   // Verify that we fit in a "quantum-spaced" jemalloc bucket.
   static_assert(sizeof(*this) <= 512, "Exceeded expected size class");
 
@@ -193,7 +192,6 @@ void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
   input_buf_ = mozilla::MakeUnique<char[]>(Channel::kReadBufferSize);
   input_cmsg_buf_ = mozilla::MakeUnique<char[]>(kControlBufferSize);
   SetPipe(-1);
-  listener_ = listener;
   waiting_connect_ = true;
 #if defined(XP_DARWIN)
   last_pending_fd_id_ = 0;
@@ -213,19 +211,24 @@ bool Channel::ChannelImpl::EnqueueHelloMessage() {
   return true;
 }
 
-bool Channel::ChannelImpl::Connect() {
+bool Channel::ChannelImpl::Connect(Listener* listener) {
   IOThread().AssertOnCurrentThread();
   mozilla::MutexAutoLock lock(SendMutex());
-  return ConnectLocked();
-}
-
-bool Channel::ChannelImpl::ConnectLocked() {
   chan_cap_.NoteExclusiveAccess();
 
   if (pipe_ == -1) {
     return false;
   }
 
+  listener_ = listener;
+
+  return ContinueConnect();
+}
+
+bool Channel::ChannelImpl::ContinueConnect() {
+  chan_cap_.NoteExclusiveAccess();
+  MOZ_ASSERT(pipe_ != -1);
+
 #if defined(XP_DARWIN)
   // If we're still waiting for our peer task to be provided, don't start
   // listening yet. We'll start receiving messages once the task_t is set.
@@ -900,7 +903,7 @@ void Channel::ChannelImpl::SetOtherMachTask(task_t task) {
   MOZ_ASSERT(accept_mach_ports_ && privileged_ && waiting_connect_);
   other_task_ = mozilla::RetainMachSendRight(task);
   // Now that `other_task_` is provided, we can continue connecting.
-  ConnectLocked();
+  ContinueConnect();
 }
 
 void Channel::ChannelImpl::StartAcceptingMachPorts(Mode mode) {
@@ -1160,21 +1163,19 @@ bool Channel::ChannelImpl::TransferMachPorts(Message& msg) {
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
-Channel::Channel(ChannelHandle pipe, Mode mode, Listener* listener)
-    : channel_impl_(new ChannelImpl(std::move(pipe), mode, listener)) {
+Channel::Channel(ChannelHandle pipe, Mode mode)
+    : channel_impl_(new ChannelImpl(std::move(pipe), mode)) {
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
 Channel::~Channel() { MOZ_COUNT_DTOR(IPC::Channel); }
 
-bool Channel::Connect() { return channel_impl_->Connect(); }
+bool Channel::Connect(Listener* listener) {
+  return channel_impl_->Connect(listener);
+}
 
 void Channel::Close() { channel_impl_->Close(); }
 
-Channel::Listener* Channel::set_listener(Listener* listener) {
-  return channel_impl_->set_listener(listener);
-}
-
 bool Channel::Send(mozilla::UniquePtr<Message> message) {
   return channel_impl_->Send(std::move(message));
 }

ipc/chromium/src/chrome/common/ipc_channel_posix.h

@@ -36,16 +36,10 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
       ChannelImpl, IOThread().GetEventTarget());
 
   // Mirror methods of Channel, see ipc_channel.h for description.
-  ChannelImpl(ChannelHandle pipe, Mode mode, Listener* listener);
-  bool Connect() MOZ_EXCLUDES(SendMutex());
+  ChannelImpl(ChannelHandle pipe, Mode mode);
+  bool Connect(Listener* listener) MOZ_EXCLUDES(SendMutex());
   void Close() MOZ_EXCLUDES(SendMutex());
-  Listener* set_listener(Listener* listener) {
-    IOThread().AssertOnCurrentThread();
-    chan_cap_.NoteOnIOThread();
-    Listener* old = listener_;
-    listener_ = listener;
-    return old;
-  }
+
   // NOTE: `Send` may be called on threads other than the I/O thread.
   bool Send(mozilla::UniquePtr<Message> message) MOZ_EXCLUDES(SendMutex());
 
@@ -72,8 +66,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
  private:
   ~ChannelImpl() { Close(); }
 
-  void Init(Mode mode, Listener* listener)
-      MOZ_REQUIRES(SendMutex(), IOThread());
+  void Init(Mode mode) MOZ_REQUIRES(SendMutex(), IOThread());
   void SetPipe(int fd) MOZ_REQUIRES(SendMutex(), IOThread());
   void SetOtherPid(int other_pid) MOZ_REQUIRES(IOThread())
       MOZ_EXCLUDES(SendMutex()) {
@@ -84,7 +77,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   bool PipeBufHasSpaceAfter(size_t already_written)
       MOZ_REQUIRES_SHARED(chan_cap_);
   bool EnqueueHelloMessage() MOZ_REQUIRES(SendMutex(), IOThread());
-  bool ConnectLocked() MOZ_REQUIRES(SendMutex(), IOThread());
+  bool ContinueConnect() MOZ_REQUIRES(SendMutex(), IOThread());
   void CloseLocked() MOZ_REQUIRES(SendMutex(), IOThread());
 
   bool ProcessIncomingMessages() MOZ_REQUIRES(IOThread());

ipc/chromium/src/chrome/common/ipc_channel_win.cc

@@ -42,13 +42,12 @@ Channel::ChannelImpl::State::~State() {
 
 //------------------------------------------------------------------------------
 
-Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
-                                  Listener* listener)
+Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode)
     : chan_cap_("ChannelImpl::SendMutex",
                 MessageLoopForIO::current()->SerialEventTarget()),
       ALLOW_THIS_IN_INITIALIZER_LIST(input_state_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(output_state_(this)) {
-  Init(mode, listener);
+  Init(mode);
 
   if (!pipe) {
     return;
@@ -58,7 +57,7 @@ Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
   EnqueueHelloMessage();
 }
 
-void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
+void Channel::ChannelImpl::Init(Mode mode) {
   // Verify that we fit in a "quantum-spaced" jemalloc bucket.
   static_assert(sizeof(*this) <= 512, "Exceeded expected size class");
 
@@ -66,7 +65,6 @@ void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
 
   mode_ = mode;
   pipe_ = INVALID_HANDLE_VALUE;
-  listener_ = listener;
   waiting_connect_ = true;
   processing_incoming_ = false;
   input_buf_offset_ = 0;
@@ -181,13 +179,15 @@ bool Channel::ChannelImpl::EnqueueHelloMessage() {
   return true;
 }
 
-bool Channel::ChannelImpl::Connect() {
+bool Channel::ChannelImpl::Connect(Listener* listener) {
   IOThread().AssertOnCurrentThread();
   mozilla::MutexAutoLock lock(SendMutex());
   chan_cap_.NoteExclusiveAccess();
 
   if (pipe_ == INVALID_HANDLE_VALUE) return false;
 
+  listener_ = listener;
+
   MessageLoopForIO::current()->RegisterIOHandler(pipe_, this);
   waiting_connect_ = false;
 
@@ -639,14 +639,16 @@ bool Channel::ChannelImpl::TransferHandles(Message& msg) {
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
-Channel::Channel(ChannelHandle pipe, Mode mode, Listener* listener)
-    : channel_impl_(new ChannelImpl(std::move(pipe), mode, listener)) {
+Channel::Channel(ChannelHandle pipe, Mode mode)
+    : channel_impl_(new ChannelImpl(std::move(pipe), mode)) {
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
 Channel::~Channel() { MOZ_COUNT_DTOR(IPC::Channel); }
 
-bool Channel::Connect() { return channel_impl_->Connect(); }
+bool Channel::Connect(Listener* listener) {
+  return channel_impl_->Connect(listener);
+}
 
 void Channel::Close() { channel_impl_->Close(); }
 
@@ -654,10 +656,6 @@ void Channel::StartAcceptingHandles(Mode mode) {
   channel_impl_->StartAcceptingHandles(mode);
 }
 
-Channel::Listener* Channel::set_listener(Listener* listener) {
-  return channel_impl_->set_listener(listener);
-}
-
 bool Channel::Send(mozilla::UniquePtr<Message> message) {
   return channel_impl_->Send(std::move(message));
 }

ipc/chromium/src/chrome/common/ipc_channel_win.h

@@ -34,17 +34,10 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
   using ChannelHandle = Channel::ChannelHandle;
 
   // Mirror methods of Channel, see ipc_channel.h for description.
-  ChannelImpl(ChannelHandle pipe, Mode mode, Listener* listener);
-  bool Connect() MOZ_EXCLUDES(SendMutex());
+  ChannelImpl(ChannelHandle pipe, Mode mode);
+  bool Connect(Listener* listener) MOZ_EXCLUDES(SendMutex());
   void Close() MOZ_EXCLUDES(SendMutex());
   void StartAcceptingHandles(Mode mode) MOZ_EXCLUDES(SendMutex());
-  Listener* set_listener(Listener* listener) {
-    IOThread().AssertOnCurrentThread();
-    chan_cap_.NoteOnIOThread();
-    Listener* old = listener_;
-    listener_ = listener;
-    return old;
-  }
   // NOTE: `Send` may be called on threads other than the I/O thread.
   bool Send(mozilla::UniquePtr<Message> message) MOZ_EXCLUDES(SendMutex());
 
@@ -71,8 +64,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
     }
   }
 
-  void Init(Mode mode, Listener* listener)
-      MOZ_REQUIRES(SendMutex(), IOThread());
+  void Init(Mode mode) MOZ_REQUIRES(SendMutex(), IOThread());
 
   void OutputQueuePush(mozilla::UniquePtr<Message> msg)
       MOZ_REQUIRES(SendMutex());

ipc/glue/GeckoChildProcessHost.cpp

@@ -874,8 +874,8 @@ void GeckoChildProcessHost::InitializeChannel(
     IPC::Channel::ChannelHandle&& aServerHandle) {
   // Create the IPC channel which will be used for communication with this
   // process.
-  mozilla::UniquePtr<IPC::Channel> channel(new IPC::Channel(
-      std::move(aServerHandle), IPC::Channel::MODE_SERVER, nullptr));
+  mozilla::UniquePtr<IPC::Channel> channel = MakeUnique<IPC::Channel>(
+      std::move(aServerHandle), IPC::Channel::MODE_SERVER);
 #if defined(XP_WIN)
   channel->StartAcceptingHandles(IPC::Channel::MODE_SERVER);
 #elif defined(XP_DARWIN)

ipc/glue/NodeChannel.cpp

@@ -85,9 +85,7 @@ void NodeChannel::FinalDestroy() {
 void NodeChannel::Start() {
   AssertIOThread();
 
-  MOZ_ALWAYS_TRUE(nullptr == mChannel->set_listener(this));
-
-  if (!mChannel->Connect()) {
+  if (!mChannel->Connect(this)) {
     OnChannelError();
   }
 }
@@ -97,7 +95,6 @@ void NodeChannel::Close() {
 
   if (mState.exchange(State::Closed) != State::Closed) {
     mChannel->Close();
-    mChannel->set_listener(nullptr);
   }
 }
 
@@ -296,9 +293,8 @@ void NodeChannel::OnChannelError() {
     return;
   }
 
-  // Clean up the channel and make sure we're no longer the active listener.
+  // Clean up the channel.
   mChannel->Close();
-  MOZ_ALWAYS_TRUE(this == mChannel->set_listener(nullptr));
 
   // Tell our listener about the error.
   mListener->OnChannelError(mName);

ipc/glue/NodeController.cpp

@@ -611,7 +611,7 @@ void NodeController::OnIntroduce(const NodeName& aFromNode,
   }
 
   auto channel = MakeUnique<IPC::Channel>(std::move(aIntroduction.mHandle),
-                                          aIntroduction.mMode, nullptr);
+                                          aIntroduction.mMode);
   auto nodeChannel = MakeRefPtr<NodeChannel>(
       aIntroduction.mName, std::move(channel), this, aIntroduction.mOtherPid);