Bug 1476405: Follow-up: Handle nsThread cleanup for threads that never shutdown. r=me
☠☠ backed out by 03c1386d0839 ☠ ☠
authorKris Maglione <maglione.k@gmail.com>
Thu, 26 Jul 2018 16:36:16 -0700
changeset 428683 cb7f7cc326875b2fd28d4a63101b07360a6606fd
parent 428682 6d18a8bd5ee351da1a0cdfaa63f49706a2f95ba3
child 428684 090236db7f10ce9bdda89dc924b29aaa3da2edd9
push id34339
push userdluca@mozilla.com
push dateFri, 27 Jul 2018 10:20:10 +0000
treeherdermozilla-central@87bcafe428a4 [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,36 @@ 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);
+  MOZ_ASSERT(thread->mHasTLSEntry);
+
+  get().UnregisterCurrentThread(*thread, true);
+
+  thread->mHasTLSEntry = false;
+  thread->Release();
 }
 
 // statically allocated instance
 NS_IMETHODIMP_(MozExternalRefCountType)
 nsThreadManager::AddRef()
 {
   return 2;
 }
@@ -231,17 +245,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 +313,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 +371,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 +406,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;