From 54ac39289a500b5f252f14b7f59cb5c411950a0b Mon Sep 17 00:00:00 2001 From: Ray Kraesig Date: Fri, 4 Nov 2022 21:04:18 +0000 Subject: [PATCH] Bug 1772908 - [1/6] Drive-by cleanup: simplify use of SyncRunnable r=xpcom-reviewers,necko-reviewers,nika,valentin `SyncRunnable`'s helper functions take an `nsIRunnable *`; but the most common way of building nsIRunnables, `NS_NewRunnableFunction`, returns an `already_AddRefed` instead. Add two new overloads of the helper functions to eliminate the impedance mismatch. (This does result in an uncomfortable amount of code duplication. While we could eliminate that with appropriate use of SFINAE, it'll be simpler if we wait for C++20 and its `requires` keyword.) Additionally, add two explicitly-deleted overloads to catch and prevent a previously-common antipattern that presumably resulted from this type mismatch: accidentally wrapping the actual runnable in two layers of `SyncRunnable`. Fix the former use-sites appropriately. (This was probably harmless, but is also probably best avoided.) No functional changes. This is in some sense a continuation of bug 1281626. (This is no longer actually relevant to bug 1772908 due to a different approach being taken. It remains in the patchset anyway, for simplicity's sake.) Differential Revision: https://phabricator.services.mozilla.com/D157131 --- netwerk/base/nsSocketTransport2.cpp | 4 +-- netwerk/dns/DNSUtils.cpp | 4 +-- netwerk/dns/nsDNSService2.cpp | 5 ++- netwerk/protocol/http/AlternateServices.cpp | 4 +-- netwerk/protocol/http/nsHttpHandler.cpp | 7 +++-- security/manager/ssl/NSSKeyStore.cpp | 18 +++++------ security/manager/ssl/nsNSSComponent.cpp | 7 +++-- tools/fuzzing/ipc/IPCFuzzController.cpp | 11 +++---- xpcom/threads/SyncRunnable.h | 34 ++++++++++++++++++--- 9 files changed, 59 insertions(+), 35 deletions(-) diff --git a/netwerk/base/nsSocketTransport2.cpp b/netwerk/base/nsSocketTransport2.cpp index f0a9b0c6ded9..34fc565db9c1 100644 --- a/netwerk/base/nsSocketTransport2.cpp +++ b/netwerk/base/nsSocketTransport2.cpp @@ -976,8 +976,8 @@ nsresult nsSocketTransport::ResolveHost() { SyncRunnable::DispatchToThread( mainThread, - new SyncRunnable(NS_NewRunnableFunction( - "nsSocketTransport::ResolveHost->GetDNSService", initTask))); + NS_NewRunnableFunction("nsSocketTransport::ResolveHost->GetDNSService", + initTask)); } else { initTask(); } diff --git a/netwerk/dns/DNSUtils.cpp b/netwerk/dns/DNSUtils.cpp index 5a4ef20b3bd6..ac4647bd2128 100644 --- a/netwerk/dns/DNSUtils.cpp +++ b/netwerk/dns/DNSUtils.cpp @@ -58,8 +58,8 @@ nsresult DNSUtils::CreateChannelHelper(nsIURI* aUri, nsIChannel** aResult) { if (main) { // Forward to the main thread synchronously. SyncRunnable::DispatchToThread( - main, new SyncRunnable(NS_NewRunnableFunction( - "InitHttpHandler", []() { InitHttpHandler(); }))); + main, NS_NewRunnableFunction("InitHttpHandler", + []() { InitHttpHandler(); })); } } diff --git a/netwerk/dns/nsDNSService2.cpp b/netwerk/dns/nsDNSService2.cpp index d0eb13ee3afe..db2ca2703ab8 100644 --- a/netwerk/dns/nsDNSService2.cpp +++ b/netwerk/dns/nsDNSService2.cpp @@ -682,9 +682,8 @@ already_AddRefed GetOrInitDNSService() { return nullptr; } - SyncRunnable::DispatchToThread(mainThread, - new SyncRunnable(NS_NewRunnableFunction( - "GetOrInitDNSService", initTask))); + SyncRunnable::DispatchToThread( + mainThread, NS_NewRunnableFunction("GetOrInitDNSService", initTask)); } else { initTask(); } diff --git a/netwerk/protocol/http/AlternateServices.cpp b/netwerk/protocol/http/AlternateServices.cpp index 59d33d9f432a..c6e1590b5507 100644 --- a/netwerk/protocol/http/AlternateServices.cpp +++ b/netwerk/protocol/http/AlternateServices.cpp @@ -898,8 +898,8 @@ void AltSvcCache::EnsureStorageInited() { } SyncRunnable::DispatchToThread( - main, new SyncRunnable(NS_NewRunnableFunction( - "AltSvcCache::EnsureStorageInited", initTask))); + main, + NS_NewRunnableFunction("AltSvcCache::EnsureStorageInited", initTask)); } already_AddRefed AltSvcCache::LookupMapping( diff --git a/netwerk/protocol/http/nsHttpHandler.cpp b/netwerk/protocol/http/nsHttpHandler.cpp index 2cf7771a4737..9dd7aa848e8b 100644 --- a/netwerk/protocol/http/nsHttpHandler.cpp +++ b/netwerk/protocol/http/nsHttpHandler.cpp @@ -1839,9 +1839,10 @@ nsresult nsHttpHandler::SetAcceptLanguages() { // Forward to the main thread synchronously. SyncRunnable::DispatchToThread( - mainThread, new SyncRunnable(NS_NewRunnableFunction( - "nsHttpHandler::SetAcceptLanguages", - [&rv]() { rv = gHttpHandler->SetAcceptLanguages(); }))); + mainThread, + NS_NewRunnableFunction("nsHttpHandler::SetAcceptLanguages", [&rv]() { + rv = gHttpHandler->SetAcceptLanguages(); + })); return rv; } diff --git a/security/manager/ssl/NSSKeyStore.cpp b/security/manager/ssl/NSSKeyStore.cpp index 984e30506134..8574c4f3750f 100644 --- a/security/manager/ssl/NSSKeyStore.cpp +++ b/security/manager/ssl/NSSKeyStore.cpp @@ -62,10 +62,10 @@ nsresult NSSKeyStore::Lock() { // Forward to the main thread synchronously. SyncRunnable::DispatchToThread( - mainThread, new SyncRunnable(NS_NewRunnableFunction( - "NSSKeyStoreMainThreadLock", [slot = mSlot.get()]() { - NSSKeyStoreMainThreadLock(slot); - }))); + mainThread, NS_NewRunnableFunction("NSSKeyStoreMainThreadLock", + [slot = mSlot.get()]() { + NSSKeyStoreMainThreadLock(slot); + })); return NS_OK; } @@ -91,11 +91,11 @@ nsresult NSSKeyStore::Unlock() { // Forward to the main thread synchronously. nsresult result = NS_ERROR_FAILURE; SyncRunnable::DispatchToThread( - mainThread, new SyncRunnable(NS_NewRunnableFunction( - "NSSKeyStoreMainThreadUnlock", - [slot = mSlot.get(), result = &result]() { - *result = NSSKeyStoreMainThreadUnlock(slot); - }))); + mainThread, + NS_NewRunnableFunction("NSSKeyStoreMainThreadUnlock", + [slot = mSlot.get(), result = &result]() { + *result = NSSKeyStoreMainThreadUnlock(slot); + })); return result; } diff --git a/security/manager/ssl/nsNSSComponent.cpp b/security/manager/ssl/nsNSSComponent.cpp index 883c3ec80ed7..2101a77d0a74 100644 --- a/security/manager/ssl/nsNSSComponent.cpp +++ b/security/manager/ssl/nsNSSComponent.cpp @@ -185,9 +185,10 @@ bool EnsureNSSInitializedChromeOrContent() { // Forward to the main thread synchronously. mozilla::SyncRunnable::DispatchToThread( - mainThread, new SyncRunnable(NS_NewRunnableFunction( - "EnsureNSSInitializedChromeOrContent", - []() { EnsureNSSInitializedChromeOrContent(); }))); + mainThread, + NS_NewRunnableFunction("EnsureNSSInitializedChromeOrContent", []() { + EnsureNSSInitializedChromeOrContent(); + })); return initialized; } diff --git a/tools/fuzzing/ipc/IPCFuzzController.cpp b/tools/fuzzing/ipc/IPCFuzzController.cpp index 68a5ef6b4a34..860175cf1730 100644 --- a/tools/fuzzing/ipc/IPCFuzzController.cpp +++ b/tools/fuzzing/ipc/IPCFuzzController.cpp @@ -622,12 +622,11 @@ NS_IMETHODIMP IPCFuzzController::IPCFuzzLoop::Run() { SyncRunnable::DispatchToThread( GetMainThreadEventTarget(), - new SyncRunnable(NS_NewRunnableFunction( - "IPCFuzzController::StartFuzzing", [&]() -> void { - MOZ_FUZZING_NYX_PRINT("INFO: Main thread runnable start.\n"); - NS_ProcessPendingEvents(NS_GetCurrentThread()); - MOZ_FUZZING_NYX_PRINT("INFO: Main thread runnable done.\n"); - }))); + NS_NewRunnableFunction("IPCFuzzController::StartFuzzing", [&]() -> void { + MOZ_FUZZING_NYX_PRINT("INFO: Main thread runnable start.\n"); + NS_ProcessPendingEvents(NS_GetCurrentThread()); + MOZ_FUZZING_NYX_PRINT("INFO: Main thread runnable done.\n"); + })); MOZ_FUZZING_NYX_PRINT("INFO: Performing snapshot...\n"); Nyx::instance().start(); diff --git a/xpcom/threads/SyncRunnable.h b/xpcom/threads/SyncRunnable.h index e24ac6f83a0b..943765afaced 100644 --- a/xpcom/threads/SyncRunnable.h +++ b/xpcom/threads/SyncRunnable.h @@ -17,7 +17,7 @@ namespace mozilla { /** - * This class will wrap a nsIRunnable and dispatch it to the main thread + * This class will wrap a nsIRunnable and dispatch it to the target thread * synchronously. This is different from nsIEventTarget.DISPATCH_SYNC: * this class does not spin the event loop waiting for the event to be * dispatched. This means that you don't risk reentrance from pending @@ -25,11 +25,12 @@ namespace mozilla { * on this thread, or else you will deadlock. * * Typical usage: - * RefPtr sr = new SyncRunnable(new myrunnable...()); - * sr->DispatchToThread(t); + * RefPtr sr = new SyncRunnable(new myrunnable...()); + * sr->DispatchToThread(t); * - * We also provide a convenience wrapper: - * SyncRunnable::DispatchToThread(new myrunnable...()); + * We also provide convenience wrappers: + * SyncRunnable::DispatchToThread(pThread, new myrunnable...()); + * SyncRunnable::DispatchToThread(pThread, NS_NewRunnableFunction(...)); * */ class SyncRunnable : public Runnable { @@ -109,6 +110,29 @@ class SyncRunnable : public Runnable { return s->DispatchToThread(aThread, aForceDispatch); } + static nsresult DispatchToThread(nsIEventTarget* aThread, + already_AddRefed aRunnable, + bool aForceDispatch = false) { + RefPtr s(new SyncRunnable(std::move(aRunnable))); + return s->DispatchToThread(aThread, aForceDispatch); + } + + static nsresult DispatchToThread(AbstractThread* aThread, + already_AddRefed aRunnable, + bool aForceDispatch = false) { + RefPtr s(new SyncRunnable(std::move(aRunnable))); + return s->DispatchToThread(aThread, aForceDispatch); + } + + // These deleted overloads prevent accidentally (if harmlessly) double- + // wrapping SyncRunnable, which was previously a common anti-pattern. + static nsresult DispatchToThread(nsIEventTarget* aThread, + SyncRunnable* aRunnable, + bool aForceDispatch = false) = delete; + static nsresult DispatchToThread(AbstractThread* aThread, + SyncRunnable* aRunnable, + bool aForceDispatch = false) = delete; + protected: NS_IMETHOD Run() override { mRunnable->Run();