From 4bdaa8e2fb06b1065d0e418d21abd80dfff00a8f Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Tue, 25 May 2021 08:15:39 -0700 Subject: [PATCH] Cleanup internal ComWrappers cache when object enters Finalization queue (#52771) * Clean up the ExternalObjectContext cache for ComWrappers' RCWs. * Add testing for verifying internal cache clean up. --- src/coreclr/interop/comwrappers.cpp | 5 +- src/coreclr/vm/gcenv.ee.cpp | 6 +-- src/coreclr/vm/interoplibinterface.cpp | 68 ++++++++++++++++++++++++ src/coreclr/vm/interoplibinterface.h | 11 +++- src/coreclr/vm/interoplibinterface_shared.cpp | 19 +++++-- src/tests/Interop/COM/ComWrappers/API/Program.cs | 51 ++++++++++++++++++ src/tests/Interop/COM/ComWrappers/Common.cs | 11 +++- 7 files changed, 158 insertions(+), 13 deletions(-) diff --git a/src/coreclr/interop/comwrappers.cpp b/src/coreclr/interop/comwrappers.cpp index 8485125..90c366c 100644 --- a/src/coreclr/interop/comwrappers.cpp +++ b/src/coreclr/interop/comwrappers.cpp @@ -357,7 +357,8 @@ ManagedObjectWrapper* ManagedObjectWrapper::MapFromIUnknown(_In_ IUnknown* pUnk) // If the first Vtable entry is part of the ManagedObjectWrapper IUnknown impl, // we know how to interpret the IUnknown. void** vtable = *reinterpret_cast(pUnk); - if (*vtable != ManagedObjectWrapper_IUnknownImpl.QueryInterface) + if (*vtable != ManagedObjectWrapper_IUnknownImpl.QueryInterface + && *vtable != ManagedObjectWrapper_IReferenceTrackerTargetImpl.QueryInterface) return nullptr; ABI::ComInterfaceDispatch* disp = reinterpret_cast(pUnk); @@ -841,7 +842,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. diff --git a/src/coreclr/vm/gcenv.ee.cpp b/src/coreclr/vm/gcenv.ee.cpp index 650e912..4df3e3a 100644 --- a/src/coreclr/vm/gcenv.ee.cpp +++ b/src/coreclr/vm/gcenv.ee.cpp @@ -66,8 +66,7 @@ void GCToEEInterface::BeforeGcScanRoots(int condemned, bool is_bgc, bool is_conc } #endif // VERIFY_HEAP - if (!is_concurrent) - Interop::OnBeforeGCScanRoots(); + Interop::OnBeforeGCScanRoots(is_concurrent); } //EE can perform post stack scanning action, while the @@ -88,8 +87,7 @@ VOID GCToEEInterface::AfterGcScanRoots (int condemned, int max_gen, ::GetAppDomain()->DetachRCWs(); #endif // FEATURE_COMINTEROP - if (!sc->concurrent) - Interop::OnAfterGCScanRoots(); + Interop::OnAfterGCScanRoots(sc->concurrent); } /* diff --git a/src/coreclr/vm/interoplibinterface.cpp b/src/coreclr/vm/interoplibinterface.cpp index f8804e3..b027d6b 100644 --- a/src/coreclr/vm/interoplibinterface.cpp +++ b/src/coreclr/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 @@ -428,6 +446,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 @@ -727,6 +771,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); @@ -1796,4 +1849,19 @@ void ComWrappersNative::OnFullGCFinished() } } +void ComWrappersNative::AfterRefCountedHandleCallbacks() +{ + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + ExtObjCxtCache* cache = ExtObjCxtCache::GetInstanceNoThrow(); + if (cache != NULL) + cache->DetachNotPromotedEOCs(); +} + #endif // FEATURE_COMWRAPPERS diff --git a/src/coreclr/vm/interoplibinterface.h b/src/coreclr/vm/interoplibinterface.h index 28139db..367b419 100644 --- a/src/coreclr/vm/interoplibinterface.h +++ b/src/coreclr/vm/interoplibinterface.h @@ -63,6 +63,7 @@ public: // Unwrapping support public: // GC interaction static void OnFullGCStarted(); static void OnFullGCFinished(); + static void AfterRefCountedHandleCallbacks(); }; class GlobalComWrappersForMarshalling @@ -181,13 +182,19 @@ public: _Outptr_ void** context); // Notify started/finished when GC is running. + // These methods are called in a blocking fashion when a + // GC of generation is started. These calls may overlap + // so care must be taken when taking locks. static void OnGCStarted(_In_ int nCondemnedGeneration); static void OnGCFinished(_In_ int nCondemnedGeneration); // Notify before/after when GC is scanning roots. // Present assumption is that calls will never be nested. - static void OnBeforeGCScanRoots(); - static void OnAfterGCScanRoots(); + // The input indicates if the call is from a concurrent GC + // thread or not. These will be nested within OnGCStarted + // and OnGCFinished. + static void OnBeforeGCScanRoots(_In_ bool isConcurrent); + static void OnAfterGCScanRoots(_In_ bool isConcurrent); }; #endif // _INTEROPLIBINTERFACE_H_ diff --git a/src/coreclr/vm/interoplibinterface_shared.cpp b/src/coreclr/vm/interoplibinterface_shared.cpp index 19ee32f..f31b128 100644 --- a/src/coreclr/vm/interoplibinterface_shared.cpp +++ b/src/coreclr/vm/interoplibinterface_shared.cpp @@ -116,7 +116,7 @@ void Interop::OnGCFinished(_In_ int nCondemnedGeneration) } } -void Interop::OnBeforeGCScanRoots() +void Interop::OnBeforeGCScanRoots(_In_ bool isConcurrent) { CONTRACTL { @@ -126,11 +126,16 @@ void Interop::OnBeforeGCScanRoots() CONTRACTL_END; #ifdef FEATURE_OBJCMARSHAL - ObjCMarshalNative::BeforeRefCountedHandleCallbacks(); + // The Objective-C interop begin/end for reference counted + // handles only occurs in non-concurrent scenarios. This contract + // is because of the potential for locking as a synchronization + // mechanism for Objective-C lifetime management. + if (!isConcurrent) + ObjCMarshalNative::BeforeRefCountedHandleCallbacks(); #endif // FEATURE_OBJCMARSHAL } -void Interop::OnAfterGCScanRoots() +void Interop::OnAfterGCScanRoots(_In_ bool isConcurrent) { CONTRACTL { @@ -139,7 +144,13 @@ void Interop::OnAfterGCScanRoots() } CONTRACTL_END; +#ifdef FEATURE_COMWRAPPERS + ComWrappersNative::AfterRefCountedHandleCallbacks(); +#endif // FEATURE_COMWRAPPERS + #ifdef FEATURE_OBJCMARSHAL - ObjCMarshalNative::AfterRefCountedHandleCallbacks(); + // See Interop::OnBeforeGCScanRoots for why non-concurrent. + if (!isConcurrent) + ObjCMarshalNative::AfterRefCountedHandleCallbacks(); #endif // FEATURE_OBJCMARSHAL } diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index b934ac9..1a779a7 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -280,6 +280,56 @@ namespace ComWrappersTests }); } + 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 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 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(nativeWrapper, trackResurrection: true); + } + } + static void ValidateSuppliedInnerNotAggregation() { Console.WriteLine($"Running {nameof(ValidateSuppliedInnerNotAggregation)}..."); @@ -566,6 +616,7 @@ namespace ComWrappersTests ValidateCreateObjectCachingScenario(); ValidateWrappersInstanceIsolation(); ValidatePrecreatedExternalWrapper(); + ValidateExternalWrapperCacheCleanUp(); ValidateSuppliedInnerNotAggregation(); ValidateIUnknownImpls(); ValidateBadComWrapperImpl(); diff --git a/src/tests/Interop/COM/ComWrappers/Common.cs b/src/tests/Interop/COM/ComWrappers/Common.cs index 3e505c1..d100744 100644 --- a/src/tests/Interop/COM/ComWrappers/Common.cs +++ b/src/tests/Interop/COM/ComWrappers/Common.cs @@ -213,9 +213,18 @@ namespace ComWrappersTests.Common ~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; -- 2.7.4