Fix assertion/possible deadlock in thread shutdown in the presence of ThreadStatics on collectible assemblies

src/coreclr/vm/loaderallocator.cpp

@@ -769,7 +769,7 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value)
             do
             {
                 {
-                    CrstHolder ch(&m_crstLoaderAllocator);
+                    CrstHolder ch(&m_crstLoaderAllocatorHandleTable);
 
                     gc.handleTable = gc.loaderAllocator->GetHandleTable();
 
@@ -781,7 +781,7 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value)
                         retVal = (UINT_PTR)((freeHandleIndex + 1) << 1);
                         break;
                     }
-
+                    
                     slotsUsed = gc.loaderAllocator->GetSlotsUsed();
 
                     if (slotsUsed > MAX_LOADERALLOCATOR_HANDLE)
@@ -808,7 +808,7 @@ LOADERHANDLE LoaderAllocator::AllocateHandle(OBJECTREF value)
                 gc.handleTable = (PTRARRAYREF)AllocateObjectArray(newSize, g_pObjectClass);
 
                 {
-                    CrstHolder ch(&m_crstLoaderAllocator);
+                    CrstHolder ch(&m_crstLoaderAllocatorHandleTable);
 
                     if (gc.loaderAllocator->GetHandleTable() == gc.handleTableOld)
                     {
@@ -886,7 +886,7 @@ void LoaderAllocator::FreeHandle(LOADERHANDLE handle)
         // The slot value doesn't have the low bit set, so it is an index to the handle table.
         // In this case, push the index of the handle to the stack of freed indexes for
         // reuse.
-        CrstHolder ch(&m_crstLoaderAllocator);
+        CrstHolder ch(&m_crstLoaderAllocatorHandleTable);
 
         UINT_PTR index = (((UINT_PTR)handle) >> 1) - 1;
         // The Push can fail due to OOM. Ignore this failure, it is better than crashing. The
@@ -937,7 +937,7 @@ OBJECTREF LoaderAllocator::CompareExchangeValueInHandle(LOADERHANDLE handle, OBJ
     else
     {
         /* The handle table is read locklessly, be careful */
-        CrstHolder ch(&m_crstLoaderAllocator);
+        CrstHolder ch(&m_crstLoaderAllocatorHandleTable);
 
         _ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle));
 
@@ -983,7 +983,7 @@ void LoaderAllocator::SetHandleValue(LOADERHANDLE handle, OBJECTREF value)
     else
     {
         // The handle table is read locklessly, be careful
-        CrstHolder ch(&m_crstLoaderAllocator);
+        CrstHolder ch(&m_crstLoaderAllocatorHandleTable);
 
         _ASSERTE(!ObjectHandleIsNull(m_hLoaderAllocatorObjectHandle));
 
@@ -1065,6 +1065,7 @@ void LoaderAllocator::Init(BaseDomain *pDomain, BYTE *pExecutableHeapMemory)
     m_pDomain = pDomain;
 
     m_crstLoaderAllocator.Init(CrstLoaderAllocator, (CrstFlags)CRST_UNSAFE_COOPGC);
+    m_crstLoaderAllocatorHandleTable.Init(CrstLeafLock, (CrstFlags)CRST_UNSAFE_COOPGC);
     m_InteropDataCrst.Init(CrstInteropData, CRST_REENTRANCY);
 #ifdef FEATURE_COMINTEROP
     m_ComCallWrapperCrst.Init(CrstCOMCallWrapper);

src/coreclr/vm/loaderallocator.hpp

@@ -324,6 +324,9 @@ class LoaderAllocator
     // The LoaderAllocator specific string literal map.
     StringLiteralMap   *m_pStringLiteralMap;
     CrstExplicitInit    m_crstLoaderAllocator;
+
+    // Protect the handle table data structures, seperated from m_crstLoaderAllocator to allow thread cleanup to use the lock
+    CrstExplicitInit    m_crstLoaderAllocatorHandleTable;
     bool                m_fGCPressure;
     bool                m_fUnloaded;
     bool                m_fTerminated;

src/coreclr/vm/object.h

@@ -2081,6 +2081,7 @@ class LoaderAllocatorObject : public Object
     friend class CoreLibBinder;
 
 public:
+    // All uses of this must be protected by m_crstLoaderAllocatorHandleTable or be safe lock-free reads
     PTRARRAYREF GetHandleTable()
     {
         LIMITED_METHOD_DAC_CONTRACT;
@@ -2099,6 +2100,7 @@ class LoaderAllocatorObject : public Object
         return m_slotsUsed;
     }
 
+    // All uses of this must be protected by m_crstLoaderAllocatorHandleTable
     void SetSlotsUsed(INT32 newSlotsUsed)
     {
         LIMITED_METHOD_CONTRACT;

src/tests/Loader/CollectibleAssemblies/Statics/CollectibleStatics.cs

@@ -8,6 +8,7 @@
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Runtime.Loader;
+using System.Threading;
 using Xunit;
 
 public class Program
@@ -65,6 +66,48 @@ public static int TestEntryPoint()
         if (val5Obj != obj5)
             return 15;
 
+        int otherThreadResult = 0;
+        Thread t = new ((ThreadStart)delegate {
+
+            object obj1 = new object();
+            object obj2 = new object();
+            object obj3 = new object();
+            object obj4 = new object();
+            object obj5 = new object();
+            accessor.SetStaticObject(obj1, obj2, obj3, obj4, obj5);
+            accessor.GetStaticObject(out object val1Obj, out object val2Obj, out object val3Obj, out object val4Obj, out object val5Obj);
+            if (val1Obj != obj1)
+            {
+                otherThreadResult = 111;
+                return;
+            }
+            if (val2Obj != obj2)
+            {
+                otherThreadResult = 112;
+                return;
+            }
+            if (val3Obj != obj3)
+            {
+                otherThreadResult = 113;
+                return;
+            }
+            if (val4Obj != obj4)
+            {
+                otherThreadResult = 114;
+                return;
+            }
+            if (val5Obj != obj5)
+            {
+                otherThreadResult = 115;
+                return;
+            }
+        });
+
+        t.Start();
+        t.Join();
+        if (otherThreadResult != 0)
+            return otherThreadResult;
+
         GC.KeepAlive(accessor);
         return 100;
     }