From 6d037d0cba2c3c083c4cec8728532b0bb4f1881d Mon Sep 17 00:00:00 2001 From: Coroiu Cristina Date: Fri, 27 Jul 2018 08:56:36 +0300 Subject: [PATCH] Backed out 9 changesets (bug 1476405) for causing leaks Backed out changeset 4113d6fb3c1c (bug 1476405) Backed out changeset cb7f7cc32687 (bug 1476405) Backed out changeset 6d18a8bd5ee3 (bug 1476405) Backed out changeset b2a99f50e642 (bug 1476405) Backed out changeset b5b9d295545d (bug 1476405) Backed out changeset f092a32a3639 (bug 1476405) Backed out changeset 6c154f4d9dd9 (bug 1476405) Backed out changeset d0ebb3aa8e0f (bug 1476405) Backed out changeset 06b8093ddc6a (bug 1476405) --- dom/media/CubebUtils.cpp | 4 - .../src/base/platform_thread_posix.cc | 30 +-- ipc/chromium/src/base/platform_thread_win.cc | 28 ++- js/src/vm/HelperThreads.cpp | 7 +- js/xpconnect/src/XPCJSContext.cpp | 8 +- media/audioipc/client/src/context.rs | 25 +-- media/audioipc/client/src/lib.rs | 3 - .../BackgroundHangMonitor.cpp | 3 +- tools/profiler/core/platform.cpp | 5 - xpcom/threads/nsThread.cpp | 174 ++++++++---------- xpcom/threads/nsThread.h | 11 -- xpcom/threads/nsThreadManager.cpp | 104 ++++------- xpcom/threads/nsThreadManager.h | 4 +- 13 files changed, 147 insertions(+), 259 deletions(-) diff --git a/dom/media/CubebUtils.cpp b/dom/media/CubebUtils.cpp index fe2643dd2fc4..ee750011ffff 100644 --- a/dom/media/CubebUtils.cpp +++ b/dom/media/CubebUtils.cpp @@ -59,7 +59,6 @@ struct AudioIpcInitParams { int mServerConnection; size_t mPoolSize; size_t mStackSize; - void (*mThreadCreateCallback)(const char*); }; // These functions are provided by audioipc-server crate @@ -431,9 +430,6 @@ cubeb* GetCubebContextUnlocked() initParams.mPoolSize = sAudioIPCPoolSize; initParams.mStackSize = sAudioIPCStackSize; initParams.mServerConnection = sIPCConnection->ClonePlatformHandle().release(); - initParams.mThreadCreateCallback = [](const char* aName) { - PROFILER_REGISTER_THREAD(aName); - }; MOZ_LOG(gCubebLog, LogLevel::Debug, ("%s: %d", PREF_AUDIOIPC_POOL_SIZE, (int) initParams.mPoolSize)); MOZ_LOG(gCubebLog, LogLevel::Debug, ("%s: %d", PREF_AUDIOIPC_STACK_SIZE, (int) initParams.mStackSize)); diff --git a/ipc/chromium/src/base/platform_thread_posix.cc b/ipc/chromium/src/base/platform_thread_posix.cc index 1c6410453fcf..21398dc8a5b0 100644 --- a/ipc/chromium/src/base/platform_thread_posix.cc +++ b/ipc/chromium/src/base/platform_thread_posix.cc @@ -26,8 +26,6 @@ #include #endif -#include "nsThreadUtils.h" - #if defined(OS_MACOSX) namespace base { void InitThreading(); @@ -35,10 +33,6 @@ void InitThreading(); #endif static void* ThreadFunc(void* closure) { - // Create a nsThread wrapper for the current platform thread, and register it - // with the thread manager. - (void) NS_GetCurrentThread(); - PlatformThread::Delegate* delegate = static_cast(closure); delegate->ThreadMain(); @@ -98,10 +92,21 @@ void PlatformThread::SetName(const char* name) { if (PlatformThread::CurrentId() == getpid()) return; - // Using NS_SetCurrentThreadName, as opposed to using platform APIs directly, - // also sets the thread name on the PRThread wrapper, and allows us to - // retrieve it using PR_GetThreadName. - NS_SetCurrentThreadName(name); + // http://0pointer.de/blog/projects/name-your-threads.html + // Set the name for the LWP (which gets truncated to 15 characters). + // Note that glibc also has a 'pthread_setname_np' api, but it may not be + // available everywhere and it's only benefit over using prctl directly is + // that it can set the name of threads other than the current thread. +#if defined(OS_LINUX) + prctl(PR_SET_NAME, reinterpret_cast(name), 0, 0, 0); +#elif defined(OS_NETBSD) + pthread_setname_np(pthread_self(), "%s", (void *)name); +#elif defined(OS_BSD) && !defined(__GLIBC__) + pthread_set_name_np(pthread_self(), name); +#elif defined(OS_SOLARIS) + pthread_setname_np(pthread_self(), name); +#else +#endif } #endif // !OS_MACOSX @@ -124,9 +129,8 @@ bool CreateThread(size_t stack_size, bool joinable, pthread_attr_setdetachstate(&attributes, PTHREAD_CREATE_DETACHED); } - if (stack_size == 0) - stack_size = nsIThreadManager::DEFAULT_STACK_SIZE; - pthread_attr_setstacksize(&attributes, stack_size); + if (stack_size > 0) + pthread_attr_setstacksize(&attributes, stack_size); success = !pthread_create(thread_handle, &attributes, ThreadFunc, delegate); diff --git a/ipc/chromium/src/base/platform_thread_win.cc b/ipc/chromium/src/base/platform_thread_win.cc index 762aa7e305e3..2e7614230624 100644 --- a/ipc/chromium/src/base/platform_thread_win.cc +++ b/ipc/chromium/src/base/platform_thread_win.cc @@ -9,8 +9,6 @@ #include "base/logging.h" #include "base/win_util.h" -#include "nsThreadUtils.h" - namespace { // The information on how to set the thread name comes from @@ -25,10 +23,6 @@ typedef struct tagTHREADNAME_INFO { } THREADNAME_INFO; DWORD __stdcall ThreadFunc(void* closure) { - // Create a nsThread wrapper for the current platform thread, and register it - // with the thread manager. - (void) NS_GetCurrentThread(); - PlatformThread::Delegate* delegate = static_cast(closure); delegate->ThreadMain(); @@ -54,10 +48,24 @@ void PlatformThread::Sleep(int duration_ms) { // static void PlatformThread::SetName(const char* name) { - // Using NS_SetCurrentThreadName, as opposed to using platform APIs directly, - // also sets the thread name on the PRThread wrapper, and allows us to - // retrieve it using PR_GetThreadName. - NS_SetCurrentThreadName(name); +#ifdef HAVE_SEH_EXCEPTIONS + // The debugger needs to be around to catch the name in the exception. If + // there isn't a debugger, we are just needlessly throwing an exception. + if (!::IsDebuggerPresent()) + return; + + THREADNAME_INFO info; + info.dwType = 0x1000; + info.szName = name; + info.dwThreadID = CurrentId(); + info.dwFlags = 0; + + MOZ_SEH_TRY { + RaiseException(kVCThreadNameException, 0, sizeof(info)/sizeof(DWORD), + reinterpret_cast(&info)); + } MOZ_SEH_EXCEPT(EXCEPTION_CONTINUE_EXECUTION) { + } +#endif } // static diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp index 86e0f3b9eceb..ae15f86f5d50 100644 --- a/js/src/vm/HelperThreads.cpp +++ b/js/src/vm/HelperThreads.cpp @@ -943,12 +943,7 @@ js::CurrentThreadIsParseThread() } #endif -// We want our default stack size limit to be approximately 2MB, to be safe, but -// expect most threads to use much less. On Linux, however, requesting a stack -// of 2MB or larger risks the kernel allocating an entire 2MB huge page for it -// on first access, which we do not want. To avoid this possibility, we subtract -// 2 standard VM page sizes from our default. -static const uint32_t kDefaultHelperStackSize = 2048 * 1024 - 2 * 4096; +static const uint32_t kDefaultHelperStackSize = 2048 * 1024; static const uint32_t kDefaultHelperStackQuota = 1800 * 1024; // TSan enforces a minimum stack size that's just slightly larger than our diff --git a/js/xpconnect/src/XPCJSContext.cpp b/js/xpconnect/src/XPCJSContext.cpp index 59ebfd9079e1..496212d7bbde 100644 --- a/js/xpconnect/src/XPCJSContext.cpp +++ b/js/xpconnect/src/XPCJSContext.cpp @@ -79,10 +79,6 @@ using namespace xpc; using namespace JS; using mozilla::dom::AutoEntryScript; -// The watchdog thread loop is pretty trivial, and should not require much stack -// space to do its job. So only give it 32KiB. -static constexpr size_t kWatchdogStackSize = 32 * 1024; - static void WatchdogMain(void* arg); class Watchdog; class WatchdogManager; @@ -147,7 +143,7 @@ class Watchdog // join it on shutdown. mThread = PR_CreateThread(PR_USER_THREAD, WatchdogMain, this, PR_PRIORITY_NORMAL, PR_GLOBAL_THREAD, - PR_JOINABLE_THREAD, kWatchdogStackSize); + PR_JOINABLE_THREAD, 0); if (!mThread) MOZ_CRASH("PR_CreateThread failed!"); @@ -476,8 +472,6 @@ static void WatchdogMain(void* arg) { AUTO_PROFILER_REGISTER_THREAD("JS Watchdog"); - // Create an nsThread wrapper for the thread and register it with the thread manager. - Unused << NS_GetCurrentThread(); NS_SetCurrentThreadName("JS Watchdog"); Watchdog* self = static_cast(arg); diff --git a/media/audioipc/client/src/context.rs b/media/audioipc/client/src/context.rs index de3df3fd4f0f..1e3a5311ffa5 100644 --- a/media/audioipc/client/src/context.rs +++ b/media/audioipc/client/src/context.rs @@ -20,7 +20,6 @@ use std::os::raw::c_void; use std::os::unix::io::FromRawFd; use std::os::unix::net; use std::sync::mpsc; -use std::thread; use stream; use tokio_core::reactor::{Handle, Remote}; use tokio_uds::UnixStream; @@ -100,25 +99,9 @@ impl ContextOps for ClientContext { let (tx_rpc, rx_rpc) = mpsc::channel(); - let params = CPUPOOL_INIT_PARAMS.with(|p| { - p.replace(None).unwrap() - }); - - let thread_create_callback = params.thread_create_callback; - - let register_thread = move || { - if let Some(func) = thread_create_callback { - let thr = thread::current(); - let name = CString::new(thr.name().unwrap()).unwrap(); - func(name.as_ptr()); - } - }; - let core = t!(core::spawn_thread("AudioIPC Client RPC", move || { let handle = core::handle(); - register_thread(); - open_server_stream() .ok() .and_then(|stream| UnixStream::from_stream(stream, &handle).ok()) @@ -133,12 +116,14 @@ impl ContextOps for ClientContext { let rpc = t!(rx_rpc.recv()); - let cpupool = futures_cpupool::Builder::new() + let cpupool = CPUPOOL_INIT_PARAMS.with(|p| { + let params = p.replace(None).unwrap(); + futures_cpupool::Builder::new() .name_prefix("AudioIPC") - .after_start(register_thread) .pool_size(params.pool_size) .stack_size(params.stack_size) - .create(); + .create() + }); let ctx = Box::new(ClientContext { _ops: &CLIENT_OPS as *const _, diff --git a/media/audioipc/client/src/lib.rs b/media/audioipc/client/src/lib.rs index 21d5cc0d8a62..f2cd99362d0f 100644 --- a/media/audioipc/client/src/lib.rs +++ b/media/audioipc/client/src/lib.rs @@ -37,14 +37,12 @@ pub struct AudioIpcInitParams { pub server_connection: c_int, pub pool_size: usize, pub stack_size: usize, - pub thread_create_callback: Option, } #[derive(Clone, Copy, Debug)] struct CpuPoolInitParams { pub pool_size: usize, pub stack_size: usize, - pub thread_create_callback: Option, } impl CpuPoolInitParams { @@ -52,7 +50,6 @@ impl CpuPoolInitParams { CpuPoolInitParams { pool_size: params.pool_size, stack_size: params.stack_size, - thread_create_callback: params.thread_create_callback, } } } diff --git a/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp b/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp index 827e77750b04..f9e7922fe538 100644 --- a/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp +++ b/toolkit/components/backgroundhangmonitor/BackgroundHangMonitor.cpp @@ -268,8 +268,7 @@ BackgroundHangManager::BackgroundHangManager() mHangMonitorThread = PR_CreateThread( PR_USER_THREAD, MonitorThread, this, - PR_PRIORITY_LOW, PR_GLOBAL_THREAD, PR_JOINABLE_THREAD, - nsIThreadManager::DEFAULT_STACK_SIZE); + PR_PRIORITY_LOW, PR_GLOBAL_THREAD, PR_JOINABLE_THREAD, 0); MOZ_ASSERT(mHangMonitorThread, "Failed to create BHR monitor thread"); diff --git a/tools/profiler/core/platform.cpp b/tools/profiler/core/platform.cpp index 5a2404839324..0ef4c807ab32 100644 --- a/tools/profiler/core/platform.cpp +++ b/tools/profiler/core/platform.cpp @@ -3298,11 +3298,6 @@ profiler_register_thread(const char* aName, void* aGuessStackTop) MOZ_ASSERT_IF(NS_IsMainThread(), Scheduler::IsCooperativeThread()); MOZ_RELEASE_ASSERT(CorePS::Exists()); - // Make sure we have a nsThread wrapper for the current thread, and that NSPR - // knows its name. - (void) NS_GetCurrentThread(); - NS_SetCurrentThreadName(aName); - PSAutoLock lock(gPSMutex); void* stackTop = GetStackTop(aGuessStackTop); diff --git a/xpcom/threads/nsThread.cpp b/xpcom/threads/nsThread.cpp index b687de9d63cb..8c7d0a6cf8bf 100644 --- a/xpcom/threads/nsThread.cpp +++ b/xpcom/threads/nsThread.cpp @@ -405,13 +405,6 @@ nsThread::ThreadList() return sList; } -/* static */ void -nsThread::ClearThreadList() -{ - OffTheBooksMutexAutoLock mal(ThreadListMutex()); - while (ThreadList().popFirst()) {} -} - /* static */ nsThreadEnumerator nsThread::Enumerate() { @@ -427,6 +420,7 @@ nsThread::ThreadFunc(void* aArg) nsThread* self = initData->thread; // strong reference self->mThread = PR_GetCurrentThread(); + self->mThreadId = uint32_t(PlatformThread::CurrentId()); self->mVirtualThread = GetCurrentVirtualThread(); self->mEventTarget->SetCurrentThread(); SetupCurrentThreadForChaosMode(); @@ -435,7 +429,71 @@ nsThread::ThreadFunc(void* aArg) NS_SetCurrentThreadName(initData->name.BeginReading()); } - self->InitCommon(); + { +#if defined(XP_LINUX) + pthread_attr_t attr; + pthread_attr_init(&attr); + pthread_getattr_np(pthread_self(), &attr); + + size_t stackSize; + pthread_attr_getstack(&attr, &self->mStackBase, &stackSize); + + // Glibc prior to 2.27 reports the stack size and base including the guard + // region, so we need to compensate for it to get accurate accounting. + // Also, this behavior difference isn't guarded by a versioned symbol, so we + // actually need to check the runtime glibc version, not the version we were + // compiled against. + static bool sAdjustForGuardSize = ({ +#ifdef __GLIBC__ + unsigned major, minor; + sscanf(gnu_get_libc_version(), "%u.%u", &major, &minor) < 2 || + major < 2 || (major == 2 && minor < 27); +#else + false; +#endif + }); + if (sAdjustForGuardSize) { + size_t guardSize; + pthread_attr_getguardsize(&attr, &guardSize); + + // Note: This assumes that the stack grows down, as is the case on all of + // our tier 1 platforms. On platforms where the stack grows up, the + // mStackBase adjustment is unnecessary, but doesn't cause any harm other + // than under-counting stack memory usage by one page. + self->mStackBase = reinterpret_cast(self->mStackBase) + guardSize; + stackSize -= guardSize; + } + + self->mStackSize = stackSize; + + // This is a bit of a hack. + // + // We really do want the NOHUGEPAGE flag on our thread stacks, since we + // don't expect any of them to need anywhere near 2MB of space. But setting + // it here is too late to have an effect, since the first stack page has + // already been faulted in existence, and NSPR doesn't give us a way to set + // it beforehand. + // + // What this does get us, however, is a different set of VM flags on our + // thread stacks compared to normal heap memory. Which makes the Linux + // kernel report them as separate regions, even when they are adjacent to + // heap memory. This allows us to accurately track the actual memory + // consumption of our allocated stacks. + madvise(self->mStackBase, stackSize, MADV_NOHUGEPAGE); + + pthread_attr_destroy(&attr); +#elif defined(XP_WIN) + static const DynamicallyLinkedFunctionPtr + sGetStackLimits(L"kernel32.dll", "GetCurrentThreadStackLimits"); + + if (sGetStackLimits) { + ULONG_PTR stackBottom, stackTop; + sGetStackLimits(&stackBottom, &stackTop); + self->mStackBase = reinterpret_cast(stackBottom); + self->mStackSize = stackTop - stackBottom; + } +#endif + } // Inform the ThreadManager nsThreadManager::get().RegisterCurrentThread(*self); @@ -516,81 +574,6 @@ nsThread::ThreadFunc(void* aArg) NS_RELEASE(self); } -void -nsThread::InitCommon() -{ - mThreadId = uint32_t(PlatformThread::CurrentId()); - - { -#if defined(XP_LINUX) - pthread_attr_t attr; - pthread_attr_init(&attr); - pthread_getattr_np(pthread_self(), &attr); - - size_t stackSize; - pthread_attr_getstack(&attr, &mStackBase, &stackSize); - - // Glibc prior to 2.27 reports the stack size and base including the guard - // region, so we need to compensate for it to get accurate accounting. - // Also, this behavior difference isn't guarded by a versioned symbol, so we - // actually need to check the runtime glibc version, not the version we were - // compiled against. - static bool sAdjustForGuardSize = ({ -#ifdef __GLIBC__ - unsigned major, minor; - sscanf(gnu_get_libc_version(), "%u.%u", &major, &minor) < 2 || - major < 2 || (major == 2 && minor < 27); -#else - false; -#endif - }); - if (sAdjustForGuardSize) { - size_t guardSize; - pthread_attr_getguardsize(&attr, &guardSize); - - // Note: This assumes that the stack grows down, as is the case on all of - // our tier 1 platforms. On platforms where the stack grows up, the - // mStackBase adjustment is unnecessary, but doesn't cause any harm other - // than under-counting stack memory usage by one page. - mStackBase = reinterpret_cast(mStackBase) + guardSize; - stackSize -= guardSize; - } - - mStackSize = stackSize; - - // This is a bit of a hack. - // - // We really do want the NOHUGEPAGE flag on our thread stacks, since we - // don't expect any of them to need anywhere near 2MB of space. But setting - // it here is too late to have an effect, since the first stack page has - // already been faulted in existence, and NSPR doesn't give us a way to set - // it beforehand. - // - // What this does get us, however, is a different set of VM flags on our - // thread stacks compared to normal heap memory. Which makes the Linux - // kernel report them as separate regions, even when they are adjacent to - // heap memory. This allows us to accurately track the actual memory - // consumption of our allocated stacks. - madvise(mStackBase, stackSize, MADV_NOHUGEPAGE); - - pthread_attr_destroy(&attr); -#elif defined(XP_WIN) - static const DynamicallyLinkedFunctionPtr - sGetStackLimits(L"kernel32.dll", "GetCurrentThreadStackLimits"); - - if (sGetStackLimits) { - ULONG_PTR stackBottom, stackTop; - sGetStackLimits(&stackBottom, &stackTop); - mStackBase = reinterpret_cast(stackBottom); - mStackSize = stackTop - stackBottom; - } -#endif - } - - OffTheBooksMutexAutoLock mal(ThreadListMutex()); - ThreadList().insertBack(this); -} - //----------------------------------------------------------------------------- // Tell the crash reporter to save a memory report if our heuristics determine @@ -687,17 +670,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()); - } - + MOZ_ASSERT(!isInList()); #ifdef DEBUG // We deliberately leak these so they can be tracked by the leak checker. // If you're having nsThreadShutdownContext leaks, you can set: @@ -731,6 +704,11 @@ nsThread::Init(const nsACString& aName) return NS_ERROR_OUT_OF_MEMORY; } + { + OffTheBooksMutexAutoLock mal(ThreadListMutex()); + ThreadList().insertBack(this); + } + // ThreadFunc will wait for this event to be run before it tries to access // mThread. By delaying insertion of this event into the queue, we ensure // that mThread is set properly. @@ -750,7 +728,6 @@ nsThread::InitCurrentThread() mThread = PR_GetCurrentThread(); mVirtualThread = GetCurrentVirtualThread(); SetupCurrentThreadForChaosMode(); - InitCommon(); nsThreadManager::get().RegisterCurrentThread(*this); return NS_OK; @@ -879,13 +856,6 @@ nsThread::ShutdownComplete(NotNull 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. diff --git a/xpcom/threads/nsThread.h b/xpcom/threads/nsThread.h index 776be874ba7e..ad018197ea8b 100644 --- a/xpcom/threads/nsThread.h +++ b/xpcom/threads/nsThread.h @@ -66,12 +66,6 @@ public: // Initialize this as a wrapper for the current PRThread. nsresult InitCurrentThread(); -private: - // Initializes the mThreadId and stack base/size members, and adds the thread - // to the ThreadList(). - void InitCommon(); - -public: // The PRThread corresponding to this thread. PRThread* GetPRThread() { @@ -176,11 +170,8 @@ protected: struct nsThreadShutdownContext* ShutdownInternal(bool aSync); - friend class nsThreadManager; - static mozilla::OffTheBooksMutex& ThreadListMutex(); static mozilla::LinkedList& ThreadList(); - static void ClearThreadList(); RefPtr mEvents; RefPtr mEventTarget; @@ -214,8 +205,6 @@ protected: // 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 mCurrentEvent; diff --git a/xpcom/threads/nsThreadManager.cpp b/xpcom/threads/nsThreadManager.cpp index 3b745755927e..b8b9af2ea7bd 100644 --- a/xpcom/threads/nsThreadManager.cpp +++ b/xpcom/threads/nsThreadManager.cpp @@ -94,27 +94,12 @@ AssertIsOnMainThread() typedef nsTArray>> nsThreadArray; -static bool sShutdownComplete; - //----------------------------------------------------------------------------- -/* static */ void -nsThreadManager::ReleaseThread(void* aData) +static void +ReleaseObject(void* aData) { - 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(aData); - - get().UnregisterCurrentThread(*thread, true); - - if (thread->mHasTLSEntry) { - thread->mHasTLSEntry = false; - thread->Release(); - } + static_cast(aData)->Release(); } // statically allocated instance @@ -251,7 +236,7 @@ nsThreadManager::Init() Scheduler::EventLoopActivation::Init(); - if (PR_NewThreadPrivateIndex(&mCurThreadIndex, ReleaseThread) == PR_FAILURE) { + if (PR_NewThreadPrivateIndex(&mCurThreadIndex, ReleaseObject) == PR_FAILURE) { return NS_ERROR_FAILURE; } @@ -319,34 +304,32 @@ nsThreadManager::Shutdown() // 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; { - // 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& thread = iter.Data(); - threads.AppendElement(WrapNotNull(thread)); - iter.Remove(); - } + OffTheBooksMutexAutoLock lock(mLock); + for (auto iter = mThreadsByPRThread.Iter(); !iter.Done(); iter.Next()) { + RefPtr& 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 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 thread = threads[i]; + if (thread->ShutdownRequired()) { + thread->Shutdown(); } } @@ -377,24 +360,6 @@ nsThreadManager::Shutdown() // 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> 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 @@ -412,28 +377,21 @@ nsThreadManager::RegisterCurrentThread(nsThread& aThread) 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) { MOZ_ASSERT(aThread.GetPRThread() == PR_GetCurrentThread(), "bad aThread"); - { - OffTheBooksMutexAutoLock lock(mLock); + OffTheBooksMutexAutoLock lock(mLock); - if (aIfExists && !mThreadsByPRThread.GetWeak(aThread.GetPRThread())) { - return; - } - - --mCurrentNumberOfThreads; - mThreadsByPRThread.Remove(aThread.GetPRThread()); - } + --mCurrentNumberOfThreads; + mThreadsByPRThread.Remove(aThread.GetPRThread()); PR_SetThreadPrivate(mCurThreadIndex, nullptr); - // Ref-count balanced via ReleaseThread + // Ref-count balanced via ReleaseObject } nsThread* diff --git a/xpcom/threads/nsThreadManager.h b/xpcom/threads/nsThreadManager.h index dd936be11081..b917133215ae 100644 --- a/xpcom/threads/nsThreadManager.h +++ b/xpcom/threads/nsThreadManager.h @@ -36,7 +36,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. @@ -85,8 +85,6 @@ private: SpinEventLoopUntilInternal(nsINestedEventLoopCondition* aCondition, bool aCheckingShutdown); - static void ReleaseThread(void* aData); - nsRefPtrHashtable, nsThread> mThreadsByPRThread; unsigned mCurThreadIndex; // thread-local-storage index RefPtr mMainThread;