diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 0b268cc6e12bee..5a94a338c06588 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -449,6 +449,15 @@ internal static unsafe bool ObjectHasComponentSize(object obj) return GetMethodTable(obj)->HasComponentSize; } + // Returns true iff the type of the object requires finalization, + // which includes a finalizer inherited from a base type. + internal static unsafe bool ObjectHasFinalizer(object obj) + { + bool hasFinalizer = GetMethodTable(obj)->HasFinalizer; + GC.KeepAlive(obj); // Keep MethodTable alive + return hasFinalizer; + } + /// /// Boxes a given value using an input to determine its type. /// diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs index 487c460e1c1b23..93d0d56a4f94b0 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.NativeAot.cs @@ -213,6 +213,13 @@ internal static unsafe bool ObjectHasComponentSize(object obj) return GetMethodTable(obj)->HasComponentSize; } + // Returns true iff the type of the object requires finalization, + // which includes a finalizer inherited from a base type. + internal static unsafe bool ObjectHasFinalizer(object obj) + { + return GetMethodTable(obj)->IsFinalizable; + } + public static void PrepareMethod(RuntimeMethodHandle method) { if (method.Value == IntPtr.Zero) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs index 7bc461cef8d54c..f6fa0b09c2aaf3 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs @@ -186,7 +186,8 @@ internal static void DetachNonPromotedObjects() ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); if (nativeObjectWrapper != null && nativeObjectWrapper.TrackerObject != IntPtr.Zero && - !RuntimeImports.RhIsPromoted(nativeObjectWrapper.ProxyHandle.Target)) + nativeObjectWrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && + !RuntimeImports.RhIsPromoted(proxyTarget)) { // Notify the wrapper it was not promoted and is being collected. BeforeWrapperFinalized(nativeObjectWrapper.TrackerObject); @@ -205,9 +206,9 @@ internal static unsafe class FindReferenceTargetsCallback internal ref struct Instance { private readonly IntPtr _vtable; // First field is IUnknown based vtable. - public GCHandle RootObject; + public WeakGCHandle RootObject; - public Instance(GCHandle handle) + public Instance(WeakGCHandle handle) { _vtable = (IntPtr)Unsafe.AsPointer(in FindReferenceTargetsCallback.Vftbl); RootObject = handle; @@ -240,7 +241,11 @@ private static unsafe int IFindReferenceTargetsCallback_FoundTrackerTarget(IntPt return HResults.E_POINTER; } - object sourceObject = ((FindReferenceTargetsCallback.Instance*)pThis)->RootObject.Target!; + _ = ((FindReferenceTargetsCallback.Instance*)pThis)->RootObject.TryGetTarget(out object? sourceObject); + + // The callback is only ever set up with the handle of an RCW that was alive at the time, and + // that RCW keeps its wrapper alive, so the handle is expected to still have its target here + Debug.Assert(sourceObject is not null); if (!TryGetObject(referenceTrackerTarget, out object? targetObject)) { @@ -253,7 +258,7 @@ private static unsafe int IFindReferenceTargetsCallback_FoundTrackerTarget(IntPt } // Notify the runtime a reference path was found. - return TrackerObjectManager.AddReferencePath(sourceObject, targetObject) ? HResults.S_OK : HResults.S_FALSE; + return TrackerObjectManager.AddReferencePath(sourceObject!, targetObject) ? HResults.S_OK : HResults.S_FALSE; } internal struct ReferenceTargetsVftbl diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs index 84c316b880390a..8362b47016ee07 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs @@ -548,8 +548,8 @@ internal unsafe class NativeObjectWrapper private ComWrappers _comWrappers; private IntPtr _externalComObject; private IntPtr _inner; - private GCHandle _proxyHandle; - private GCHandle _proxyHandleTrackingResurrection; + private WeakGCHandle _proxyHandle; + private WeakGCHandle _proxyHandleTrackingResurrection; private readonly bool _aggregatedManagedObjectWrapper; private readonly bool _uniqueInstance; @@ -594,14 +594,31 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper _inner = inner; _comWrappers = comWrappers; _uniqueInstance = flags.HasFlag(CreateObjectFlags.UniqueInstance); - _proxyHandle = GCHandle.Alloc(comProxy, GCHandleType.Weak); - // We have a separate handle tracking resurrection as we want to make sure - // we clean up the NativeObjectWrapper only after the RCW has been finalized - // due to it can access the native object in the finalizer. At the same time, - // we want other callers which are using ProxyHandle such as the reference tracker runtime - // to see the object as not alive once it is eligible for finalization. - _proxyHandleTrackingResurrection = GCHandle.Alloc(comProxy, GCHandleType.WeakTrackResurrection); + // The wrapper's finalizer must not release anything while the RCW is still able to observe the + // native object, which is why a handle that tracks resurrection is needed: unlike a plain weak + // handle, it stays set until the RCW has actually been collected, rather than merely becoming + // unreachable. Callers such as the reference tracker runtime want the opposite, and need to see + // the RCW as gone as soon as it is eligible for finalization, which is what 'ProxyHandle' is for. + // + // Those two only disagree while the RCW is unreachable but not yet collected. An RCW that has + // no finalizer is never in that state on its own account, so a single handle can serve both + // purposes, halving the handles every such RCW costs. It can still be put in that state by + // something else's finalizer holding on to it, and then resurrecting it, and in that case having + // the one handle track resurrection is what keeps this wrapper from tearing down state the + // resurrected RCW still needs. Reporting such an RCW as alive is also the honest answer, as it + // may well be about to become reachable again. + // + // An RCW that does have a finalizer, whether its own or an inherited one, does reach that state + // on its own, and there the two meanings genuinely differ, so it pays for both handles. + bool proxyHasFinalizer = RuntimeHelpers.ObjectHasFinalizer(comProxy); + + _proxyHandle = new WeakGCHandle(comProxy, trackResurrection: !proxyHasFinalizer); + + if (proxyHasFinalizer) + { + _proxyHandleTrackingResurrection = new WeakGCHandle(comProxy, trackResurrection: true); + } // If this is an aggregation scenario and the identity object // is a managed object wrapper, we need to call Release() to @@ -617,7 +634,7 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper internal IntPtr ExternalComObject => _externalComObject; internal ComWrappers ComWrappers => _comWrappers; - internal GCHandle ProxyHandle => _proxyHandle; + internal WeakGCHandle ProxyHandle => _proxyHandle; internal bool IsUniqueInstance => _uniqueInstance; internal bool IsAggregatedWithManagedObjectWrapper => _aggregatedManagedObjectWrapper; @@ -629,15 +646,8 @@ public virtual void Release() _comWrappers = null!; } - if (_proxyHandle.IsAllocated) - { - _proxyHandle.Free(); - } - - if (_proxyHandleTrackingResurrection.IsAllocated) - { - _proxyHandleTrackingResurrection.Free(); - } + _proxyHandle.Dispose(); + _proxyHandleTrackingResurrection.Dispose(); // If the inner was supplied, we need to release our reference. if (_inner != IntPtr.Zero) @@ -651,7 +661,15 @@ public virtual void Release() ~NativeObjectWrapper() { - if (_proxyHandleTrackingResurrection.IsAllocated && _proxyHandleTrackingResurrection.Target != null) + // When the RCW has no finalizer, no second handle was allocated and the proxy handle is + // the one tracking resurrection, so it answers this question just as well. Neither is allocated + // once this wrapper has been released, which happens eagerly when one loses a registration race, + // and then there is nothing left to keep alive for. + WeakGCHandle resurrectionHandle = _proxyHandleTrackingResurrection.IsAllocated + ? _proxyHandleTrackingResurrection + : _proxyHandle; + + if (resurrectionHandle.IsAllocated && resurrectionHandle.TryGetTarget(out _)) { // The RCW object has not been fully collected, so it still // can make calls on the native object in its finalizer. @@ -1261,7 +1279,7 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro // for the same COM instance, but in that case we'll be passed the same NativeObjectWrapper instance // for both threads. In that case, it doesn't matter which thread adds the entry to the NativeObjectWrapper table // as the entry is always the same pair. - Debug.Assert(wrapper.ProxyHandle.Target == comProxy); + Debug.Assert(wrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy); Debug.Assert(wrapper.IsUniqueInstance || _rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy); // Add the input wrapper bound to the COM proxy, if there isn't one already. If another thread raced @@ -1431,7 +1449,7 @@ public Bucket() _lock.EnterWriteLock(); try { - Debug.Assert(wrapper.ProxyHandle.Target == comProxy); + Debug.Assert(wrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy); ref WeakGCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); if (!exists) { @@ -1447,10 +1465,9 @@ public Bucket() } else { - object? existingProxy = cachedWrapper.ProxyHandle.Target; // The target NativeObjectWrapper was not collected, but we need to make sure // that the proxy object is still alive. - if (existingProxy is not null) + if (cachedWrapper.ProxyHandle.TryGetTarget(out object? existingProxy)) { // The existing proxy object is still alive, we will use that. return (cachedWrapper, existingProxy); @@ -1482,7 +1499,7 @@ public Bucket() return null; } if (existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) - && cachedWrapper.ProxyHandle.Target is object cachedProxy) + && cachedWrapper.ProxyHandle.TryGetTarget(out object? cachedProxy)) { // The target exists and is still alive. Return it. return cachedProxy; diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.cs index e63c6900ac5b6e..8de9a4d60bd455 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.cs @@ -86,8 +86,7 @@ internal static void ReleaseExternalObjectsFromCurrentThread() { wrappersToRemove.Add(nativeObjectWrapper); - object? target = nativeObjectWrapper.ProxyHandle.Target; - if (target != null) + if (nativeObjectWrapper.ProxyHandle.TryGetTarget(out object? target)) { objects.Add(target); } diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 6377fcb2e7c351..233a476c4d747f 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -33,6 +33,8 @@ static TestComWrappers() fpWrappedQueryInterface = MockReferenceTrackerRuntime.WrapQueryInterface(fpQueryInterface); } + public bool UseManualReleaseITestObjectWrapper { get; init; } + protected unsafe override ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) { ComInterfaceEntry* entryRaw = null; @@ -133,7 +135,14 @@ protected override object CreateObject(IntPtr externalComObject, CreateObjectFla hr = Marshal.QueryInterface(externalComObject, typeof(ITest).GUID, out iTest); if (hr == 0) { - return new ITestObjectWrapper(iTest); + if (UseManualReleaseITestObjectWrapper) + { + return new ManualReleaseITestObjectWrapper(iTest); + } + else + { + return new ITestObjectWrapper(iTest); + } } Assert.Fail("The COM object should support ITrackerObject or ITest for all tests in this test suite."); @@ -418,9 +427,9 @@ unsafe static void CallSetValue(TestComWrappers wrappers, Test testInstance, int [MethodImpl(MethodImplOptions.NoInlining)] [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] [Fact] - public void ValidateResurrection() + public void ValidateManagedObjectWrapperResurrection() { - Console.WriteLine($"Running {nameof(ValidateResurrection)}..."); + Console.WriteLine($"Running {nameof(ValidateManagedObjectWrapperResurrection)}..."); var wrappers = new TestComWrappers(); @@ -621,6 +630,72 @@ public void ValidateMappingAPIs() Marshal.Release(unmanagedObjIUnknown); } + class Resurrecter() + { + public ManualReleaseITestObjectWrapper? UnmanagedWrapper; + + ~Resurrecter() + { + if (UnmanagedWrapper != null) + { + GC.ReRegisterForFinalize(this); + } + } + } + + + [MethodImpl(MethodImplOptions.NoInlining)] + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateNativeObjectWrapperResurrection() + { + Console.WriteLine($"Running {nameof(ValidateNativeObjectWrapperResurrection)}..."); + + var cw = new TestComWrappers() + { + UseManualReleaseITestObjectWrapper = true, + }; + + WeakGCHandle resurrecter; + nint unmanagedObj = AllocateWrapper(cw, out resurrecter); + Assert.Equal(0, Marshal.QueryInterface(unmanagedObj, IUnknownVtbl.IID_IUnknown, out IntPtr unmanagedObjIUnknown)); + ForceGC(); + AssertNativeObjectWrapperAlive(cw, resurrecter, unmanagedObjIUnknown); + + resurrecter.Dispose(); + Marshal.Release(unmanagedObjIUnknown); + Assert.Equal(0, Marshal.Release(unmanagedObj)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static nint AllocateWrapper(ComWrappers cw, out WeakGCHandle handle) + { + Test test = new(); + nint comWrapper = cw.GetOrCreateComInterfaceForObject(test, CreateComInterfaceFlags.None); + Assert.NotEqual(IntPtr.Zero, comWrapper); + + var unmanagedWrapper = (ManualReleaseITestObjectWrapper)cw.GetOrCreateObjectForComInstance(comWrapper, CreateObjectFlags.UniqueInstance); + Resurrecter resurrecter = new() + { + UnmanagedWrapper = unmanagedWrapper, + }; + handle = new WeakGCHandle(resurrecter, true); + return comWrapper; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void AssertNativeObjectWrapperAlive(ComWrappers cw, WeakGCHandle handle, IntPtr unmanagedObj) + { + Assert.True(handle.TryGetTarget(out Resurrecter resurrecter)); + ManualReleaseITestObjectWrapper? unmanagedWrapper = resurrecter.UnmanagedWrapper; + Assert.NotNull(resurrecter); + Assert.True(ComWrappers.TryGetComInstance(unmanagedWrapper, out IntPtr unmanagedObjOther)); + Assert.Equal(unmanagedObj, unmanagedObjOther); + resurrecter.UnmanagedWrapper = null; + Marshal.Release(unmanagedObjOther); + unmanagedWrapper.FinalRelease(); + } + } + [MethodImpl(MethodImplOptions.NoInlining)] [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] [Fact] diff --git a/src/tests/Interop/COM/ComWrappers/Common.cs b/src/tests/Interop/COM/ComWrappers/Common.cs index ca32876a04a2ba..03d98749076873 100644 --- a/src/tests/Interop/COM/ComWrappers/Common.cs +++ b/src/tests/Interop/COM/ComWrappers/Common.cs @@ -94,13 +94,13 @@ public static int SetValueInternal(IntPtr dispatchPtr, int i) } } - public class ITestObjectWrapper : ITest + public class ITestObjectWrapperBase : ITest { private readonly ITestVtbl._SetValue _setValue; - private readonly IntPtr _ptr; - private bool _released; + protected readonly IntPtr _ptr; + protected bool _released; - public ITestObjectWrapper(IntPtr ptr) + public ITestObjectWrapperBase(IntPtr ptr) { _ptr = ptr; VtblPtr inst = Marshal.PtrToStructure(ptr); @@ -117,6 +117,24 @@ public int FinalRelease() return count; } + public void SetValue(int i) => _setValue(_ptr, i); + } + + public class ManualReleaseITestObjectWrapper : ITestObjectWrapperBase + { + public ManualReleaseITestObjectWrapper(IntPtr ptr) + : base(ptr) + { + } + } + + public class ITestObjectWrapper : ITestObjectWrapperBase + { + public ITestObjectWrapper(IntPtr ptr) + : base(ptr) + { + } + ~ITestObjectWrapper() { if (_ptr != IntPtr.Zero && !_released) @@ -124,8 +142,6 @@ public int FinalRelease() Marshal.Release(_ptr); } } - - public void SetValue(int i) => _setValue(_ptr, i); } //