Port to 3.1: Fix allocation of RuntimeTypeCache GC handle (#28025)

* Port to 3.1: Fix allocation of RuntimeTypeCache GC handle

When there is a race calling RuntimeType.InitializeCache, each of the
racing threads creates a new GC handle using
new
RuntimeTypeHandle(this).GetGCHandle(GCHandleType.WeakTrackResurrection);
This ends up calling RuntimeTypeHandle::GetGCHandle native method that
adds the allocated handle into the handle cleanup list of the
AssemblyLoaderAllocator specific for the runtime type.
All but the winning thread then call GCHandle.InternalFree on the just
allocated handle. That frees the handle, but leaves it on the cleanup
list of the loader allocator. The same handle can be later allocated for
some
other purpose. When the AssemblyLoaderAllocator is being destroyed, all
the handles on the cleanup list are destroyed too. So it destroys also
the handle that was left on the cleanup list incorrectly. That can cause
all kinds of hard to diagnose issues, like the
dotnet/runtime#32171.

This change fixes it by adding a FreeGCHandle method on the
RuntimeTypeHandle that besides freeing the handle also removes it from
the cleanup list of the related AssemblyLoadContext.

 ## Customer impact
Hard to diagnose crashes in the runtime caused by closing of random
GC handles. The customer that has reported this issue was using
collectible assemblies and it was resulting in collecting
LoaderAllocator that was still in use and it lead to crashes at various
places.

 ## Regression?
Yes, it was introduced in 3.0. In 2.1 and 2.2, the thread that loses
the race destroys the handle only if the type was not in a collectible
assembly. Since the non-collectible assemblies LoaderAllocator is never
destroyed, the handles were never cleaned up and so no problem could occur.
It was introduced in #21737

 ##Testing
Customer affected by the issue heavily has tested a fixed version and
reported the issue doesn't occur anymore.

 ## Risk
Low, the new code is executed at single place once per process runtine
only when a thread races for allocating the GC handle with another one
and loses the race.

* Fix build break - subtle differences from runtime master

Author: janvorli

src/System.Private.CoreLib/src/System/RuntimeHandles.cs

@@ -339,6 +339,15 @@ internal IntPtr GetGCHandle(GCHandleType type)
             return GetGCHandle(JitHelpers.GetQCallTypeHandleOnStack(ref nativeHandle), type);
         }
 
+        [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
+        private static extern IntPtr FreeGCHandle(QCallTypeHandle typeHandle, IntPtr objHandle);
+
+        internal IntPtr FreeGCHandle(IntPtr objHandle)
+        {
+            RuntimeTypeHandle nativeHandle = GetNativeHandle();
+            return FreeGCHandle(JitHelpers.GetQCallTypeHandleOnStack(ref nativeHandle), objHandle);
+        }
+
         [MethodImplAttribute(MethodImplOptions.InternalCall)]
         internal static extern int GetNumVirtuals(RuntimeType type);
 

src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

@@ -2414,10 +2414,11 @@ private RuntimeTypeCache InitializeCache()
         {
             if (m_cache == IntPtr.Zero)
             {
-                IntPtr newgcHandle = new RuntimeTypeHandle(this).GetGCHandle(GCHandleType.WeakTrackResurrection);
+                RuntimeTypeHandle th = new RuntimeTypeHandle(this);
+                IntPtr newgcHandle = th.GetGCHandle(GCHandleType.WeakTrackResurrection);
                 IntPtr gcHandle = Interlocked.CompareExchange(ref m_cache, newgcHandle, IntPtr.Zero);
                 if (gcHandle != IntPtr.Zero)
-                    GCHandle.InternalFree(newgcHandle);
+                    th.FreeGCHandle(newgcHandle);
             }
 
             RuntimeTypeCache cache = (RuntimeTypeCache)GCHandle.InternalGet(m_cache);

src/vm/ecalllist.h

@@ -225,6 +225,7 @@ FCFuncStart(gCOMTypeHandleFuncs)
     FCFuncElement("CreateCaInstance", RuntimeTypeHandle::CreateCaInstance)
     FCFuncElement("CreateInstanceForAnotherGenericParameter", RuntimeTypeHandle::CreateInstanceForGenericType)
     QCFuncElement("GetGCHandle", RuntimeTypeHandle::GetGCHandle)
+    QCFuncElement("FreeGCHandle", RuntimeTypeHandle::FreeGCHandle)
 
     FCFuncElement("IsInstanceOfType", RuntimeTypeHandle::IsInstanceOfType)
     FCFuncElement("GetDeclaringMethod", RuntimeTypeHandle::GetDeclaringMethod)

src/vm/loaderallocator.cpp

@@ -1838,6 +1838,31 @@ void AssemblyLoaderAllocator::RegisterHandleForCleanup(OBJECTHANDLE objHandle)
     m_handleCleanupList.InsertTail(new (pItem) HandleCleanupListItem(objHandle));
 }
 
+void AssemblyLoaderAllocator::UnregisterHandleFromCleanup(OBJECTHANDLE objHandle)
+{
+    CONTRACTL
+    {
+        MODE_ANY;
+        CAN_TAKE_LOCK;
+        PRECONDITION(CheckPointer(objHandle));
+    }
+    CONTRACTL_END;
+
+    // FindAndRemove must be protected by a lock. Just use the loader allocator lock
+    CrstHolder ch(&m_crstLoaderAllocator);
+
+    for (HandleCleanupListItem* item = m_handleCleanupList.GetHead(); item != NULL; item = SList<HandleCleanupListItem>::GetNext(item))
+    {
+        if (item->m_handle == objHandle)
+        {
+            m_handleCleanupList.FindAndRemove(item);
+            return;
+        }
+    }
+
+    _ASSERTE(!"Trying to unregister a handle that was never registered");
+}
+
 void AssemblyLoaderAllocator::CleanupHandles()
 {
     CONTRACTL

src/vm/loaderallocator.hpp

@@ -467,6 +467,7 @@ class LoaderAllocator
 
     // The default implementation is a no-op. Only collectible loader allocators implement this method.
     virtual void RegisterHandleForCleanup(OBJECTHANDLE /* objHandle */) { }
+    virtual void UnregisterHandleFromCleanup(OBJECTHANDLE /* objHandle */) { }
     virtual void CleanupHandles() { }
 
     void RegisterFailedTypeInitForCleanup(ListLockEntry *pListLockEntry);
@@ -671,6 +672,7 @@ class AssemblyLoaderAllocator : public LoaderAllocator
 
 #if !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)
     virtual void RegisterHandleForCleanup(OBJECTHANDLE objHandle);
+    virtual void UnregisterHandleFromCleanup(OBJECTHANDLE /* objHandle */);
     virtual void CleanupHandles();
     CLRPrivBinderAssemblyLoadContext* GetBinder()
     {

src/vm/runtimehandles.cpp

@@ -1109,6 +1109,21 @@ PVOID QCALLTYPE RuntimeTypeHandle::GetGCHandle(QCall::TypeHandle pTypeHandle, IN
     return objHandle;
 }
 
+void QCALLTYPE RuntimeTypeHandle::FreeGCHandle(QCall::TypeHandle pTypeHandle, OBJECTHANDLE objHandle)
+{
+    QCALL_CONTRACT;
+
+    BEGIN_QCALL;
+
+    GCX_COOP();
+
+    TypeHandle th = pTypeHandle.AsTypeHandle();
+    th.GetLoaderAllocator()->UnregisterHandleFromCleanup(objHandle);
+    DestroyTypedHandle(objHandle);
+
+    END_QCALL;
+}
+
 void QCALLTYPE RuntimeTypeHandle::VerifyInterfaceIsImplemented(QCall::TypeHandle pTypeHandle, QCall::TypeHandle pIFaceHandle)
 {
     QCALL_CONTRACT;

src/vm/runtimehandles.h

@@ -241,6 +241,9 @@ class RuntimeTypeHandle {
     static
     PVOID QCALLTYPE GetGCHandle(QCall::TypeHandle pTypeHandle, INT32 handleType);
 
+    static
+    void QCALLTYPE FreeGCHandle(QCall::TypeHandle pTypeHandle, OBJECTHANDLE objHandle);
+
     static FCDECL1(INT32, GetCorElementType, PTR_ReflectClassBaseObject pType);
     static FCDECL1(ReflectClassBaseObject*, GetElementType, ReflectClassBaseObject* pType);