Bug 1836225 - Prevent stack walking deadlock with MOZ_PROFILER_STARTU…

…P=1. r=handyman

We currently fail to guarantee that OnEndDllLoad is called on the same
gLoaderObserver as OnBeginDllLoad. We must implement additional
synchronization to prevent a race condition where a call to
LoaderPrivateAPIImp::SetObserver would come in between the two and
change gLoaderObserver.

This has led to issues when using MOZ_PROFILER_STARTUP=1 where we would
have sStackWalkSuppressions reach (size_t)-1 instead of 0, later
resulting in deadlock or missing stacks. See bug 1687510 comment 10 for
extra details.

Depends on D181436

Differential Revision: https://phabricator.services.mozilla.com/D181437

browser/app/winlauncher/freestanding/LoaderPrivateAPI.cpp

@@ -115,6 +115,9 @@ static mozilla::freestanding::LoaderPrivateAPIImp gPrivateAPI;
 
 static mozilla::nt::SRWLock gLoaderObserverLock;
 static mozilla::nt::LoaderObserver* gLoaderObserver = &gDefaultObserver;
+static CONDITION_VARIABLE gLoaderObserverNoOngoingLoadsCV =
+    CONDITION_VARIABLE_INIT;
+static mozilla::Atomic<uint32_t> gLoaderObserverOngoingLoadsCount(0);
 
 namespace mozilla {
 namespace freestanding {
@@ -177,7 +180,11 @@ ModuleLoadInfo LoaderPrivateAPIImp::ConstructAndNotifyBeginDllLoad(
 
 bool LoaderPrivateAPIImp::SubstituteForLSP(PCUNICODE_STRING aLSPLeafName,
                                            PHANDLE aOutHandle) {
-  nt::AutoSharedLock lock(gLoaderObserverLock);
+  // This method should only be called between NotifyBeginDllLoad and
+  // NotifyEndDllLoad, so it is already protected against gLoaderObserver
+  // change, through gLoaderObserverOngoingLoadsCount.
+  MOZ_RELEASE_ASSERT(gLoaderObserverOngoingLoadsCount);
+
   return gLoaderObserver->SubstituteForLSP(aLSPLeafName, aOutHandle);
 }
 
@@ -190,13 +197,21 @@ void LoaderPrivateAPIImp::NotifyEndDllLoad(void* aContext,
     aModuleLoadInfo.CaptureBacktrace();
   }
 
-  nt::AutoSharedLock lock(gLoaderObserverLock);
+  // This method should only be called after a matching call to
+  // NotifyBeginDllLoad, so it is already protected against gLoaderObserver
+  // change, through gLoaderObserverOngoingLoadsCount.
 
   // We need to notify the observer that the DLL load has ended even when
   // |aLoadNtStatus| indicates a failure. This is to ensure that any resources
   // acquired by the observer during OnBeginDllLoad are cleaned up.
   gLoaderObserver->OnEndDllLoad(aContext, aLoadNtStatus,
                                 std::move(aModuleLoadInfo));
+
+  auto previousValue = gLoaderObserverOngoingLoadsCount--;
+  MOZ_RELEASE_ASSERT(previousValue);
+  if (previousValue == 1) {
+    ::RtlWakeAllConditionVariable(&gLoaderObserverNoOngoingLoadsCV);
+  }
 }
 
 nt::AllocatedUnicodeString LoaderPrivateAPIImp::GetSectionName(
@@ -246,6 +261,7 @@ nt::MemorySectionNameBuf LoaderPrivateAPIImp::GetSectionNameBuffer(
 void LoaderPrivateAPIImp::NotifyBeginDllLoad(
     void** aContext, PCUNICODE_STRING aRequestedDllName) {
   nt::AutoSharedLock lock(gLoaderObserverLock);
+  ++gLoaderObserverOngoingLoadsCount;
   gLoaderObserver->OnBeginDllLoad(aContext, aRequestedDllName);
 }
 
@@ -260,6 +276,11 @@ void LoaderPrivateAPIImp::SetObserver(nt::LoaderObserver* aNewObserver) {
   nt::LoaderObserver* prevLoaderObserver = nullptr;
 
   nt::AutoExclusiveLock lock(gLoaderObserverLock);
+  while (gLoaderObserverOngoingLoadsCount) {
+    NTSTATUS status = ::RtlSleepConditionVariableSRW(
+        &gLoaderObserverNoOngoingLoadsCV, &gLoaderObserverLock, nullptr, 0);
+    MOZ_RELEASE_ASSERT(NT_SUCCESS(status) && status != STATUS_TIMEOUT);
+  }
 
   MOZ_ASSERT(aNewObserver);
   if (!aNewObserver) {

mozglue/misc/NativeNt.h

@@ -93,6 +93,11 @@ VOID NTAPI RtlAcquireSRWLockShared(PSRWLOCK aLock);
 VOID NTAPI RtlReleaseSRWLockExclusive(PSRWLOCK aLock);
 VOID NTAPI RtlReleaseSRWLockShared(PSRWLOCK aLock);
 
+NTSTATUS NTAPI RtlSleepConditionVariableSRW(
+    PCONDITION_VARIABLE aConditionVariable, PSRWLOCK aSRWLock,
+    PLARGE_INTEGER aTimeOut, ULONG aFlags);
+VOID NTAPI RtlWakeAllConditionVariable(PCONDITION_VARIABLE aConditionVariable);
+
 ULONG NTAPI RtlNtStatusToDosError(NTSTATUS aStatus);
 VOID NTAPI RtlSetLastWin32Error(DWORD aError);
 DWORD NTAPI RtlGetLastWin32Error();

mozglue/misc/StackWalk.cpp

@@ -128,7 +128,12 @@ static Atomic<size_t> sStackWalkSuppressions;
 
 void SuppressStackWalking() { ++sStackWalkSuppressions; }
 
-void DesuppressStackWalking() { --sStackWalkSuppressions; }
+void DesuppressStackWalking() {
+  auto previousValue = sStackWalkSuppressions--;
+  // We should never desuppress from 0. See bug 1687510 comment 10 for an
+  // example in which this occured.
+  MOZ_RELEASE_ASSERT(previousValue);
+}
 
 MFBT_API
 AutoSuppressStackWalking::AutoSuppressStackWalking() { SuppressStackWalking(); }