[Release/5.0] - Fix internal cache clean up for ComWrappers (#53203)

* Cleanup internal ComWrappers cache when object enters Finalization queue

* Object cleanup scenario.

* Narrow usage to the COMINTEROP feature.

* Feedback from main review

Author: AaronRobinsonMSFT

src/coreclr/src/interop/comwrappers.cpp

@@ -856,7 +856,7 @@ void* NativeObjectWrapperContext::GetRuntimeContext() const noexcept
 
 IReferenceTracker* NativeObjectWrapperContext::GetReferenceTracker() const noexcept
 {
-    return ((_trackerObjectState == TrackerObjectState_NotSet) ? nullptr : _trackerObject);
+    return ((_trackerObjectState == TrackerObjectState_NotSet || _trackerObjectDisconnected) ? nullptr : _trackerObject);
 }
 
 // See TrackerObjectManager::AfterWrapperCreated() for AddRefFromTrackerSource() usage.

src/coreclr/src/vm/gcenv.ee.cpp

@@ -65,6 +65,8 @@ VOID GCToEEInterface::AfterGcScanRoots (int condemned, int max_gen,
     // Go through all the only app domain and detach all the *unmarked* RCWs to prevent
     // the RCW cache from resurrecting them.
     ::GetAppDomain()->DetachRCWs();
+
+    Interop::OnAfterGCScanRoots();
 #endif // FEATURE_COMINTEROP
 }
 

src/coreclr/src/vm/interoplibinterface.cpp

@@ -30,9 +30,16 @@ namespace
         enum
         {
             Flags_None = 0,
+
+            // The EOC has been collected and is no longer visible from managed code.
             Flags_Collected = 1,
+
             Flags_ReferenceTracker = 2,
             Flags_InCache = 4,
+
+            // The EOC is "detached" and no longer used to map between identity and a managed object.
+            // This will only be set if the EOC was inserted into the cache.
+            Flags_Detached = 8,
         };
         DWORD Flags;
 
@@ -80,6 +87,17 @@ namespace
             Flags |= Flags_Collected;
         }
 
+        void MarkDetached()
+        {
+            _ASSERTE(GCHeapUtilities::IsGCInProgress());
+            Flags |= Flags_Detached;
+        }
+
+        void MarkNotInCache()
+        {
+            ::InterlockedAnd((LONG*)&Flags, (~Flags_InCache));
+        }
+
         OBJECTREF GetObjectRef()
         {
             CONTRACTL
@@ -432,6 +450,32 @@ namespace
 
             _hashMap.Remove(cxt->GetKey());
         }
+
+        void DetachNotPromotedEOCs()
+        {
+            CONTRACTL
+            {
+                NOTHROW;
+                GC_NOTRIGGER;
+                MODE_ANY;
+                PRECONDITION(GCHeapUtilities::IsGCInProgress()); // GC is in progress and the runtime is suspended
+            }
+            CONTRACTL_END;
+
+            Iterator curr = _hashMap.Begin();
+            Iterator end = _hashMap.End();
+
+            ExternalObjectContext* cxt;
+            for (; curr != end; ++curr)
+            {
+                cxt = *curr;
+                if (!cxt->IsSet(ExternalObjectContext::Flags_Detached)
+                    && !GCHeapUtilities::GetGCHeap()->IsPromoted(OBJECTREFToObject(cxt->GetObjectRef())))
+                {
+                    cxt->MarkDetached();
+                }
+            }
+        }
     };
 
     // Global instance of the external object cache
@@ -731,6 +775,15 @@ namespace
                     handle = handleLocal;
                 }
             }
+            else if (extObjCxt != NULL && extObjCxt->IsSet(ExternalObjectContext::Flags_Detached))
+            {
+                // If an EOC has been found but is marked detached, then we will remove it from the
+                // cache here instead of letting the GC do it later and pretend like it wasn't found.
+                STRESS_LOG1(LF_INTEROP, LL_INFO10, "Detached EOC requested: 0x%p\n", extObjCxt);
+                cache->Remove(extObjCxt);
+                extObjCxt->MarkNotInCache();
+                extObjCxt = NULL;
+            }
         }
 
         STRESS_LOG2(LF_INTEROP, LL_INFO1000, "EOC: 0x%p or Handle: 0x%p\n", extObjCxt, handle);
@@ -1827,3 +1880,19 @@ void Interop::OnGCFinished(_In_ int nCondemnedGeneration)
     }
 #endif // FEATURE_COMWRAPPERS
 }
+
+void Interop::OnAfterGCScanRoots()
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+    }
+    CONTRACTL_END;
+
+#ifdef FEATURE_COMWRAPPERS
+    ExtObjCxtCache* cache = ExtObjCxtCache::GetInstanceNoThrow();
+    if (cache != NULL)
+        cache->DetachNotPromotedEOCs();
+#endif // FEATURE_COMWRAPPERS
+}

src/coreclr/src/vm/interoplibinterface.h

@@ -99,4 +99,6 @@ class Interop
 
     // Notify when GC finished
     static void OnGCFinished(_In_ int nCondemnedGeneration);
+
+    static void OnAfterGCScanRoots();
 };

src/tests/Interop/COM/ComWrappers/API/Program.cs

@@ -261,6 +261,56 @@ static void ValidatePrecreatedExternalWrapper()
                 });
         }
 
+        static void ValidateExternalWrapperCacheCleanUp()
+        {
+            Console.WriteLine($"Running {nameof(ValidateExternalWrapperCacheCleanUp)}...");
+
+            var cw = new TestComWrappers();
+
+            // Get an object from a tracker runtime.
+            IntPtr trackerObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject();
+
+            // Create a wrapper for the object instance.
+            var weakRef1 = CreateAndRegisterWrapper(cw, trackerObjRaw);
+
+            // Run the GC to have the wrapper marked for collection.
+            ForceGC();
+
+            // Create a new wrapper for the same external object.
+            var weakRef2 = CreateAndRegisterWrapper(cw, trackerObjRaw);
+
+            // We are using a tracking resurrection WeakReference<T> so we should be able
+            // to get back the objects as they are all continually re-registering for Finalization.
+            Assert.IsTrue(weakRef1.TryGetTarget(out ITrackerObjectWrapper wrapper1));
+            Assert.IsTrue(weakRef2.TryGetTarget(out ITrackerObjectWrapper wrapper2));
+
+            // Check that the two wrappers aren't equal, meaning we created a new wrapper since
+            // the first wrapper was removed from the internal cache.
+            Assert.AreNotEqual(wrapper1, wrapper2);
+
+            // Let the wrappers Finalize.
+            wrapper1.ReregisterForFinalize = false;
+            wrapper2.ReregisterForFinalize = false;
+
+            static WeakReference<ITrackerObjectWrapper> CreateAndRegisterWrapper(ComWrappers cw, IntPtr trackerObjRaw)
+            {
+                // Manually create a wrapper
+                var iid = typeof(ITrackerObject).GUID;
+                IntPtr iTestComObject;
+                int hr = Marshal.QueryInterface(trackerObjRaw, ref iid, out iTestComObject);
+                Assert.AreEqual(0, hr);
+                var nativeWrapper = new ITrackerObjectWrapper(iTestComObject);
+
+                nativeWrapper = (ITrackerObjectWrapper)cw.GetOrRegisterObjectForComInstance(trackerObjRaw, CreateObjectFlags.None, nativeWrapper);
+
+                // Set this on the return instead of during creation since the returned wrapper may be the one from
+                // the internal cache and not the one passed in above.
+                nativeWrapper.ReregisterForFinalize = true;
+
+                return new WeakReference<ITrackerObjectWrapper>(nativeWrapper, trackResurrection: true);
+            }
+        }
+
         static void ValidateIUnknownImpls()
             => TestComWrappers.ValidateIUnknownImpls();
 
@@ -477,6 +527,7 @@ static int Main(string[] doNotUse)
                 ValidateCreateObjectCachingScenario();
                 ValidateWrappersInstanceIsolation();
                 ValidatePrecreatedExternalWrapper();
+                ValidateExternalWrapperCacheCleanUp();
                 ValidateIUnknownImpls();
                 ValidateBadComWrapperImpl();
                 ValidateRuntimeTrackerScenario();

src/tests/Interop/COM/ComWrappers/Common.cs

@@ -206,9 +206,18 @@ static IntPtr CreateInstance(IntPtr outer, out IntPtr inner)
 
         ~ITrackerObjectWrapper()
         {
-            ComWrappersHelper.Cleanup(ref this.classNative);
+            if (this.ReregisterForFinalize)
+            {
+                GC.ReRegisterForFinalize(this);
+            }
+            else
+            {
+                ComWrappersHelper.Cleanup(ref this.classNative);
+            }
         }
 
+        public bool ReregisterForFinalize { get; set; } = false;
+
         public int AddObjectRef(IntPtr obj)
         {
             int id;