Sleep infinitely on secondary crashing threads (#72618)

Sleeping infinitely instead of continuing should be better for crash reporting reliability. Also, change the crashing thread detection to use thread ID instead of Thread*. It is fixing bogus "Fatal error while logging another fatal error." message when the crash occurs without Thread* being setup.
dotnet · Jul 21, 2022 · 5b8d9eb · 5b8d9eb
1 parent 0b1c61e
commit 5b8d9eb
Showing 1 changed file with 22 additions and 17 deletions.
diff --git a/src/coreclr/vm/eepolicy.cpp b/src/coreclr/vm/eepolicy.cpp
@@ -325,25 +325,31 @@ void LogInfoForFatalError(UINT exitCode, LPCWSTR pszMessage, LPCWSTR errorSource
 {
     WRAPPER_NO_CONTRACT;
 
-    static Thread *const FatalErrorNotSeenYet = nullptr;
-    static Thread *const FatalErrorLoggingFinished = reinterpret_cast<Thread *>(1);
+    static size_t s_pCrashingThreadID;
 
-    static Thread *volatile s_pCrashingThread = FatalErrorNotSeenYet;
+    size_t currentThreadID;
+#ifndef TARGET_UNIX
+    currentThreadID = GetCurrentThreadId();
+#else
+    currentThreadID = PAL_GetCurrentOSThreadId();
+#endif
 
-    Thread *pThread = GetThreadNULLOk();
-    Thread *pPreviousThread = InterlockedCompareExchangeT<Thread *>(&s_pCrashingThread, pThread, FatalErrorNotSeenYet);
+    size_t previousThreadID = InterlockedCompareExchangeT<size_t>(&s_pCrashingThreadID, currentThreadID, 0);
 
-    if (pPreviousThread == pThread)
-    {
-        PrintToStdErrA("Fatal error while logging another fatal error.\n");
-        return;
-    }
-    else if (pPreviousThread != nullptr)
+    // Let the first crashing thread take care of the reporting.
+    if (previousThreadID != 0)
     {
-        GCX_PREEMP(); // Avoid blocking other threads that may be trying to suspend the runtime
-        while (s_pCrashingThread != FatalErrorLoggingFinished)
+        if (previousThreadID == currentThreadID)
+        {
+            PrintToStdErrA("Fatal error while logging another fatal error.\n");
+        }
+        else
         {
-            ClrSleepEx(50, /*bAlertable*/ FALSE);
+            // Switch to preemptive mode to avoid blocking the crashing thread. It may try to suspend the runtime
+            // for GC during the stacktrace reporting.
+            GCX_PREEMP();
+
+            ClrSleepEx(INFINITE, /*bAlertable*/ FALSE);
         }
         return;
     }
@@ -379,9 +385,10 @@ void LogInfoForFatalError(UINT exitCode, LPCWSTR pszMessage, LPCWSTR errorSource
 
         PrintToStdErrA("\n");
 
+        Thread* pThread = GetThreadNULLOk();
         if (pThread && errorSource == NULL)
         {
-            LogCallstackForLogWorker(GetThread());
+            LogCallstackForLogWorker(pThread);
 
             if (argExceptionString != NULL) {
                 PrintToStdErrW(argExceptionString);
@@ -392,8 +399,6 @@ void LogInfoForFatalError(UINT exitCode, LPCWSTR pszMessage, LPCWSTR errorSource
     {
     }
     EX_END_CATCH(SwallowAllExceptions)
-
-    InterlockedCompareExchangeT<Thread *>(&s_pCrashingThread, FatalErrorLoggingFinished, pThread);
 }
 
 //This starts FALSE and then converts to true if HandleFatalError has ever been called by a GC thread