Bug 1599922 clear PRThread references before the PRThread is deleted r?froydnj draft
authorKarl Tomlinson <karlt+@karlt.net>
Wed, 04 Dec 2019 09:25:48 +1300
changeset 2526066 53eb40e3c0b80995e4f0fbaabdd1632861826cbe
parent 2513214 3c08edf74d039af79f9daad8ff5b57ffb64fdab6
child 2526067 16e195cb4d8a8f9805e192e5396b4248db29b61c
push id462929
push userktomlinson@mozilla.com
push dateTue, 10 Dec 2019 05:40:13 +0000
treeherdertry@09b411cd75da [default view] [failures only]
reviewersfroydnj
bugs1599922
milestone73.0a1
Bug 1599922 clear PRThread references before the PRThread is deleted r?froydnj Virtual thread references are used for IsOnCurrentThread(), which would spuriously return true when the dangling pointer happened to match that of a new PRThread. Differential Revision: https://phabricator.services.mozilla.com/D55765
xpcom/threads/ThreadEventTarget.cpp
xpcom/threads/ThreadEventTarget.h
xpcom/threads/nsIEventTarget.idl
xpcom/threads/nsThread.cpp
--- a/xpcom/threads/ThreadEventTarget.cpp
+++ b/xpcom/threads/ThreadEventTarget.cpp
@@ -91,16 +91,18 @@ ThreadEventTarget::ThreadEventTarget(Thr
     : mSink(aSink), mIsMainThread(aIsMainThread) {
   mVirtualThread = GetCurrentVirtualThread();
 }
 
 void ThreadEventTarget::SetCurrentThread() {
   mVirtualThread = GetCurrentVirtualThread();
 }
 
+void ThreadEventTarget::ClearCurrentThread() { mVirtualThread = nullptr; }
+
 NS_IMPL_ISUPPORTS(ThreadEventTarget, nsIEventTarget, nsISerialEventTarget)
 
 NS_IMETHODIMP
 ThreadEventTarget::DispatchFromScript(nsIRunnable* aRunnable, uint32_t aFlags) {
   return Dispatch(do_AddRef(aRunnable), aFlags);
 }
 
 NS_IMETHODIMP
--- a/xpcom/threads/ThreadEventTarget.h
+++ b/xpcom/threads/ThreadEventTarget.h
@@ -26,16 +26,18 @@ class ThreadEventTarget final : public n
   // Disconnects the target so that it can no longer post events.
   void Disconnect(const MutexAutoLock& aProofOfLock) {
     mSink->Disconnect(aProofOfLock);
   }
 
   // Sets the thread for which IsOnCurrentThread returns true to the current
   // thread.
   void SetCurrentThread();
+  // Call ClearCurrentThread() before the PRThread is deleted on thread join.
+  void ClearCurrentThread();
 
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
     size_t n = 0;
     if (mSink) {
       n += mSink->SizeOfIncludingThis(aMallocSizeOf);
     }
     return aMallocSizeOf(this) + n;
   }
--- a/xpcom/threads/nsIEventTarget.idl
+++ b/xpcom/threads/nsIEventTarget.idl
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsISupports.idl"
 #include "nsIRunnable.idl"
 %{C++
 #include "nsCOMPtr.h"
 #include "mozilla/AlreadyAddRefed.h"
+#include "mozilla/Atomics.h"
 %}
 
 native alreadyAddRefed_nsIRunnable(already_AddRefed<nsIRunnable>);
 
 [builtinclass, scriptable, uuid(a03b8b63-af8b-4164-b0e5-c41e8b2b7cfa)]
 interface nsIEventTarget : nsISupports
 {
   /* until we can get rid of all uses, keep the non-alreadyAddRefed<> version */
@@ -89,17 +90,17 @@ interface nsIEventTarget : nsISupports
    */
   %{C++
 public:
   // Infallible. Defined in nsThreadUtils.cpp. Delegates to
   // IsOnCurrentThreadInfallible when mVirtualThread is null.
   bool IsOnCurrentThread();
 
 protected:
-  PRThread* mVirtualThread;
+  mozilla::Atomic<PRThread*> mVirtualThread;
 
   nsIEventTarget() : mVirtualThread(nullptr) {}
   %}
   // Note that this method is protected.  We define it through IDL, rather than
   // in a %{C++ block, to ensure that the correct method indices are recorded
   // for XPConnect purposes.
   [noscript,notxpcom] boolean isOnCurrentThreadInfallible();
   %{C++
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -236,25 +236,27 @@ class nsThreadStartupEvent final : publi
 };
 //-----------------------------------------------------------------------------
 
 struct nsThreadShutdownContext {
   nsThreadShutdownContext(NotNull<nsThread*> aTerminatingThread,
                           NotNull<nsThread*> aJoiningThread,
                           bool aAwaitingShutdownAck)
       : mTerminatingThread(aTerminatingThread),
+        mTerminatingPRThread(aTerminatingThread->GetPRThread()),
         mJoiningThread(aJoiningThread),
         mAwaitingShutdownAck(aAwaitingShutdownAck),
         mIsMainThreadJoining(NS_IsMainThread()) {
     MOZ_COUNT_CTOR(nsThreadShutdownContext);
   }
   ~nsThreadShutdownContext() { MOZ_COUNT_DTOR(nsThreadShutdownContext); }
 
   // NB: This will be the last reference.
   NotNull<RefPtr<nsThread>> mTerminatingThread;
+  PRThread* const mTerminatingPRThread;
   NotNull<nsThread*> MOZ_UNSAFE_REF(
       "Thread manager is holding reference to joining thread") mJoiningThread;
   bool mAwaitingShutdownAck;
   bool mIsMainThreadJoining;
 };
 
 // This event is responsible for notifying nsThread::Shutdown that it is time
 // to call PR_JoinThread. It implements nsICancelableRunnable so that it can
@@ -499,16 +501,20 @@ void nsThread::ThreadFunc(void* aArg) {
 
   // Release any observer of the thread here.
   self->SetObserver(nullptr);
 
 #ifdef MOZ_TASK_TRACER
   FreeTraceInfo();
 #endif
 
+  // The PRThread will be deleted in PR_JoinThread(), so clear references.
+  self->mThread = nullptr;
+  self->mVirtualThread = nullptr;
+  self->mEventTarget->ClearCurrentThread();
   NS_RELEASE(self);
 }
 
 void nsThread::InitCommon() {
   mThreadId = uint32_t(PlatformThread::CurrentId());
 
   {
 #if defined(XP_LINUX)
@@ -759,18 +765,20 @@ nsThread::IsOnCurrentThread(bool* aResul
     return mEventTarget->IsOnCurrentThread(aResult);
   }
   *aResult = GetCurrentVirtualThread() == mVirtualThread;
   return NS_OK;
 }
 
 NS_IMETHODIMP_(bool)
 nsThread::IsOnCurrentThreadInfallible() {
-  // Rely on mVirtualThread being correct.
-  MOZ_CRASH("IsOnCurrentThreadInfallible should never be called on nsIThread");
+  // This method is only going to be called if `mVirtualThread` is null, which
+  // only happens when the thread has exited the event loop.  Therefore, when
+  // we are called, we can never be on this thread.
+  return false;
 }
 
 //-----------------------------------------------------------------------------
 // nsIThread
 
 NS_IMETHODIMP
 nsThread::GetPRThread(PRThread** aResult) {
   *aResult = mThread;
@@ -861,32 +869,30 @@ nsThreadShutdownContext* nsThread::Shutd
   // task, but that's okay because we process pending events in ThreadFunc
   // after setting mShutdownContext just before exiting.
   return context;
 }
 
 void nsThread::ShutdownComplete(NotNull<nsThreadShutdownContext*> aContext) {
   MOZ_ASSERT(mEvents);
   MOZ_ASSERT(mEventTarget);
-  MOZ_ASSERT(mThread);
   MOZ_ASSERT(aContext->mTerminatingThread == this);
 
   MaybeRemoveFromThreadList();
 
   if (aContext->mAwaitingShutdownAck) {
     // We're in a synchronous shutdown, so tell whatever is up the stack that
     // we're done and unwind the stack so it can call us again.
     aContext->mAwaitingShutdownAck = false;
     return;
   }
 
   // Now, it should be safe to join without fear of dead-locking.
-
-  PR_JoinThread(mThread);
-  mThread = nullptr;
+  PR_JoinThread(aContext->mTerminatingPRThread);
+  MOZ_ASSERT(!mThread);
 
 #ifdef DEBUG
   nsCOMPtr<nsIThreadObserver> obs = mEvents->GetObserver();
   MOZ_ASSERT(!obs, "Should have been cleared at shutdown!");
 #endif
 
   // Delete aContext.
   // aContext might not be in mRequestedShutdownContexts if it belongs to a