Bug 1503725 - Do not deallocate nsThreadShutdownContext when leaking thread. r=erahm, a=RyanVM
authorValentin Gosu <valentin.gosu@gmail.com>
Fri, 02 Nov 2018 17:38:37 -0400
changeset 501038 8016d0d1a391f1327a0375fb6d4f20228b2784aa
parent 501037 10cf5976e46f3fd3a0895873b6f73bf08ce7acef
child 501039 9aacd377517c9d54df39a717b6f474e3666389ef
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm, RyanVM
bugs1503725
milestone64.0
Bug 1503725 - Do not deallocate nsThreadShutdownContext when leaking thread. r=erahm, a=RyanVM Sometimes when we call ShutdownWithTimeout on a thread pool, the unresponsive thread that we leak will actually complete before the main thread is done. In that case, the thread will dereference the thread shutdown context, so we must intentionally leak it too. Differential Revision: https://phabricator.services.mozilla.com/D10645
xpcom/tests/gtest/TestThreadPool.cpp
xpcom/threads/nsThread.cpp
xpcom/threads/nsThread.h
xpcom/threads/nsThreadPool.cpp
--- a/xpcom/tests/gtest/TestThreadPool.cpp
+++ b/xpcom/tests/gtest/TestThreadPool.cpp
@@ -161,8 +161,44 @@ TEST(ThreadPool, ShutdownWithTimeout)
           PR_Sleep(PR_MillisecondsToInterval(100));
       }
       EXPECT_TRUE(false); // We should never get here.
     }), NS_DISPATCH_NORMAL);
 
   pool->ShutdownWithTimeout(1000);
   EXPECT_EQ(Task::sCount, 3);
 }
+
+TEST(ThreadPool, ShutdownWithTimeoutThenSleep)
+{
+  Task::sCount = 0;
+  nsCOMPtr<nsIThreadPool> pool = new nsThreadPool();
+
+  for (int i = 0; i < 3; ++i) {
+    nsCOMPtr<nsIRunnable> task = new Task(i);
+    EXPECT_TRUE(task);
+
+    pool->Dispatch(task, NS_DISPATCH_NORMAL);
+  }
+
+  pool->Dispatch(NS_NewRunnableFunction("sleep-for-400-ms", []() {
+      printf("### running from thread that sleeps for 400ms: %p\n",
+             (void *) PR_GetCurrentThread());
+      PR_Sleep(PR_MillisecondsToInterval(400));
+      Task::sCount++;
+      printf("### thread awoke from long sleep: %p\n",
+             (void *) PR_GetCurrentThread());
+    }), NS_DISPATCH_NORMAL);
+
+
+  // Wait for a max of 300 ms. The thread should still be sleeping, and will
+  // be leaked.
+  pool->ShutdownWithTimeout(300);
+  EXPECT_EQ(Task::sCount, 3);
+
+  // Sleep for a bit, and wait for the last thread to finish up.
+  PR_Sleep(PR_MillisecondsToInterval(200));
+
+  // Process events so the shutdown ack is received
+  NS_ProcessPendingEvents(NS_GetCurrentThread());
+
+  EXPECT_EQ(Task::sCount, 4);
+}
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -913,18 +913,19 @@ nsThread::ShutdownComplete(NotNull<nsThr
   mThread = nullptr;
 
 #ifdef DEBUG
   nsCOMPtr<nsIThreadObserver> obs = mEvents->GetObserver();
   MOZ_ASSERT(!obs, "Should have been cleared at shutdown!");
 #endif
 
   // Delete aContext.
-  MOZ_ALWAYS_TRUE(
-    aContext->mJoiningThread->mRequestedShutdownContexts.RemoveElement(aContext));
+  // aContext might not be in mRequestedShutdownContexts if it belongs to a
+  // thread that was leaked by calling nsIThreadPool::ShutdownWithTimeout.
+  aContext->mJoiningThread->mRequestedShutdownContexts.RemoveElement(aContext);
 }
 
 void
 nsThread::WaitForAllAsynchronousShutdowns()
 {
   // This is the motivating example for why SpinEventLoop has the template
   // parameter we are providing here.
   SpinEventLoopUntil<ProcessFailureBehavior::IgnoreAndContinue>([&]() {
--- a/xpcom/threads/nsThread.h
+++ b/xpcom/threads/nsThread.h
@@ -201,17 +201,18 @@ protected:
   //
   // For full nsThreads, they will always contain valid pointers. For thin
   // wrappers around non-XPCOM threads, they will be null, and event dispatch
   // methods which rely on them will fail (and assert) if called.
   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;
+  using ShutdownContexts = nsTArray<nsAutoPtr<struct nsThreadShutdownContext>>;
+  ShutdownContexts mRequestedShutdownContexts;
   // The shutdown context for ourselves.
   struct nsThreadShutdownContext* mShutdownContext;
 
   mozilla::CycleCollectedJSContext* mScriptObserver;
 
   PRThread* mThread;
   void*     mStackBase = nullptr;
   uint32_t  mStackSize;
--- a/xpcom/threads/nsThreadPool.cpp
+++ b/xpcom/threads/nsThreadPool.cpp
@@ -437,17 +437,23 @@ nsThreadPool::ShutdownWithTimeout(int32_
   // For any threads that have not shutdown yet, we need to remove them from
   // mRequestedShutdownContexts so the thread manager does not wait for them
   // at shutdown.
   for (int32_t i = 0; i < threads.Count(); ++i) {
     nsThread* thread = static_cast<nsThread*>(threads[i]);
     // If mThread is not null on the thread it means that it hasn't shutdown
     // context[i] corresponds to thread[i]
     if (thread->mThread && contexts[i]) {
-      currentThread->mRequestedShutdownContexts.RemoveElement(contexts[i]);
+      auto index = currentThread->mRequestedShutdownContexts.IndexOf(contexts[i]);
+      if (index != nsThread::ShutdownContexts::NoIndex) {
+        // We must leak the shutdown context just in case the leaked thread
+        // does get unstuck and completes before the main thread is done.
+        currentThread->mRequestedShutdownContexts[index].forget();
+        currentThread->mRequestedShutdownContexts.RemoveElementsAt(index, 1);
+      }
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsThreadPool::GetThreadLimit(uint32_t* aValue)