From c24dce6f5b39c66e3c3e174e626f90b7914fdec5 Mon Sep 17 00:00:00 2001 From: Jens Stutte Date: Thu, 7 Nov 2024 06:18:52 +0000 Subject: [PATCH] Bug 1926722 - Refactoring: Make RemoteLazyInputStreamThread's lifecycle more sound. r=dom-storage-reviewers,asuth,xpcom-reviewers,nika In order to make things clearer, we: - Use already_AddRefed for Get and GetOrCreate's return, ensuring the AddRef happens under the locked mutex and expecting that callers hold a strong reference until they need it. - Make mThread const, such that there can never be a situation where we cannot use it as long as we have our RemoteLazyInputStreamThread instance. - Add thread safety annotions for gRemoteLazyThreadMutex. We can rely on mThread refusing to work after shutdown, in particular to dispatch runnables, such that we can just delegate most calls without additional checks. We also remove the previous explicit observer implementation in favor of the simpler RunOnShutdown. Differential Revision: https://phabricator.services.mozilla.com/D227036 --- dom/file/ipc/RemoteLazyInputStream.cpp | 12 +- dom/file/ipc/RemoteLazyInputStreamChild.cpp | 2 +- dom/file/ipc/RemoteLazyInputStreamThread.cpp | 163 +++++------------- dom/file/ipc/RemoteLazyInputStreamThread.h | 20 +-- .../web-platform/meta/IndexedDB/__dir__.ini | 2 +- 5 files changed, 66 insertions(+), 133 deletions(-) diff --git a/dom/file/ipc/RemoteLazyInputStream.cpp b/dom/file/ipc/RemoteLazyInputStream.cpp index 51b33d9da2d1..f631169f16df 100644 --- a/dom/file/ipc/RemoteLazyInputStream.cpp +++ b/dom/file/ipc/RemoteLazyInputStream.cpp @@ -157,7 +157,8 @@ RemoteLazyInputStream::RemoteLazyInputStream(nsIInputStream* aStream) static already_AddRefed BindChildActor( nsID aId, mozilla::ipc::Endpoint aEndpoint) { - auto* thread = RemoteLazyInputStreamThread::GetOrCreate(); + RefPtr thread = + RemoteLazyInputStreamThread::GetOrCreate(); if (NS_WARN_IF(!thread)) { return nullptr; } @@ -803,7 +804,8 @@ void RemoteLazyInputStream::StreamNeeded() { MOZ_LOG(gRemoteLazyStreamLog, LogLevel::Debug, ("StreamNeeded %s", Describe().get())); - auto* thread = RemoteLazyInputStreamThread::GetOrCreate(); + RefPtr thread = + RemoteLazyInputStreamThread::GetOrCreate(); if (NS_WARN_IF(!thread)) { return; } @@ -1216,7 +1218,8 @@ RemoteLazyInputStream::AsyncLengthWait(nsIInputStreamLengthCallback* aCallback, if (mActor) { if (aCallback) { - auto* thread = RemoteLazyInputStreamThread::GetOrCreate(); + RefPtr thread = + RemoteLazyInputStreamThread::GetOrCreate(); if (NS_WARN_IF(!thread)) { return NS_ERROR_ILLEGAL_DURING_SHUTDOWN; } @@ -1324,7 +1327,8 @@ void RemoteLazyInputStream::IPCWrite(IPC::MessageWriter* aWriter) { MOZ_ALWAYS_SUCCEEDS( PRemoteLazyInputStream::CreateEndpoints(&parentEp, &childEp)); - auto* thread = RemoteLazyInputStreamThread::GetOrCreate(); + RefPtr thread = + RemoteLazyInputStreamThread::GetOrCreate(); if (thread) { thread->Dispatch(NS_NewRunnableFunction( "RemoteLazyInputStreamChild::SendClone", diff --git a/dom/file/ipc/RemoteLazyInputStreamChild.cpp b/dom/file/ipc/RemoteLazyInputStreamChild.cpp index f03aa47aad8c..3444c56f19b1 100644 --- a/dom/file/ipc/RemoteLazyInputStreamChild.cpp +++ b/dom/file/ipc/RemoteLazyInputStreamChild.cpp @@ -29,7 +29,7 @@ void RemoteLazyInputStreamChild::StreamConsumed() { // When the count reaches zero, close the underlying actor. if (count == 0) { - auto* t = RemoteLazyInputStreamThread::Get(); + RefPtr t = RemoteLazyInputStreamThread::Get(); if (t) { t->Dispatch( NS_NewRunnableFunction("RemoteLazyInputStreamChild::StreamConsumed", diff --git a/dom/file/ipc/RemoteLazyInputStreamThread.cpp b/dom/file/ipc/RemoteLazyInputStreamThread.cpp index 67f670ac4aee..20d8640c36ad 100644 --- a/dom/file/ipc/RemoteLazyInputStreamThread.cpp +++ b/dom/file/ipc/RemoteLazyInputStreamThread.cpp @@ -22,114 +22,65 @@ namespace mozilla { namespace { StaticMutex gRemoteLazyThreadMutex; -StaticRefPtr gRemoteLazyThread; - -class ThreadInitializeRunnable final : public Runnable { - public: - ThreadInitializeRunnable() : Runnable("dom::ThreadInitializeRunnable") {} - - NS_IMETHOD - Run() override { - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - MOZ_ASSERT(gRemoteLazyThread); - if (NS_WARN_IF(!gRemoteLazyThread->InitializeOnMainThread())) { - // RemoteLazyInputStreamThread::GetOrCreate might have handed out a - // pointer to our thread already at this point such that we cannot - // just do gRemoteLazyThread = nullptr; here. - MOZ_DIAGNOSTIC_ASSERT( - false, "Async gRemoteLazyThread->InitializeOnMainThread() failed."); - return NS_ERROR_FAILURE; - } - return NS_OK; - } -}; +StaticRefPtr gRemoteLazyThread + MOZ_GUARDED_BY(gRemoteLazyThreadMutex); } // namespace -NS_IMPL_ISUPPORTS(RemoteLazyInputStreamThread, nsIObserver, nsIEventTarget, +NS_IMPL_ISUPPORTS(RemoteLazyInputStreamThread, nsIEventTarget, nsISerialEventTarget, nsIDirectTaskDispatcher) -bool RLISThreadIsInOrBeyondShutdown() { - // ShutdownPhase::XPCOMShutdownThreads matches - // obs->AddObserver(this, NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID, false); - return AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads); +/* static */ +already_AddRefed +RemoteLazyInputStreamThread::Get() { + StaticMutexAutoLock lock(gRemoteLazyThreadMutex); + + return do_AddRef(gRemoteLazyThread); } /* static */ -RemoteLazyInputStreamThread* RemoteLazyInputStreamThread::Get() { - if (RLISThreadIsInOrBeyondShutdown()) { - return nullptr; - } - +already_AddRefed +RemoteLazyInputStreamThread::GetOrCreate() { StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - return gRemoteLazyThread; -} - -/* static */ -RemoteLazyInputStreamThread* RemoteLazyInputStreamThread::GetOrCreate() { - if (RLISThreadIsInOrBeyondShutdown()) { + if (AppShutdown::IsInOrBeyond(ShutdownPhase::XPCOMShutdownThreads)) { return nullptr; } - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - if (!gRemoteLazyThread) { - gRemoteLazyThread = new RemoteLazyInputStreamThread(); - if (!gRemoteLazyThread->Initialize()) { - gRemoteLazyThread = nullptr; + nsCOMPtr thread; + nsresult rv = NS_NewNamedThread("RemoteLzyStream", getter_AddRefs(thread)); + if (NS_WARN_IF(NS_FAILED(rv))) { + return nullptr; } + + gRemoteLazyThread = + new RemoteLazyInputStreamThread(WrapMovingNotNull(thread)); + + // Dispatch to the main thread, which will set up a listener + // to shut down the thread during XPCOMShutdownThreads. + // + // We do this even if we're already on the main thread, as + // if we're too late in shutdown, this will trigger the thread + // to shut down synchronously. + NS_DispatchToMainThread(NS_NewRunnableFunction( + "RemoteLazyInputStreamThread::MainThreadInit", [] { + RunOnShutdown( + [] { + RefPtr rlis = + RemoteLazyInputStreamThread::Get(); + // This is the only place supposed to ever null our reference. + MOZ_ASSERT(rlis); + rlis->mThread->Shutdown(); + + StaticMutexAutoLock lock(gRemoteLazyThreadMutex); + gRemoteLazyThread = nullptr; + }, + ShutdownPhase::XPCOMShutdownThreads); + })); } - return gRemoteLazyThread; -} - -bool RemoteLazyInputStreamThread::Initialize() { - nsCOMPtr thread; - nsresult rv = NS_NewNamedThread("RemoteLzyStream", getter_AddRefs(thread)); - if (NS_WARN_IF(NS_FAILED(rv))) { - return false; - } - - mThread = thread; - - if (!NS_IsMainThread()) { - RefPtr runnable = new ThreadInitializeRunnable(); - nsresult rv = SchedulerGroup::Dispatch(runnable.forget()); - return !NS_WARN_IF(NS_FAILED(rv)); - } - - return InitializeOnMainThread(); -} - -bool RemoteLazyInputStreamThread::InitializeOnMainThread() { - MOZ_ASSERT(NS_IsMainThread()); - - nsCOMPtr obs = services::GetObserverService(); - if (NS_WARN_IF(!obs)) { - return false; - } - - nsresult rv = - obs->AddObserver(this, NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID, false); - return !NS_WARN_IF(NS_FAILED(rv)); -} - -NS_IMETHODIMP -RemoteLazyInputStreamThread::Observe(nsISupports* aSubject, const char* aTopic, - const char16_t* aData) { - MOZ_ASSERT(!strcmp(aTopic, NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID)); - - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - - if (mThread) { - mThread->Shutdown(); - mThread = nullptr; - } - - gRemoteLazyThread = nullptr; - - return NS_OK; + return do_AddRef(gRemoteLazyThread); } // nsIEventTarget @@ -147,25 +98,13 @@ RemoteLazyInputStreamThread::IsOnCurrentThread(bool* aRetval) { NS_IMETHODIMP RemoteLazyInputStreamThread::Dispatch(already_AddRefed aRunnable, uint32_t aFlags) { - nsCOMPtr runnable(aRunnable); - - if (RLISThreadIsInOrBeyondShutdown()) { - // nsIEventTarget::Dispatch must leak the runnable if the dispatch fails. - Unused << runnable.forget(); - - return NS_ERROR_ILLEGAL_DURING_SHUTDOWN; - } - - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - - return mThread->Dispatch(runnable.forget(), aFlags); + return mThread->Dispatch(std::move(aRunnable), aFlags); } NS_IMETHODIMP RemoteLazyInputStreamThread::DispatchFromScript(nsIRunnable* aRunnable, uint32_t aFlags) { - nsCOMPtr runnable(aRunnable); - return Dispatch(runnable.forget(), aFlags); + return mThread->Dispatch(do_AddRef(aRunnable), aFlags); } NS_IMETHODIMP @@ -189,8 +128,6 @@ RemoteLazyInputStreamThread::DispatchDirectTask( already_AddRefed aRunnable) { nsCOMPtr runnable(aRunnable); - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - nsCOMPtr dispatcher = do_QueryInterface(mThread); if (dispatcher) { @@ -201,8 +138,6 @@ RemoteLazyInputStreamThread::DispatchDirectTask( } NS_IMETHODIMP RemoteLazyInputStreamThread::DrainDirectTasks() { - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - nsCOMPtr dispatcher = do_QueryInterface(mThread); if (dispatcher) { @@ -213,8 +148,6 @@ NS_IMETHODIMP RemoteLazyInputStreamThread::DrainDirectTasks() { } NS_IMETHODIMP RemoteLazyInputStreamThread::HaveDirectTasks(bool* aValue) { - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - nsCOMPtr dispatcher = do_QueryInterface(mThread); if (dispatcher) { @@ -225,12 +158,8 @@ NS_IMETHODIMP RemoteLazyInputStreamThread::HaveDirectTasks(bool* aValue) { } bool IsOnDOMFileThread() { - MOZ_ASSERT(!RLISThreadIsInOrBeyondShutdown()); - - StaticMutexAutoLock lock(gRemoteLazyThreadMutex); - MOZ_ASSERT(gRemoteLazyThread); - - return gRemoteLazyThread->IsOnCurrentThreadInfallible(); + RefPtr rlis = RemoteLazyInputStreamThread::Get(); + return rlis && rlis->IsOnCurrentThread(); } void AssertIsOnDOMFileThread() { MOZ_ASSERT(IsOnDOMFileThread()); } diff --git a/dom/file/ipc/RemoteLazyInputStreamThread.h b/dom/file/ipc/RemoteLazyInputStreamThread.h index 378cb0900943..c9dc19781617 100644 --- a/dom/file/ipc/RemoteLazyInputStreamThread.h +++ b/dom/file/ipc/RemoteLazyInputStreamThread.h @@ -19,28 +19,28 @@ namespace mozilla { class RemoteLazyInputStreamChild; // XXX Rename this class since it's used by LSNG too. -class RemoteLazyInputStreamThread final : public nsIObserver, - public nsISerialEventTarget, +class RemoteLazyInputStreamThread final : public nsISerialEventTarget, public nsIDirectTaskDispatcher { public: NS_DECL_THREADSAFE_ISUPPORTS - NS_DECL_NSIOBSERVER - NS_DECL_NSIEVENTTARGET + NS_DECL_NSIEVENTTARGET_FULL NS_DECL_NSISERIALEVENTTARGET NS_DECL_NSIDIRECTTASKDISPATCHER - static RemoteLazyInputStreamThread* Get(); + explicit RemoteLazyInputStreamThread( + MovingNotNull> aThread) + : mThread(std::move(aThread)) {} - static RemoteLazyInputStreamThread* GetOrCreate(); + static already_AddRefed Get(); - bool Initialize(); - - bool InitializeOnMainThread(); + static already_AddRefed GetOrCreate(); private: ~RemoteLazyInputStreamThread() = default; - nsCOMPtr mThread; + // As long as we can access gRemoteLazyThread, mThread remains a valid + // object. We rely on it failing on late dispatch after its shutdown. + const NotNull> mThread; }; bool IsOnDOMFileThread(); diff --git a/testing/web-platform/meta/IndexedDB/__dir__.ini b/testing/web-platform/meta/IndexedDB/__dir__.ini index aa61e0010036..68aab897ce46 100644 --- a/testing/web-platform/meta/IndexedDB/__dir__.ini +++ b/testing/web-platform/meta/IndexedDB/__dir__.ini @@ -1,3 +1,3 @@ prefs: [extensions.blocklist.enabled:false] +lsan-allowed: [Malloc, NS_NewCancelableRunnableFunction, NS_NewPipe2, PR_NewMonitor, mozilla::RemoteLazyInputStream::CloneWithRange, mozilla::RemoteLazyInputStreamThread::GetOrCreate, nsSegmentedBuffer::AppendNewSegment, Alloc, MakeUnique, nsThread::nsThread, NewPage, mozilla::Queue, mozilla::detail::EventQueueInternal, mozilla::ThreadEventQueue::PutEventInternal ] leak-threshold: [default:51200, tab:51200] -lsan-allowed: [Alloc, MakeUnique, Malloc, NS_NewCancelableRunnableFunction, NS_NewPipe2, NewPage, PR_NewMonitor, mozilla::RemoteLazyInputStream::CloneWithRange, mozilla::RemoteLazyInputStreamThread::GetOrCreate, nsSegmentedBuffer::AppendNewSegment, nsThread::nsThread]