Bug 1725335 - Streamline ownership and locking in MessageTask, r=ipc-reviewers,mccr8

This simplifies the logic around MessageTask's lifecycle to make
ownership as clear as possible and reduce the number of redundant
checks.

This new change no longer clears the mChannel member when the
MessageTask is disconnected, instead relying on isInList() to check
whether the MessageTask is still in the channel's mPending list. This is
already being automatically managed as the mPending list is modified,
and should avoid potential usage mistakes.

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

ipc/glue/MessageChannel.cpp

@@ -757,9 +757,6 @@ void MessageChannel::Clear() {
   }
 
   // Free up any memory used by pending messages.
-  for (MessageTask* task : mPending) {
-    task->Clear();
-  }
   mPending.clear();
 
   mMaybeDeferredPendingCount = 0;
@@ -1832,19 +1829,15 @@ NS_IMPL_ISUPPORTS_INHERITED(MessageChannel::MessageTask, CancelableRunnable,
 MessageChannel::MessageTask::MessageTask(MessageChannel* aChannel,
                                          Message&& aMessage)
     : CancelableRunnable(aMessage.name()),
+      mMonitor(aChannel->mMonitor),
       mChannel(aChannel),
       mMessage(std::move(aMessage)),
       mScheduled(false) {}
 
 nsresult MessageChannel::MessageTask::Run() {
-  if (!mChannel) {
-    return NS_OK;
-  }
+  mMonitor->AssertNotCurrentThreadOwns();
 
-  mChannel->AssertWorkerThread();
-  mChannel->mMonitor->AssertNotCurrentThreadOwns();
-
-  MonitorAutoLock lock(*mChannel->mMonitor);
+  MonitorAutoLock lock(*mMonitor);
 
   // In case we choose not to run this message, we may need to be able to Post
   // it again.
@@ -1854,34 +1847,33 @@ nsresult MessageChannel::MessageTask::Run() {
     return NS_OK;
   }
 
-  mChannel->RunMessage(*this);
+  Channel()->AssertWorkerThread();
+  Channel()->RunMessage(*this);
   return NS_OK;
 }
 
 // Warning: This method removes the receiver from whatever list it might be in.
 nsresult MessageChannel::MessageTask::Cancel() {
-  if (!mChannel) {
-    return NS_OK;
-  }
+  mMonitor->AssertNotCurrentThreadOwns();
 
-  mChannel->AssertWorkerThread();
-  mChannel->mMonitor->AssertNotCurrentThreadOwns();
-
-  MonitorAutoLock lock(*mChannel->mMonitor);
+  MonitorAutoLock lock(*mMonitor);
 
   if (!isInList()) {
     return NS_OK;
   }
-  remove();
 
+  Channel()->AssertWorkerThread();
   if (!IsAlwaysDeferred(Msg())) {
-    mChannel->mMaybeDeferredPendingCount--;
+    Channel()->mMaybeDeferredPendingCount--;
   }
 
+  remove();
+
   return NS_OK;
 }
 
 void MessageChannel::MessageTask::Post() {
+  mMonitor->AssertCurrentThreadOwns();
   MOZ_RELEASE_ASSERT(!mScheduled);
   MOZ_RELEASE_ASSERT(isInList());
 
@@ -1889,21 +1881,15 @@ void MessageChannel::MessageTask::Post() {
 
   RefPtr<MessageTask> self = this;
   nsCOMPtr<nsISerialEventTarget> eventTarget =
-      mChannel->mListener->GetMessageEventTarget(mMessage);
+      Channel()->mListener->GetMessageEventTarget(mMessage);
 
   if (eventTarget) {
     eventTarget->Dispatch(self.forget(), NS_DISPATCH_NORMAL);
   } else {
-    mChannel->mWorkerThread->Dispatch(self.forget());
+    Channel()->mWorkerThread->Dispatch(self.forget());
   }
 }
 
-void MessageChannel::MessageTask::Clear() {
-  mChannel->AssertWorkerThread();
-
-  mChannel = nullptr;
-}
-
 NS_IMETHODIMP
 MessageChannel::MessageTask::GetPriority(uint32_t* aPriority) {
   switch (mMessage.priority()) {

ipc/glue/MessageChannel.h

@@ -530,6 +530,8 @@ class MessageChannel : HasResultCodes {
                       public nsIRunnableIPCMessageType {
    public:
     explicit MessageTask(MessageChannel* aChannel, Message&& aMessage);
+    MessageTask() = delete;
+    MessageTask(const MessageTask&) = delete;
 
     NS_DECL_ISUPPORTS_INHERITED
 
@@ -538,19 +540,30 @@ class MessageChannel : HasResultCodes {
     NS_IMETHOD GetPriority(uint32_t* aPriority) override;
     NS_DECL_NSIRUNNABLEIPCMESSAGETYPE
     void Post();
-    void Clear();
 
-    bool IsScheduled() const { return mScheduled; }
+    bool IsScheduled() const {
+      mMonitor->AssertCurrentThreadOwns();
+      return mScheduled;
+    }
 
     Message& Msg() { return mMessage; }
     const Message& Msg() const { return mMessage; }
 
    private:
-    MessageTask() = delete;
-    MessageTask(const MessageTask&) = delete;
     ~MessageTask() = default;
 
-    MessageChannel* mChannel;
+    MessageChannel* Channel() {
+      mMonitor->AssertCurrentThreadOwns();
+      MOZ_RELEASE_ASSERT(isInList());
+      return mChannel;
+    }
+
+    // The connected MessageChannel's monitor. Guards `mChannel` and
+    // `mScheduled`.
+    RefPtr<RefCountedMonitor> const mMonitor;
+    // The channel which this MessageTask is associated with. Only valid while
+    // `mMonitor` is held, and this MessageTask `isInList()`.
+    MessageChannel* const mChannel;
     Message mMessage;
     bool mScheduled : 1;
   };