Bug 1807049 - Refactor MessageChannel shutdown states, r=ipc-reviewers,mccr8

This refactoring cleans up some dead code, and makes some semantic
changes to how the MessageChannel lifecycle is handled.

These changes ensure that messages which were sent by a peer before the
GOODBYE message will be delivered, without allowing messages sent after
the GOODBYE message (e.g. by a misbehaving process) to be delivered.

The lifecycle and shutdown states were simplified, and moved to be
entirely in MessageChannel, rather than split between MessageChannel and
MessageLink.

The dead-code ChannelTimeout error state was removed, along with the
corresponding CloseWithTimeout method.

The CloseWithError method was updated to behave more consistently with
the normal Close method, synchronously triggering a connection error,
and closing the MessageLink. This method is currently unused, but will
useful in the future for handling processing errors.

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

ipc/glue/MessageChannel.cpp

@@ -482,11 +482,6 @@ MessageChannel::~MessageChannel() {
             "MessageChannel destroyed without being closed "
             "(mChannelState == ChannelConnected).");
         break;
-      case ChannelTimeout:
-        MOZ_CRASH(
-            "MessageChannel destroyed without being closed "
-            "(mChannelState == ChannelTimeout).");
-        break;
       case ChannelClosing:
         MOZ_CRASH(
             "MessageChannel destroyed without being closed "
@@ -588,6 +583,11 @@ bool MessageChannel::Connected() const {
   return ChannelConnected == mChannelState;
 }
 
+bool MessageChannel::ConnectedOrClosing() const {
+  mMonitor->AssertCurrentThreadOwns();
+  return ChannelConnected == mChannelState || ChannelClosing == mChannelState;
+}
+
 bool MessageChannel::CanSend() const {
   if (!mMonitor) {
     return false;
@@ -600,6 +600,7 @@ void MessageChannel::Clear() {
   AssertWorkerThread();
   mMonitor->AssertCurrentThreadOwns();
   MOZ_DIAGNOSTIC_ASSERT(IsClosedLocked(), "MessageChannel cleared too early?");
+  MOZ_ASSERT(ChannelClosed == mChannelState || ChannelError == mChannelState);
 
   // Don't clear mWorkerThread; we use it in AssertWorkerThread().
   //
@@ -680,6 +681,7 @@ bool MessageChannel::Open(ScopedPort aPort, Side aSide,
     mWorkerThread = eventTarget;
     mShutdownTask = shutdownTask;
     mLink = MakeUnique<PortLink>(this, std::move(aPort));
+    mChannelState = ChannelConnected;
     mSide = aSide;
   }
 
@@ -935,8 +937,9 @@ bool MessageChannel::MaybeInterceptSpecialIOMessage(const Message& aMsg) {
 
   if (MSG_ROUTING_NONE == aMsg.routing_id()) {
     if (GOODBYE_MESSAGE_TYPE == aMsg.type()) {
-      // :TODO: Sort out Close() on this side racing with Close() on the
-      // other side
+      // We've received a GOODBYE message, close the connection and mark
+      // ourselves as "Closing".
+      mLink->Close();
       mChannelState = ChannelClosing;
       if (LoggingEnabled()) {
         printf(
@@ -946,6 +949,14 @@ bool MessageChannel::MaybeInterceptSpecialIOMessage(const Message& aMsg) {
             static_cast<uint32_t>(base::GetCurrentProcId()),
             (mSide == ChildSide) ? "child" : "parent");
       }
+
+      // Notify the worker thread that the connection has been closed, as we
+      // will not receive an `OnChannelErrorFromLink` after calling
+      // `mLink->Close()`.
+      if (AwaitingSyncReply()) {
+        NotifyWorkerThread();
+      }
+      PostErrorNotifyTask();
       return true;
     } else if (CANCEL_MESSAGE_TYPE == aMsg.type()) {
       IPC_LOG("Cancel from message");
@@ -1017,6 +1028,7 @@ bool MessageChannel::ShouldDeferMessage(const Message& aMsg) {
 
 void MessageChannel::OnMessageReceivedFromLink(UniquePtr<Message> aMsg) {
   mMonitor->AssertCurrentThreadOwns();
+  MOZ_ASSERT(mChannelState == ChannelConnected);
 
   if (MaybeInterceptSpecialIOMessage(*aMsg)) {
     return;
@@ -1429,7 +1441,7 @@ bool MessageChannel::Send(UniquePtr<Message> aMsg, UniquePtr<Message>* aReply) {
 bool MessageChannel::HasPendingEvents() {
   AssertWorkerThread();
   mMonitor->AssertCurrentThreadOwns();
-  return Connected() && !mPending.isEmpty();
+  return ConnectedOrClosing() && !mPending.isEmpty();
 }
 
 bool MessageChannel::ProcessPendingRequest(ActorLifecycleProxy* aProxy,
@@ -1444,7 +1456,7 @@ bool MessageChannel::ProcessPendingRequest(ActorLifecycleProxy* aProxy,
   msgid_t msgType = aUrgent->type();
 
   DispatchMessage(aProxy, std::move(aUrgent));
-  if (!Connected()) {
+  if (!ConnectedOrClosing()) {
     ReportConnectionError("ProcessPendingRequest", msgType);
     return false;
   }
@@ -1490,7 +1502,7 @@ void MessageChannel::RunMessage(ActorLifecycleProxy* aProxy,
 
   UniquePtr<Message>& msg = aTask.Msg();
 
-  if (!Connected()) {
+  if (!ConnectedOrClosing()) {
     ReportConnectionError("RunMessage", msg->type());
     return;
   }
@@ -1902,12 +1914,8 @@ void MessageChannel::ReportConnectionError(const char* aFunctionName,
     case ChannelClosed:
       errorMsg = "Closed channel: cannot send/recv";
       break;
-    case ChannelTimeout:
-      errorMsg = "Channel timeout: cannot send/recv";
-      break;
     case ChannelClosing:
-      errorMsg =
-          "Channel closing: too late to send/recv, messages will be lost";
+      errorMsg = "Channel closing: too late to send, messages will be lost";
       break;
     case ChannelError:
       errorMsg = "Channel error: cannot send/recv";
@@ -1990,6 +1998,7 @@ bool MessageChannel::MaybeHandleError(Result code, const Message& aMsg,
 
 void MessageChannel::OnChannelErrorFromLink() {
   mMonitor->AssertCurrentThreadOwns();
+  MOZ_ASSERT(mChannelState == ChannelConnected);
 
   IPC_LOG("OnChannelErrorFromLink");
 
@@ -1997,22 +2006,20 @@ void MessageChannel::OnChannelErrorFromLink() {
     NotifyWorkerThread();
   }
 
-  if (ChannelClosing != mChannelState) {
-    if (mAbortOnError) {
-      // mAbortOnError is set by main actors (e.g., ContentChild) to ensure
-      // that the process terminates even if normal shutdown is prevented.
-      // A MOZ_CRASH() here is not helpful because crash reporting relies
-      // on the parent process which we know is dead or otherwise unusable.
-      //
-      // Additionally, the parent process can (and often is) killed on Android
-      // when apps are backgrounded. We don't need to report a crash for
-      // normal behavior in that case.
-      printf_stderr("Exiting due to channel error.\n");
-      ProcessChild::QuickExit();
-    }
-    mChannelState = ChannelError;
-    mMonitor->Notify();
+  if (mAbortOnError) {
+    // mAbortOnError is set by main actors (e.g., ContentChild) to ensure
+    // that the process terminates even if normal shutdown is prevented.
+    // A MOZ_CRASH() here is not helpful because crash reporting relies
+    // on the parent process which we know is dead or otherwise unusable.
+    //
+    // Additionally, the parent process can (and often is) killed on Android
+    // when apps are backgrounded. We don't need to report a crash for
+    // normal behavior in that case.
+    printf_stderr("Exiting due to channel error.\n");
+    ProcessChild::QuickExit();
   }
+  mChannelState = ChannelError;
+  mMonitor->Notify();
 
   PostErrorNotifyTask();
 }
@@ -2021,9 +2028,9 @@ void MessageChannel::NotifyMaybeChannelError(ReleasableMonitorAutoLock& aLock) {
   AssertWorkerThread();
   mMonitor->AssertCurrentThreadOwns();
   aLock.AssertCurrentThreadOwns();
+  MOZ_ASSERT(mChannelState != ChannelConnected);
 
-  // TODO sort out Close() on this side racing with Close() on the other side
-  if (ChannelClosing == mChannelState) {
+  if (ChannelClosing == mChannelState || ChannelClosed == mChannelState) {
     // the channel closed, but we received a "Goodbye" message warning us
     // about it. no worries
     mChannelState = ChannelClosed;
@@ -2031,10 +2038,9 @@ void MessageChannel::NotifyMaybeChannelError(ReleasableMonitorAutoLock& aLock) {
     return;
   }
 
-  Clear();
+  MOZ_ASSERT(ChannelError == mChannelState);
 
-  // Oops, error!  Let the listener know about it.
-  mChannelState = ChannelError;
+  Clear();
 
   // IPDL assumes these notifications do not fire twice, so we do not let
   // that happen.
@@ -2101,38 +2107,31 @@ class GoodbyeMessage : public IPC::Message {
   }
 };
 
-void MessageChannel::SynchronouslyClose() {
-  AssertWorkerThread();
-  mMonitor->AssertCurrentThreadOwns();
-  mLink->SendClose();
-
-  MOZ_RELEASE_ASSERT(!mIsSameThreadChannel || ChannelClosed == mChannelState,
-                     "same-thread channel failed to synchronously close?");
-
-  while (ChannelClosed != mChannelState) mMonitor->Wait();
-}
-
 void MessageChannel::CloseWithError() {
   AssertWorkerThread();
 
-  MonitorAutoLock lock(*mMonitor);
-  if (ChannelConnected != mChannelState) {
-    return;
-  }
-  SynchronouslyClose();
-  mChannelState = ChannelError;
-  PostErrorNotifyTask();
-}
+  // This lock guard may be reset by `NotifyMaybeChannelError` before invoking
+  // listener callbacks which may destroy this `MessageChannel`.
+  ReleasableMonitorAutoLock lock(*mMonitor);
 
-void MessageChannel::CloseWithTimeout() {
-  AssertWorkerThread();
-
-  MonitorAutoLock lock(*mMonitor);
-  if (ChannelConnected != mChannelState) {
-    return;
+  switch (mChannelState) {
+    case ChannelError:
+      // Already errored, ensure we notify if we haven't yet.
+      NotifyMaybeChannelError(lock);
+      return;
+    case ChannelClosed:
+      // Already closed, we can't do anything.
+      return;
+    default:
+      // Either connected or closing, immediately convert to an error, and
+      // notify.
+      MOZ_ASSERT(mChannelState == ChannelConnected ||
+                 mChannelState == ChannelClosing);
+      mLink->Close();
+      mChannelState = ChannelError;
+      NotifyMaybeChannelError(lock);
+      return;
   }
-  SynchronouslyClose();
-  mChannelState = ChannelTimeout;
 }
 
 void MessageChannel::NotifyImpendingShutdown() {
@@ -2154,7 +2153,6 @@ void MessageChannel::Close() {
 
   switch (mChannelState) {
     case ChannelError:
-    case ChannelTimeout:
       // See bug 538586: if the listener gets deleted while the
       // IO thread's NotifyChannelError event is still enqueued
       // and subsequently deletes us, then the error event will
@@ -2173,7 +2171,8 @@ void MessageChannel::Close() {
       if (ChannelConnected == mChannelState) {
         SendMessageToLink(MakeUnique<GoodbyeMessage>());
       }
-      SynchronouslyClose();
+      mLink->Close();
+      mChannelState = ChannelClosed;
       NotifyChannelClosed(lock);
       return;
   }

ipc/glue/MessageChannel.h

@@ -100,7 +100,6 @@ using RejectCallback = std::function<void(ResponseRejectReason)>;
 enum ChannelState {
   ChannelClosed,
   ChannelConnected,
-  ChannelTimeout,
   ChannelClosing,
   ChannelError
 };
@@ -198,12 +197,10 @@ class MessageChannel : HasResultCodes {
   // Close the underlying transport channel.
   void Close() MOZ_EXCLUDES(*mMonitor);
 
-  // Force the channel to behave as if a channel error occurred. Valid
-  // for process links only, not thread links.
+  // Close the underlying transport channel, treating the closure as a
+  // connection error.
   void CloseWithError() MOZ_EXCLUDES(*mMonitor);
 
-  void CloseWithTimeout() MOZ_EXCLUDES(*mMonitor);
-
   void SetAbortOnError(bool abort) MOZ_EXCLUDES(*mMonitor) {
     MonitorAutoLock lock(*mMonitor);
     mAbortOnError = abort;
@@ -439,8 +436,16 @@ class MessageChannel : HasResultCodes {
     return mDispatchingAsyncMessageNestedLevel;
   }
 
+  // Check if there is still a live connection to our peer. This may change to
+  // `false` at any time due to the connection to our peer being closed or
+  // dropped (e.g. due to a crash).
   bool Connected() const MOZ_REQUIRES(*mMonitor);
 
+  // Check if there is either still a live connection to our peer, or we have
+  // received a `Goodbye` from our peer, and are actively shutting down our
+  // connection with our peer.
+  bool ConnectedOrClosing() const MOZ_REQUIRES(*mMonitor);
+
  private:
   // Executed on the IO thread.
   void NotifyWorkerThread() MOZ_REQUIRES(*mMonitor);
@@ -450,9 +455,6 @@ class MessageChannel : HasResultCodes {
   bool MaybeInterceptSpecialIOMessage(const Message& aMsg)
       MOZ_REQUIRES(*mMonitor);
 
-  // Tell the IO thread to close the channel and wait for it to ACK.
-  void SynchronouslyClose() MOZ_REQUIRES(*mMonitor);
-
   // Returns true if ShouldDeferMessage(aMsg) is guaranteed to return true.
   // Otherwise, the result of ShouldDeferMessage(aMsg) may be true or false,
   // depending on context.

ipc/glue/MessageLink.cpp

@@ -65,8 +65,6 @@ PortLink::PortLink(MessageChannel* aChan, ScopedPort aPort)
   mObserver = new PortObserverThunk(mChan->mMonitor, this);
   mNode->SetPortObserver(mPort, mObserver);
 
-  mChan->mChannelState = ChannelConnected;
-
   // Dispatch an event to the IO loop to trigger an initial
   // `OnPortStatusChanged` to deliver any pending messages. This needs to be run
   // asynchronously from a different thread (or in the case of a same-thread
@@ -132,13 +130,9 @@ void PortLink::SendMessage(UniquePtr<Message> aMessage) {
   }
 }
 
-void PortLink::SendClose() {
+void PortLink::Close() {
   mChan->mMonitor->AssertCurrentThreadOwns();
 
-  // Our channel has been closed, mark it as such.
-  mChan->mChannelState = ChannelClosed;
-  mChan->mMonitor->Notify();
-
   if (!mObserver) {
     // We're already being closed.
     return;

ipc/glue/MessageLink.h

@@ -53,7 +53,11 @@ class MessageLink {
   // n.b.: These methods all require that the channel monitor is
   // held when they are invoked.
   virtual void SendMessage(mozilla::UniquePtr<Message> msg) = 0;
-  virtual void SendClose() = 0;
+
+  // Synchronously close the connection, such that no further notifications will
+  // be delivered to the MessageChannel instance. Must be called with the
+  // channel monitor held.
+  virtual void Close() = 0;
 
   virtual bool IsClosed() const = 0;
 
@@ -76,7 +80,7 @@ class PortLink final : public MessageLink {
   virtual ~PortLink();
 
   void SendMessage(UniquePtr<Message> aMessage) override;
-  void SendClose() override;
+  void Close() override;
 
   bool IsClosed() const override;