From 0442082a5eb37ebb7865b3dce892834ab2d81c05 Mon Sep 17 00:00:00 2001 From: Andrew McCreight Date: Tue, 16 Jun 2020 22:33:55 +0000 Subject: [PATCH] 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 | 14 ++++++++------ ipc/glue/MessageChannel.h | 2 +- ipc/glue/MessageLink.cpp | 32 +++++++++++++++++++++++++++++--- ipc/glue/MessageLink.h | 8 +++++++- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp index 7dfd84db7fb7..6fb963d35ff5 100644 --- a/ipc/glue/MessageChannel.cpp +++ b/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 aTransport, mWorkerLoop->AddDestructionObserver(this); mListener->OnIPCChannelOpened(); - ProcessLink* link = new ProcessLink(this); + auto link = MakeUnique(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(this, aTargetChan); mSide = aSide; } diff --git a/ipc/glue/MessageChannel.h b/ipc/glue/MessageChannel.h index 4a510fc0af22..4257d7725e25 100644 --- a/ipc/glue/MessageChannel.h +++ b/ipc/glue/MessageChannel.h @@ -624,7 +624,7 @@ class MessageChannel : HasResultCodes, MessageLoop::DestructionObserver { RefPtr mMonitor; Side mSide; bool mIsCrossProcess; - MessageLink* mLink; + UniquePtr mLink; MessageLoop* mWorkerLoop; // thread where work is done RefPtr mChannelErrorTask; // NotifyMaybeChannelError runnable diff --git a/ipc/glue/MessageLink.cpp b/ipc/glue/MessageLink.cpp index 1db13020b9ea..07456785d7a0 100644 --- a/ipc/glue/MessageLink.cpp +++ b/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(mTargetChan->mLink)->mTargetChan = nullptr; + static_cast(mTargetChan->mLink.get())->mTargetChan = nullptr; } mTargetChan = nullptr; } diff --git a/ipc/glue/MessageLink.h b/ipc/glue/MessageLink.h index a6af59ee8959..883e05a5e6ca 100644 --- a/ipc/glue/MessageLink.h +++ b/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 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 msg) override; virtual void SendClose() override;