From 5988ddff3b263c583b34217452cef9303314a878 Mon Sep 17 00:00:00 2001
From: Koundinya Veluri <kouvel@microsoft.com>
Date: Mon, 23 Aug 2021 14:13:19 -0700
Subject: [PATCH] Fix frequent FuncEval abort upon hitting a breakpoint in an
 ASP.NET Core web app

- `AssemblySpecBindingCache` uses a cooperative-GC-mode data structure for the cache and operates on the cache from inside a lock taken in preemptive-GC-mode
- So when the cache is being used, cooperative-GC-mode is entered while holding the lock, which can in turn suspend for the debugger. Then a FuncEval that also happens to operate on the cache would deadlock on acquiring the lock and would have to be aborted.
- This seems to be happening very frequently when hitting an early breakpoint in a default new ASP.NET Core web app in .NET 6 when hot reload is enabled
- Fixed by using the same solution that was used for the slot backpatching lock. When cooperative-GC-mode would be entered inside the lock, a different lock holder based on `CrstAndForbidSuspendForDebuggerHolder` is used, which prevents the thread from suspending for the debugger while the lock is held. The thread would instead suspend for the debugger after leaving the forbid region after releasing the lock.
---
 src/coreclr/vm/appdomain.cpp | 20 ++++++++++++++------
 src/coreclr/vm/appdomain.hpp | 24 ++++++++++++++++++++++++
 src/coreclr/vm/crst.cpp      |  4 ++--
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/src/coreclr/vm/appdomain.cpp b/src/coreclr/vm/appdomain.cpp
index 083c33768b818d..a5d2342f8b648d 100644
--- a/src/coreclr/vm/appdomain.cpp
+++ b/src/coreclr/vm/appdomain.cpp
@@ -3566,7 +3566,9 @@ BOOL AppDomain::AddFileToCache(AssemblySpec* pSpec, PEAssembly *pFile, BOOL fAll
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     if(!m_AssemblyCache.StoreFile(pSpec, pFile) && !fAllowFailure)
     {
@@ -3596,7 +3598,9 @@ BOOL AppDomain::AddAssemblyToCache(AssemblySpec* pSpec, DomainAssembly *pAssembl
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     BOOL bRetVal = m_AssemblyCache.StoreAssembly(pSpec, pAssembly);
     return bRetVal;
@@ -3617,7 +3621,9 @@ BOOL AppDomain::AddExceptionToCache(AssemblySpec* pSpec, Exception *ex)
     if (ex->IsTransient())
         return TRUE;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
+
     // !!! suppress exceptions
     return m_AssemblyCache.StoreException(pSpec, ex);
 }
@@ -3635,7 +3641,7 @@ void AppDomain::AddUnmanagedImageToCache(LPCWSTR libraryName, NATIVE_LIBRARY_HAN
     }
     CONTRACTL_END;
 
-    CrstHolder lock(&m_DomainCacheCrst);
+    DomainCacheCrstHolderForGCPreemp lock(this);
 
     const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
     if (existingEntry != NULL)
@@ -3665,7 +3671,8 @@ NATIVE_LIBRARY_HANDLE AppDomain::FindUnmanagedImageInCache(LPCWSTR libraryName)
     }
     CONTRACT_END;
 
-    CrstHolder lock(&m_DomainCacheCrst);
+    DomainCacheCrstHolderForGCPreemp lock(this);
+
     const UnmanagedImageCacheEntry *existingEntry = m_unmanagedCache.LookupPtr(libraryName);
     if (existingEntry == NULL)
         RETURN NULL;
@@ -3707,7 +3714,8 @@ BOOL AppDomain::RemoveAssemblyFromCache(DomainAssembly* pAssembly)
     }
     CONTRACTL_END;
 
-    CrstHolder holder(&m_DomainCacheCrst);
+    GCX_PREEMP();
+    DomainCacheCrstHolderForGCCoop holder(this);
 
     return m_AssemblyCache.RemoveAssembly(pAssembly);
 }
diff --git a/src/coreclr/vm/appdomain.hpp b/src/coreclr/vm/appdomain.hpp
index 6c41b1cfd7d238..447f78345d3288 100644
--- a/src/coreclr/vm/appdomain.hpp
+++ b/src/coreclr/vm/appdomain.hpp
@@ -1164,6 +1164,30 @@ class BaseDomain
     };
     friend class LockHolder;
 
+    // To be used when the thread will remain in preemptive GC mode while holding the lock
+    class DomainCacheCrstHolderForGCPreemp : private CrstHolder
+    {
+    public:
+        DomainCacheCrstHolderForGCPreemp(BaseDomain *pD)
+            : CrstHolder(&pD->m_DomainCacheCrst)
+        {
+            WRAPPER_NO_CONTRACT;
+        }
+    };
+
+    // To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a
+    // forbid-suspend-for-debugger region along with acquiring the lock, such that it would not suspend for the debugger while
+    // holding the lock, as that may otherwise cause a FuncEval to deadlock when trying to acquire the lock.
+    class DomainCacheCrstHolderForGCCoop : private CrstAndForbidSuspendForDebuggerHolder
+    {
+    public:
+        DomainCacheCrstHolderForGCCoop(BaseDomain *pD)
+            : CrstAndForbidSuspendForDebuggerHolder(&pD->m_DomainCacheCrst)
+        {
+            WRAPPER_NO_CONTRACT;
+        }
+    };
+
     class DomainLocalBlockLockHolder : public CrstHolder
     {
     public:
diff --git a/src/coreclr/vm/crst.cpp b/src/coreclr/vm/crst.cpp
index 63472e454e210b..d617647f27233b 100644
--- a/src/coreclr/vm/crst.cpp
+++ b/src/coreclr/vm/crst.cpp
@@ -789,8 +789,8 @@ CrstBase::CrstAndForbidSuspendForDebuggerHolder::CrstAndForbidSuspendForDebugger
     // Reentrant locks are currently not supported
     _ASSERTE((pCrst->m_dwFlags & CRST_REENTRANCY) == 0);
 
-    Thread *pThread = GetThread();
-    if (pThread->IsInForbidSuspendForDebuggerRegion())
+    Thread *pThread = GetThreadNULLOk();
+    if (pThread == nullptr || pThread->IsInForbidSuspendForDebuggerRegion())
     {
         AcquireLock(pCrst);
         return;