Bug 1476405: Follow-up: Handle nsThread cleanup for threads that never shutdown. r=me
☠☠ backed out by 77b07565d021 ☠ ☠
authorKris Maglione <maglione.k@gmail.com>
Thu, 26 Jul 2018 16:36:16 -0700
changeset 483796 ad1674e9152da31151ab9f9f099f83ca4ff2d832
parent 483795 e0a021b27d2c66d46ba973d66d1360678411da26
child 483797 87bcafe428a4ad6017e59b915581ae00aa863407
child 483829 3ff805a9631e4248d0b720c25e9277a0fd7679d4
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersme
bugs1476405
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1476405: Follow-up: Handle nsThread cleanup for threads that never shutdown. r=me
xpcom/threads/nsThread.cpp
xpcom/threads/nsThread.h
xpcom/threads/nsThreadManager.cpp
xpcom/threads/nsThreadManager.h
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -400,16 +400,23 @@ nsThread::ThreadListMutex()
 
 /* static */ LinkedList<nsThread>&
 nsThread::ThreadList()
 {
   static LinkedList<nsThread> sList;
   return sList;
 }
 
+/* static */ void
+nsThread::ClearThreadList()
+{
+  OffTheBooksMutexAutoLock mal(ThreadListMutex());
+  while (ThreadList().popFirst()) {}
+}
+
 /* static */ nsThreadEnumerator
 nsThread::Enumerate()
 {
   return {};
 }
 
 /*static*/ void
 nsThread::ThreadFunc(void* aArg)
@@ -867,16 +874,23 @@ nsThread::ShutdownInternal(bool aSync)
 }
 
 void
 nsThread::ShutdownComplete(NotNull<nsThreadShutdownContext*> aContext)
 {
   MOZ_ASSERT(mThread);
   MOZ_ASSERT(aContext->mTerminatingThread == this);
 
+  {
+    OffTheBooksMutexAutoLock mal(ThreadListMutex());
+    if (isInList()) {
+      removeFrom(ThreadList());
+    }
+  }
+
   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.
--- a/xpcom/threads/nsThread.h
+++ b/xpcom/threads/nsThread.h
@@ -171,18 +171,21 @@ protected:
   {
     nsIThreadObserver* obs;
     nsThread::GetObserver(&obs);
     return already_AddRefed<nsIThreadObserver>(obs);
   }
 
   struct nsThreadShutdownContext* ShutdownInternal(bool aSync);
 
+  friend class nsThreadManager;
+
   static mozilla::OffTheBooksMutex& ThreadListMutex();
   static mozilla::LinkedList<nsThread>& ThreadList();
+  static void ClearThreadList();
 
   RefPtr<mozilla::SynchronizedEventQueue> mEvents;
   RefPtr<mozilla::ThreadEventTarget> mEventTarget;
 
   // The shutdown contexts for any other threads we've asked to shut down.
   nsTArray<nsAutoPtr<struct nsThreadShutdownContext>> mRequestedShutdownContexts;
   // The shutdown context for ourselves.
   struct nsThreadShutdownContext* mShutdownContext;
@@ -206,16 +209,18 @@ protected:
   bool IsMainThread() const
   {
     return MainThreadFlag(mIsMainThread) == MAIN_THREAD;
   }
 
   // Set to true if this thread creates a JSRuntime.
   bool mCanInvokeJS;
 
+  bool mHasTLSEntry = false;
+
   // Used to track which event is being executed by ProcessNextEvent
   nsCOMPtr<nsIRunnable> mCurrentEvent;
 
   mozilla::TimeStamp mCurrentEventStart;
   mozilla::TimeStamp mNextIdleDeadline;
 
   RefPtr<mozilla::PerformanceCounter> mCurrentPerformanceCounter;
 };
--- a/xpcom/threads/nsThreadManager.cpp
+++ b/xpcom/threads/nsThreadManager.cpp
@@ -89,22 +89,37 @@ AssertIsOnMainThread()
 }
 
 } // mozilla namespace
 
 #endif
 
 typedef nsTArray<NotNull<RefPtr<nsThread>>> nsThreadArray;
 
+static bool sShutdownComplete;
+
 //-----------------------------------------------------------------------------
 
-static void
-ReleaseObject(void* aData)
+/* static */ void
+nsThreadManager::ReleaseThread(void* aData)
 {
-  static_cast<nsISupports*>(aData)->Release();
+  if (sShutdownComplete) {
+    // We've already completed shutdown and released the references to all or
+    // our TLS wrappers. Don't try to release them again.
+    return;
+  }
+
+  auto* thread = static_cast<nsThread*>(aData);
+
+  get().UnregisterCurrentThread(*thread, true);
+
+  if (thread->mHasTLSEntry) {
+    thread->mHasTLSEntry = false;
+    thread->Release();
+  }
 }
 
 // statically allocated instance
 NS_IMETHODIMP_(MozExternalRefCountType)
 nsThreadManager::AddRef()
 {
   return 2;
 }
@@ -231,17 +246,17 @@ nsThreadManager::Init()
   }
 
   if (!gTlsCurrentVirtualThread.init()) {
     return NS_ERROR_UNEXPECTED;
   }
 
   Scheduler::EventLoopActivation::Init();
 
-  if (PR_NewThreadPrivateIndex(&mCurThreadIndex, ReleaseObject) == PR_FAILURE) {
+  if (PR_NewThreadPrivateIndex(&mCurThreadIndex, ReleaseThread) == PR_FAILURE) {
     return NS_ERROR_FAILURE;
   }
 
 
 #ifdef MOZ_CANARY
   const int flags = O_WRONLY | O_APPEND | O_CREAT | O_NONBLOCK;
   const mode_t mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
   char* env_var_flag = getenv("MOZ_KILL_CANARIES");
@@ -299,42 +314,44 @@ nsThreadManager::Shutdown()
   // between when NewThread started, and when the thread finished initializing
   // and registering with ThreadManager.
   //
   mInitialized = false;
 
   // Empty the main thread event queue before we begin shutting down threads.
   NS_ProcessPendingEvents(mMainThread);
 
-  // We gather the threads from the hashtable into a list, so that we avoid
-  // holding the hashtable lock while calling nsIThread::Shutdown.
-  nsThreadArray threads;
   {
-    OffTheBooksMutexAutoLock lock(mLock);
-    for (auto iter = mThreadsByPRThread.Iter(); !iter.Done(); iter.Next()) {
-      RefPtr<nsThread>& thread = iter.Data();
-      threads.AppendElement(WrapNotNull(thread));
-      iter.Remove();
+    // We gather the threads from the hashtable into a list, so that we avoid
+    // holding the hashtable lock while calling nsIThread::Shutdown.
+    nsThreadArray threads;
+    {
+      OffTheBooksMutexAutoLock lock(mLock);
+      for (auto iter = mThreadsByPRThread.Iter(); !iter.Done(); iter.Next()) {
+        RefPtr<nsThread>& thread = iter.Data();
+        threads.AppendElement(WrapNotNull(thread));
+        iter.Remove();
+      }
     }
-  }
 
-  // It's tempting to walk the list of threads here and tell them each to stop
-  // accepting new events, but that could lead to badness if one of those
-  // threads is stuck waiting for a response from another thread.  To do it
-  // right, we'd need some way to interrupt the threads.
-  //
-  // Instead, we process events on the current thread while waiting for threads
-  // to shutdown.  This means that we have to preserve a mostly functioning
-  // world until such time as the threads exit.
+    // It's tempting to walk the list of threads here and tell them each to stop
+    // accepting new events, but that could lead to badness if one of those
+    // threads is stuck waiting for a response from another thread.  To do it
+    // right, we'd need some way to interrupt the threads.
+    //
+    // Instead, we process events on the current thread while waiting for threads
+    // to shutdown.  This means that we have to preserve a mostly functioning
+    // world until such time as the threads exit.
 
-  // Shutdown all threads that require it (join with threads that we created).
-  for (uint32_t i = 0; i < threads.Length(); ++i) {
-    NotNull<nsThread*> thread = threads[i];
-    if (thread->ShutdownRequired()) {
-      thread->Shutdown();
+    // Shutdown all threads that require it (join with threads that we created).
+    for (uint32_t i = 0; i < threads.Length(); ++i) {
+      NotNull<nsThread*> thread = threads[i];
+      if (thread->ShutdownRequired()) {
+        thread->Shutdown();
+      }
     }
   }
 
   // NB: It's possible that there are events in the queue that want to *start*
   // an asynchronous shutdown. But we have already shutdown the threads above,
   // so there's no need to worry about them. We only have to wait for all
   // in-flight asynchronous thread shutdowns to complete.
   mMainThread->WaitForAllAsynchronousShutdowns();
@@ -355,16 +372,34 @@ nsThreadManager::Shutdown()
   // have been processed.
   mMainThread->SetObserver(nullptr);
 
   // Release main thread object.
   mMainThread = nullptr;
 
   // Remove the TLS entry for the main thread.
   PR_SetThreadPrivate(mCurThreadIndex, nullptr);
+
+  {
+    // Cleanup the last references to any threads which haven't shut down yet.
+    nsTArray<RefPtr<nsThread>> threads;
+    for (auto* thread : nsThread::Enumerate()) {
+      if (thread->mHasTLSEntry) {
+        threads.AppendElement(dont_AddRef(thread));
+        thread->mHasTLSEntry = false;
+      }
+    }
+  }
+
+  // xpcshell tests sometimes leak the main thread. They don't enable leak
+  // checking, so that doesn't cause the test to fail, but leaving the entry in
+  // the thread list triggers an assertion, which does.
+  nsThread::ClearThreadList();
+
+  sShutdownComplete = true;
 }
 
 void
 nsThreadManager::RegisterCurrentThread(nsThread& aThread)
 {
   MOZ_ASSERT(aThread.GetPRThread() == PR_GetCurrentThread(), "bad aThread");
 
   OffTheBooksMutexAutoLock lock(mLock);
@@ -372,31 +407,38 @@ nsThreadManager::RegisterCurrentThread(n
   ++mCurrentNumberOfThreads;
   if (mCurrentNumberOfThreads > mHighestNumberOfThreads) {
     mHighestNumberOfThreads = mCurrentNumberOfThreads;
   }
 
   mThreadsByPRThread.Put(aThread.GetPRThread(), &aThread);  // XXX check OOM?
 
   aThread.AddRef();  // for TLS entry
+  aThread.mHasTLSEntry = true;
   PR_SetThreadPrivate(mCurThreadIndex, &aThread);
 }
 
 void
-nsThreadManager::UnregisterCurrentThread(nsThread& aThread)
+nsThreadManager::UnregisterCurrentThread(nsThread& aThread, bool aIfExists)
 {
   MOZ_ASSERT(aThread.GetPRThread() == PR_GetCurrentThread(), "bad aThread");
 
-  OffTheBooksMutexAutoLock lock(mLock);
+  {
+    OffTheBooksMutexAutoLock lock(mLock);
 
-  --mCurrentNumberOfThreads;
-  mThreadsByPRThread.Remove(aThread.GetPRThread());
+    if (aIfExists && !mThreadsByPRThread.GetWeak(aThread.GetPRThread())) {
+      return;
+    }
+
+    --mCurrentNumberOfThreads;
+    mThreadsByPRThread.Remove(aThread.GetPRThread());
+  }
 
   PR_SetThreadPrivate(mCurThreadIndex, nullptr);
-  // Ref-count balanced via ReleaseObject
+  // Ref-count balanced via ReleaseThread
 }
 
 nsThread*
 nsThreadManager::CreateCurrentThread(SynchronizedEventQueue* aQueue,
                                      nsThread::MainThreadFlag aMainThread)
 {
   // Make sure we don't have an nsThread yet.
   MOZ_ASSERT(!PR_GetThreadPrivate(mCurThreadIndex));
--- a/xpcom/threads/nsThreadManager.h
+++ b/xpcom/threads/nsThreadManager.h
@@ -31,17 +31,17 @@ public:
   void Shutdown();
 
   // Called by nsThread to inform the ThreadManager it exists.  This method
   // must be called when the given thread is the current thread.
   void RegisterCurrentThread(nsThread& aThread);
 
   // Called by nsThread to inform the ThreadManager it is going away.  This
   // method must be called when the given thread is the current thread.
-  void UnregisterCurrentThread(nsThread& aThread);
+  void UnregisterCurrentThread(nsThread& aThread, bool aIfExists = false);
 
   // Returns the current thread.  Returns null if OOM or if ThreadManager isn't
   // initialized.  Creates the nsThread if one does not exist yet.
   nsThread* GetCurrentThread();
 
   // Returns true iff the currently running thread has an nsThread associated
   // with it (ie; whether this is a thread that we can dispatch runnables to).
   bool IsNSThread() const;
@@ -80,16 +80,18 @@ private:
     , mHighestNumberOfThreads(1)
   {
   }
 
   nsresult
   SpinEventLoopUntilInternal(nsINestedEventLoopCondition* aCondition,
                              bool aCheckingShutdown);
 
+  static void ReleaseThread(void* aData);
+
   nsRefPtrHashtable<nsPtrHashKey<PRThread>, nsThread> mThreadsByPRThread;
   unsigned            mCurThreadIndex;  // thread-local-storage index
   RefPtr<nsThread>  mMainThread;
   PRThread*         mMainPRThread;
   mozilla::OffTheBooksMutex mLock;  // protects tables
   mozilla::Atomic<bool,
                   mozilla::SequentiallyConsistent,
                   mozilla::recordreplay::Behavior::DontPreserve> mInitialized;