Bug 1337499 - Take the Win64 stackwalk lock in WalkStackMain64 to avoid deadlocks. r=mstange

mozglue/misc/StackWalk.cpp

@@ -9,6 +9,7 @@
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/IntegerPrintfMacros.h"
+#include "mozilla/ScopeExit.h"
 #include "mozilla/StackWalk.h"
 
 #include <string.h>
@@ -337,6 +338,20 @@ WalkStackMain64(struct WalkStackData* aData)
   frame64.AddrReturn.Mode  = AddrModeFlat;
 #endif
 
+#ifdef _WIN64
+  // Workaround possible deadlock where the thread we're profiling happens to
+  // be in RtlLookupFunctionEntry (see below) or in RtlAddFunctionTable or
+  // RtlDeleteFunctionTable when starting or shutting down the JS engine.
+  // On Win64 each of these Rtl* functions will take a lock, so we need to make
+  // sure we don't deadlock when a suspended thread is holding it.
+  if (!TryAcquireStackWalkWorkaroundLock()) {
+    return;
+  }
+  auto releaseLock = mozilla::MakeScopeExit([] {
+    ReleaseStackWalkWorkaroundLock();
+  });
+#endif
+
   // Skip our own stack walking frames.
   int skip = (aData->walkCallingThread ? 3 : 0) + aData->skipFrames;
 

tools/profiler/core/platform-win32.cc

@@ -34,9 +34,6 @@
 // Memory profile
 #include "nsMemoryReporterManager.h"
 
-#include "mozilla/StackWalk_windows.h"
-
-
 class PlatformData {
  public:
   // Get a handle to the calling thread. This is the thread that we are
@@ -238,29 +235,6 @@ class SamplerThread
       return;
     }
 
-    // Threads that may invoke JS require extra attention. Since, on windows,
-    // the jits also need to modify the same dynamic function table that we need
-    // to get a stack trace, we have to be wary of that to avoid deadlock.
-    //
-    // When embedded in Gecko, for threads that aren't the main thread,
-    // CanInvokeJS consults an unlocked value in the nsIThread, so we must
-    // consult this after suspending the profiled thread to avoid racing
-    // against a value change.
-    if (aThreadInfo->CanInvokeJS()) {
-      if (!TryAcquireStackWalkWorkaroundLock()) {
-        ResumeThread(profiled_thread);
-        return;
-      }
-
-      // It is safe to immediately drop the lock. We only need to contend with
-      // the case in which the profiled thread held needed system resources.
-      // If the profiled thread had held those resources, the trylock would have
-      // failed. Anyone else who grabs those resources will continue to make
-      // progress, since those threads are not suspended. Because of this,
-      // we cannot deadlock with them, and should let them run as they please.
-      ReleaseStackWalkWorkaroundLock();
-    }
-
 #if V8_HOST_ARCH_X64
     sample->pc = reinterpret_cast<Address>(context.Rip);
     sample->sp = reinterpret_cast<Address>(context.Rsp);

xpcom/threads/HangMonitor.cpp

@@ -16,9 +16,6 @@
 #include "mozilla/UniquePtr.h"
 #include "nsReadableUtils.h"
 #include "mozilla/StackWalk.h"
-#ifdef _WIN64
-#include "mozilla/StackWalk_windows.h"
-#endif
 #include "nsThreadUtils.h"
 #include "nsXULAppAPI.h"
 #include "GeckoProfiler.h"
@@ -151,13 +148,6 @@ GetChromeHangReport(Telemetry::ProcessedStack& aStack,
   std::vector<uintptr_t> rawStack;
   rawStack.reserve(MAX_CALL_STACK_PCS);
 
-  // Workaround possible deadlock where the main thread is running a
-  // long-standing JS job, and happens to be in the JIT allocator when we
-  // suspend it. Since, on win 64, this requires holding a process lock that
-  // MozStackWalk requires, take this "workaround lock" to avoid deadlock.
-#ifdef _WIN64
-  AcquireStackWalkWorkaroundLock();
-#endif
   DWORD ret = ::SuspendThread(winMainThreadHandle);
   bool suspended = false;
   if (ret != -1) {
@@ -171,10 +161,6 @@ GetChromeHangReport(Telemetry::ProcessedStack& aStack,
     }
   }
 
-#ifdef _WIN64
-  ReleaseStackWalkWorkaroundLock();
-#endif
-
   if (!suspended) {
     if (ret != -1) {
       MOZ_ALWAYS_TRUE(::ResumeThread(winMainThreadHandle) != DWORD(-1));