bug 1602646 move assertion that mThread is set in ShutdownInternal() r?froydnj draft
authorKarl Tomlinson <karlt+@karlt.net>
Tue, 10 Dec 2019 18:20:48 +1300
changeset 2526068 c46126080284ed41b950d7b143fe58d81c1ffb76
parent 2526067 16e195cb4d8a8f9805e192e5396b4248db29b61c
child 2526069 a69aa81239b5d08e7044b4183835ada4a8c45533
push id462929
push userktomlinson@mozilla.com
push dateTue, 10 Dec 2019 05:40:13 +0000
treeherdertry@09b411cd75da [default view] [failures only]
reviewersfroydnj
bugs1602646
milestone73.0a1
bug 1602646 move assertion that mThread is set in ShutdownInternal() r?froydnj DecodePoolImpl, at least can have both Shutdown() and AsyncShutdown() called on a single thread, and the latter call can occur at the same time as the thread is exiting its event loop. https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/image/DecodePool.cpp#91 mShutdownRequired is already tested to avoid multiple shutdown runnables, so callers need not test mThread, which is racy.
xpcom/threads/nsThread.cpp
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -812,39 +812,36 @@ nsThread::SetNameForWakeupTelemetry(cons
 #endif
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsThread::AsyncShutdown() {
   LOG(("THRD(%p) async shutdown\n", this));
 
-  // XXX If we make this warn, then we hit that warning at xpcom shutdown while
-  //     shutting down a thread in a thread pool.  That happens b/c the thread
-  //     in the thread pool is already shutdown by the thread manager.
-  if (!mThread) {
-    return NS_OK;
-  }
-
-  return !!ShutdownInternal(/* aSync = */ false) ? NS_OK : NS_ERROR_UNEXPECTED;
+  ShutdownInternal(/* aSync = */ false);
+  return NS_OK;
 }
 
 nsThreadShutdownContext* nsThread::ShutdownInternal(bool aSync) {
   MOZ_ASSERT(mEvents);
   MOZ_ASSERT(mEventTarget);
-  MOZ_ASSERT(mThread);
   MOZ_ASSERT(mThread != PR_GetCurrentThread());
   if (NS_WARN_IF(mThread == PR_GetCurrentThread())) {
     return nullptr;
   }
 
-  // Prevent multiple calls to this method
+  // Prevent multiple calls to this method.
+  // This can happen, for example, at xpcom shutdown while shutting down a
+  // thread in a thread pool after the thread in the thread pool is already
+  // shutdown by the thread manager.
   if (!mShutdownRequired.compareExchange(true, false)) {
     return nullptr;
   }
+  MOZ_ASSERT(mThread);
 
   MaybeRemoveFromThreadList();
 
   NotNull<nsThread*> currentThread =
       WrapNotNull(nsThreadManager::get().GetCurrentThread());
 
   MOZ_DIAGNOSTIC_ASSERT(currentThread->EventQueue(),
                         "Shutdown() may only be called from an XPCOM thread");
@@ -902,26 +899,21 @@ void nsThread::WaitForAllAsynchronousShu
   SpinEventLoopUntil<ProcessFailureBehavior::IgnoreAndContinue>(
       [&]() { return mRequestedShutdownContexts.IsEmpty(); }, this);
 }
 
 NS_IMETHODIMP
 nsThread::Shutdown() {
   LOG(("THRD(%p) sync shutdown\n", this));
 
-  // XXX If we make this warn, then we hit that warning at xpcom shutdown while
-  //     shutting down a thread in a thread pool.  That happens b/c the thread
-  //     in the thread pool is already shutdown by the thread manager.
-  if (!mThread) {
-    return NS_OK;
+  nsThreadShutdownContext* maybeContext = ShutdownInternal(/* aSync = */ true);
+  if (!maybeContext) {
+    return NS_OK;  // The thread has already shut down.
   }
 
-  nsThreadShutdownContext* maybeContext = ShutdownInternal(/* aSync = */ true);
-  if (!maybeContext) return NS_ERROR_UNEXPECTED;
-
   NotNull<nsThreadShutdownContext*> context = WrapNotNull(maybeContext);
 
   // Process events on the current thread until we receive a shutdown ACK.
   // Allows waiting; ensure no locks are held that would deadlock us!
   SpinEventLoopUntil([&, context]() { return !context->mAwaitingShutdownAck; },
                      context->mJoiningThread);
 
   ShutdownComplete(context);