Bug 1090374 - Convert MessageChannel::mLink to a UniquePtr. r=froydnj

The change to MessageChannel::Clear() makes mLink get cleared before
we call ~ThreadLink. This causes a race because Clear() is not
holding the monitor. To work around this, I introduced a new method
PrepareToDestroy() that handles the ThreadLink splitting. Once the
ThreadLinks are split, MessageChannel can clear mLink without a
race.

An alternative approach would be to hold the monitor in Clear()
before mLink is cleared, but then we'd end up acquiring the lock
when we didn't need to in the case where mLink is a ProcessLink.

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

ipc/glue/MessageChannel.cpp

@@ -567,7 +567,6 @@ MessageChannel::MessageChannel(const char* aName, IToplevelProtocol* aListener)
       mChannelState(ChannelClosed),
       mSide(UnknownSide),
       mIsCrossProcess(false),
-      mLink(nullptr),
       mWorkerLoop(nullptr),
       mChannelErrorTask(nullptr),
       mWorkerThread(nullptr),
@@ -789,8 +788,11 @@ void MessageChannel::Clear() {
   if (mLink != nullptr && mIsCrossProcess) {
     ChannelCountReporter::Decrement(mName);
   }
-  delete mLink;
-  mLink = nullptr;
+
+  if (mLink) {
+    mLink->PrepareToDestroy();
+    mLink = nullptr;
+  }
 
   mOnChannelConnectedTask->Cancel();
 
@@ -823,10 +825,10 @@ bool MessageChannel::Open(mozilla::UniquePtr<Transport> aTransport,
   mWorkerLoop->AddDestructionObserver(this);
   mListener->OnIPCChannelOpened();
 
-  ProcessLink* link = new ProcessLink(this);
+  auto link = MakeUnique<ProcessLink>(this);
   link->Open(std::move(aTransport), aIOLoop,
              aSide);  // :TODO: n.b.: sets mChild
-  mLink = link;
+  mLink = std::move(link);
   mIsCrossProcess = true;
   ChannelCountReporter::Increment(mName);
   return true;
@@ -905,7 +907,7 @@ void MessageChannel::CommonThreadOpenInit(MessageChannel* aTargetChan,
   mWorkerLoop->AddDestructionObserver(this);
   mListener->OnIPCChannelOpened();
 
-  mLink = new ThreadLink(this, aTargetChan);
+  mLink = MakeUnique<ThreadLink>(this, aTargetChan);
   mSide = aSide;
 }
 

ipc/glue/MessageChannel.h

@@ -624,7 +624,7 @@ class MessageChannel : HasResultCodes, MessageLoop::DestructionObserver {
   RefPtr<RefCountedMonitor> mMonitor;
   Side mSide;
   bool mIsCrossProcess;
-  MessageLink* mLink;
+  UniquePtr<MessageLink> mLink;
   MessageLoop* mWorkerLoop;  // thread where work is done
   RefPtr<CancelableRunnable>
       mChannelErrorTask;  // NotifyMaybeChannelError runnable

ipc/glue/MessageLink.cpp

@@ -166,7 +166,7 @@ void ProcessLink::SendClose() {
 ThreadLink::ThreadLink(MessageChannel* aChan, MessageChannel* aTargetChan)
     : MessageLink(aChan), mTargetChan(aTargetChan) {}
 
-ThreadLink::~ThreadLink() {
+void ThreadLink::PrepareToDestroy() {
   MOZ_ASSERT(mChan);
   MOZ_ASSERT(mChan->mMonitor);
   MonitorAutoLock lock(*mChan->mMonitor);
@@ -186,10 +186,36 @@ ThreadLink::~ThreadLink() {
   // to our MessageChannel.  Note that we must hold the monitor so
   // that we do this atomically with respect to them trying to send
   // us a message.  Since the channels share the same monitor this
-  // also protects against the two ~ThreadLink() calls racing.
+  // also protects against the two PrepareToDestroy() calls racing.
+  //
+  //
+  // Why splitting is done in a method separate from ~ThreadLink:
+  //
+  // ThreadLinks are destroyed in MessageChannel::Clear(), when
+  // nullptr is assigned to the UniquePtr<> MessageChannel::mLink.
+  // This single line of code gets executed in three separate steps:
+  // 1. Load the value of mLink into a temporary.
+  // 2. Store nullptr in the mLink field.
+  // 3. Call the destructor on the temporary from step 1.
+  // This is all done without holding the monitor.
+  // The splitting operation, among other things, loads the mLink field
+  // of the other thread's MessageChannel while holding the monitor.
+  // If splitting was done in the destructor, and the two sides were
+  // both running MessageChannel::Clear(), then there would be a race
+  // between the store to mLink in Clear() and the load of mLink
+  // during splitting.
+  // Instead, we call PrepareToDestroy() prior to step 1. One thread or
+  // the other will run the entire method before the other thread,
+  // because this method acquires the monitor. Once that is done, the
+  // mTargetChan of both ThreadLink will be null, so they will no
+  // longer be able to access the other and so there won't be any races.
+  //
+  // An alternate approach would be to hold the monitor in Clear() or
+  // make mLink atomic, but MessageLink does not have to worry about
+  // Clear() racing with Clear(), so it would be inefficient.
   if (mTargetChan) {
     MOZ_ASSERT(mTargetChan->mLink);
-    static_cast<ThreadLink*>(mTargetChan->mLink)->mTargetChan = nullptr;
+    static_cast<ThreadLink*>(mTargetChan->mLink.get())->mTargetChan = nullptr;
   }
   mTargetChan = nullptr;
 }

ipc/glue/MessageLink.h

@@ -42,6 +42,10 @@ class MessageLink {
   explicit MessageLink(MessageChannel* aChan);
   virtual ~MessageLink();
 
+  // This is called immediately before the MessageChannel destroys its
+  // MessageLink. See the implementation in ThreadLink for details.
+  virtual void PrepareToDestroy(){};
+
   // n.b.: These methods all require that the channel monitor is
   // held when they are invoked.
   virtual void SendMessage(mozilla::UniquePtr<Message> msg) = 0;
@@ -102,7 +106,9 @@ class ProcessLink : public MessageLink, public Transport::Listener {
 class ThreadLink : public MessageLink {
  public:
   ThreadLink(MessageChannel* aChan, MessageChannel* aTargetChan);
-  virtual ~ThreadLink();
+  virtual ~ThreadLink() = default;
+
+  virtual void PrepareToDestroy() override;
 
   virtual void SendMessage(mozilla::UniquePtr<Message> msg) override;
   virtual void SendClose() override;