Bug 1479035: Part 2 - Get rid of PRThread to nsThread map. r=erahm

These maps hold strong references which complicate nsThread lifetime handling
considerably, and only have a couple of fringe uses. We have a linked list of
active threads that the thread manager can use for its internal enumeration
purposes, and the external uses are easily done away with, so there doesn't
seem to be much reason to keep the map around.

MozReview-Commit-ID: x7dsj6C4x8

--HG--
extra : source : 5f870621361012ba459943212d8c68a9ff81cb16
extra : intermediate-source : 89a0c0874d400dd324df6fc3627c0c47d130df19
extra : histedit_source : bbd7900e3d754bde925a411c10aa30a1d6e22edd

dom/workers/WorkerPrivate.cpp

@@ -5354,22 +5354,13 @@ WorkerPrivate::CreateDebuggerGlobalScope(JSContext* aCx)
 bool
 WorkerPrivate::IsOnWorkerThread() const
 {
-  // This is much more complicated than it needs to be but we can't use mThread
-  // because it must be protected by mMutex and sometimes this method is called
-  // when mMutex is already locked. This method should always work.
+  // We can't use mThread because it must be protected by mMutex and sometimes
+  // this method is called when mMutex is already locked. This method should
+  // always work.
   MOZ_ASSERT(mPRThread,
              "AssertIsOnWorkerThread() called before a thread was assigned!");
 
-  nsCOMPtr<nsIThread> thread;
-  nsresult rv =
-    nsThreadManager::get().GetThreadFromPRThread(mPRThread,
-                                                 getter_AddRefs(thread));
-  MOZ_ASSERT(NS_SUCCEEDED(rv));
-  MOZ_ASSERT(thread);
-
-  bool current;
-  rv = thread->IsOnCurrentThread(&current);
-  return NS_SUCCEEDED(rv) && current;
+  return mPRThread == PR_GetCurrentThread();
 }
 
 #ifdef DEBUG

storage/test/gtest/storage_test_harness.h

@@ -225,7 +225,7 @@ PRThread *watched_thread = nullptr;
  * When the thread a mutex is invoked on isn't watched_thread we save it to this
  * variable.
  */
-PRThread *last_non_watched_thread = nullptr;
+nsIThread* last_non_watched_thread = nullptr;
 
 /**
  * Set a flag if the mutex is used on the thread we are watching, but always
@@ -233,11 +233,10 @@ PRThread *last_non_watched_thread = nullptr;
  */
 extern "C" void wrapped_MutexEnter(sqlite3_mutex *mutex)
 {
-  PRThread *curThread = ::PR_GetCurrentThread();
-  if (curThread == watched_thread)
+  if (PR_GetCurrentThread() == watched_thread)
     mutex_used_on_watched_thread = true;
   else
-    last_non_watched_thread = curThread;
+    last_non_watched_thread = NS_GetCurrentThread();
   orig_mutex_methods.xMutexEnter(mutex);
 }
 
@@ -351,19 +350,13 @@ get_conn_async_thread(mozIStorageConnection *db)
   blocking_async_execute(stmt);
   stmt->Finalize();
 
-  nsCOMPtr<nsIThreadManager> threadMan =
-    do_GetService("@mozilla.org/thread-manager;1");
-  nsCOMPtr<nsIThread> asyncThread;
-  threadMan->GetThreadFromPRThread(last_non_watched_thread,
-                                   getter_AddRefs(asyncThread));
+  nsCOMPtr<nsIThread> asyncThread = last_non_watched_thread;
 
   // Additionally, check that the thread we get as the background thread is the
   // same one as the one we report from getInterface.
   nsCOMPtr<nsIEventTarget> target = do_GetInterface(db);
   nsCOMPtr<nsIThread> allegedAsyncThread = do_QueryInterface(target);
-  PRThread *allegedPRThread;
-  (void)allegedAsyncThread->GetPRThread(&allegedPRThread);
-  do_check_eq(allegedPRThread, last_non_watched_thread);
+  do_check_eq(allegedAsyncThread, asyncThread);
   return asyncThread.forget();
 }
 

xpcom/threads/nsIThreadManager.idl

@@ -84,19 +84,6 @@ interface nsIThreadManager : nsISupports
    */
   [noscript] nsIThread newNamedThread(in ACString name, [optional] in unsigned long stackSize);
 
-  /**
-   * Get the nsIThread object (if any) corresponding to the given PRThread.
-   * This method returns null if there is no corresponding nsIThread.
-   *
-   * @param prthread
-   *   The PRThread of the nsIThread being requested.
-   *
-   * @returns
-   *   The nsIThread object corresponding to the given PRThread or null if no
-   *   such nsIThread exists.
-   */
-  [noscript] nsIThread getThreadFromPRThread(in PRThread prthread);
-
   /**
    * Get the main thread.
    */

xpcom/threads/nsThread.cpp

@@ -115,6 +115,9 @@ NS_DECL_CI_INTERFACE_GETTER(nsThread)
 
 Array<char, nsThread::kRunnableNameBufSize> nsThread::sMainThreadRunnableName;
 
+uint32_t nsThread::sActiveThreads;
+uint32_t nsThread::sMaxActiveThreads;
+
 //-----------------------------------------------------------------------------
 // Because we do not have our own nsIFactory, we have to implement nsIClassInfo
 // somewhat manually.
@@ -416,6 +419,40 @@ nsThread::Enumerate()
   return {};
 }
 
+/* static */ uint32_t
+nsThread::MaxActiveThreads()
+{
+  OffTheBooksMutexAutoLock mal(ThreadListMutex());
+  return sMaxActiveThreads;
+}
+
+void
+nsThread::AddToThreadList()
+{
+  OffTheBooksMutexAutoLock mal(ThreadListMutex());
+  MOZ_ASSERT(!isInList());
+
+  sActiveThreads++;
+  sMaxActiveThreads = std::max(sActiveThreads, sMaxActiveThreads);
+
+  ThreadList().insertBack(this);
+}
+
+void
+nsThread::MaybeRemoveFromThreadList()
+{
+  // We shouldn't need to lock before checking isInList at this point. We're
+  // destroying the last reference to this object, so there's no way for anyone
+  // else to remove it in the middle of our check. And the not-in-list state is
+  // determined the element's next and previous members pointing to itself, so a
+  // non-atomic update to an adjacent member won't affect the outcome either.
+  if (isInList()) {
+    OffTheBooksMutexAutoLock mal(ThreadListMutex());
+    sActiveThreads--;
+    removeFrom(ThreadList());
+  }
+}
+
 /*static*/ void
 nsThread::ThreadFunc(void* aArg)
 {
@@ -588,8 +625,7 @@ nsThread::InitCommon()
 #endif
   }
 
-  OffTheBooksMutexAutoLock mal(ThreadListMutex());
-  ThreadList().insertBack(this);
+  AddToThreadList();
 }
 
 //-----------------------------------------------------------------------------
@@ -645,15 +681,7 @@ nsThread::~nsThread()
   NS_ASSERTION(mRequestedShutdownContexts.IsEmpty(),
                "shouldn't be waiting on other threads to shutdown");
 
-  // We shouldn't need to lock before checking isInList at this point. We're
-  // destroying the last reference to this object, so there's no way for anyone
-  // else to remove it in the middle of our check. And the not-in-list state is
-  // determined the element's next and previous members pointing to itself, so a
-  // non-atomic update to an adjacent member won't affect the outcome either.
-  if (isInList()) {
-    OffTheBooksMutexAutoLock mal(ThreadListMutex());
-    removeFrom(ThreadList());
-  }
+  MaybeRemoveFromThreadList();
 
 #ifdef DEBUG
   // We deliberately leak these so they can be tracked by the leak checker.
@@ -821,12 +849,7 @@ nsThread::ShutdownInternal(bool aSync)
     return nullptr;
   }
 
-  {
-    OffTheBooksMutexAutoLock mal(ThreadListMutex());
-    if (isInList()) {
-      removeFrom(ThreadList());
-    }
-  }
+  MaybeRemoveFromThreadList();
 
   NotNull<nsThread*> currentThread =
     WrapNotNull(nsThreadManager::get().GetCurrentThread());
@@ -856,12 +879,7 @@ nsThread::ShutdownComplete(NotNull<nsThreadShutdownContext*> aContext)
   MOZ_ASSERT(mThread);
   MOZ_ASSERT(aContext->mTerminatingThread == this);
 
-  {
-    OffTheBooksMutexAutoLock mal(ThreadListMutex());
-    if (isInList()) {
-      removeFrom(ThreadList());
-    }
-  }
+  MaybeRemoveFromThreadList();
 
   if (aContext->mAwaitingShutdownAck) {
     // We're in a synchronous shutdown, so tell whatever is up the stack that

xpcom/threads/nsThread.h

@@ -150,6 +150,8 @@ public:
 
   static nsThreadEnumerator Enumerate();
 
+  static uint32_t MaxActiveThreads();
+
 private:
   void DoMainThreadSpecificProcessing(bool aReallyWait);
 
@@ -178,6 +180,15 @@ protected:
   static mozilla::LinkedList<nsThread>& ThreadList();
   static void ClearThreadList();
 
+  // The current number of active threads.
+  static uint32_t sActiveThreads;
+  // The maximum current number of active threads we've had in this session.
+  static uint32_t sMaxActiveThreads;
+
+  void AddToThreadList();
+  void MaybeRemoveFromThreadList();
+
+
   // Whether or not these members have a value determines whether the nsThread
   // is treated as a full XPCOM thread or as a thin wrapper.
   //

xpcom/threads/nsThreadManager.cpp

@@ -109,8 +109,6 @@ nsThreadManager::ReleaseThread(void* aData)
 
   auto* thread = static_cast<nsThread*>(aData);
 
-  get().UnregisterCurrentThread(*thread, true);
-
   if (thread->mHasTLSEntry) {
     thread->mHasTLSEntry = false;
     thread->Release();
@@ -321,14 +319,11 @@ nsThreadManager::Shutdown()
 
   {
     // 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();
+    // holding the enumerator lock while calling nsIThread::Shutdown.
+    nsTArray<RefPtr<nsThread>> threadsToShutdown;
+    for (auto* thread : nsThread::Enumerate()) {
+      if (thread->ShutdownRequired()) {
+        threadsToShutdown.AppendElement(thread);
       }
     }
 
@@ -342,13 +337,10 @@ nsThreadManager::Shutdown()
     // 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()) {
+    for (auto& thread : threadsToShutdown) {
       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,
@@ -361,12 +353,6 @@ nsThreadManager::Shutdown()
 
   // There are no more background threads at this point.
 
-  // Clear the table of threads.
-  {
-    OffTheBooksMutexAutoLock lock(mLock);
-    mThreadsByPRThread.Clear();
-  }
-
   // Normally thread shutdown clears the observer for the thread, but since the
   // main thread is special we do it manually here after we're sure all events
   // have been processed.
@@ -402,36 +388,16 @@ nsThreadManager::RegisterCurrentThread(nsThread& aThread)
 {
   MOZ_ASSERT(aThread.GetPRThread() == PR_GetCurrentThread(), "bad aThread");
 
-  OffTheBooksMutexAutoLock lock(mLock);
-
-  ++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, bool aIfExists)
+nsThreadManager::UnregisterCurrentThread(nsThread& aThread)
 {
-  {
-    OffTheBooksMutexAutoLock lock(mLock);
-
-    if (aIfExists && !mThreadsByPRThread.GetWeak(aThread.GetPRThread())) {
-      return;
-    }
-
   MOZ_ASSERT(aThread.GetPRThread() == PR_GetCurrentThread(), "bad aThread");
 
-    --mCurrentNumberOfThreads;
-    mThreadsByPRThread.Remove(aThread.GetPRThread());
-  }
-
   PR_SetThreadPrivate(mCurThreadIndex, nullptr);
   // Ref-count balanced via ReleaseThread
 }
@@ -537,27 +503,6 @@ nsThreadManager::NewNamedThread(const nsACString& aName,
   return NS_OK;
 }
 
-NS_IMETHODIMP
-nsThreadManager::GetThreadFromPRThread(PRThread* aThread, nsIThread** aResult)
-{
-  // Keep this functioning during Shutdown
-  if (NS_WARN_IF(!mMainThread)) {
-    return NS_ERROR_NOT_INITIALIZED;
-  }
-  if (NS_WARN_IF(!aThread)) {
-    return NS_ERROR_INVALID_ARG;
-  }
-
-  RefPtr<nsThread> temp;
-  {
-    OffTheBooksMutexAutoLock lock(mLock);
-    mThreadsByPRThread.Get(aThread, getter_AddRefs(temp));
-  }
-
-  NS_IF_ADDREF(*aResult = temp);
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 nsThreadManager::GetMainThread(nsIThread** aResult)
 {
@@ -660,8 +605,7 @@ nsThreadManager::GetSystemGroupEventTarget(nsIEventTarget** aTarget)
 uint32_t
 nsThreadManager::GetHighestNumberOfThreads()
 {
-  OffTheBooksMutexAutoLock lock(mLock);
-  return mHighestNumberOfThreads;
+  return nsThread::MaxActiveThreads();
 }
 
 NS_IMETHODIMP

xpcom/threads/nsThreadManager.h

@@ -9,7 +9,6 @@
 
 #include "mozilla/Mutex.h"
 #include "nsIThreadManager.h"
-#include "nsRefPtrHashtable.h"
 #include "nsThread.h"
 
 class nsIRunnable;
@@ -36,7 +35,7 @@ public:
 
   // 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, bool aIfExists = false);
+  void UnregisterCurrentThread(nsThread& aThread);
 
   // Returns the current thread.  Returns null if OOM or if ThreadManager isn't
   // initialized.  Creates the nsThread if one does not exist yet.
@@ -74,10 +73,7 @@ private:
   nsThreadManager()
     : mCurThreadIndex(0)
     , mMainPRThread(nullptr)
-    , mLock("nsThreadManager.mLock")
     , mInitialized(false)
-    , mCurrentNumberOfThreads(1)
-    , mHighestNumberOfThreads(1)
   {
   }
 
@@ -87,19 +83,12 @@ private:
 
   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;
-
-  // The current number of threads
-  uint32_t            mCurrentNumberOfThreads;
-  // The highest number of threads encountered so far during the session
-  uint32_t            mHighestNumberOfThreads;
 };
 
 #define NS_THREADMANAGER_CID                       \