diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp index 48b3ada7a02d..ea01c54f0da5 100644 --- a/ipc/glue/MessageChannel.cpp +++ b/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(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(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 aMsg) { mMonitor->AssertCurrentThreadOwns(); + MOZ_ASSERT(mChannelState == ChannelConnected); if (MaybeInterceptSpecialIOMessage(*aMsg)) { return; @@ -1429,7 +1441,7 @@ bool MessageChannel::Send(UniquePtr aMsg, UniquePtr* 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& 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()); } - SynchronouslyClose(); + mLink->Close(); + mChannelState = ChannelClosed; NotifyChannelClosed(lock); return; } diff --git a/ipc/glue/MessageChannel.h b/ipc/glue/MessageChannel.h index 0b268eb96d4c..e52ce7974fad 100644 --- a/ipc/glue/MessageChannel.h +++ b/ipc/glue/MessageChannel.h @@ -100,7 +100,6 @@ using RejectCallback = std::function; 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. diff --git a/ipc/glue/MessageLink.cpp b/ipc/glue/MessageLink.cpp index bc8a9456a536..872ff48b27da 100644 --- a/ipc/glue/MessageLink.cpp +++ b/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 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; diff --git a/ipc/glue/MessageLink.h b/ipc/glue/MessageLink.h index 504867ee82db..854dc6e2f1eb 100644 --- a/ipc/glue/MessageLink.h +++ b/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 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 aMessage) override; - void SendClose() override; + void Close() override; bool IsClosed() const override;