From 08a0e2855da966ff9b8ec50838e6e9256995ea4d Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Mon, 29 May 2023 22:07:29 -0700 Subject: [PATCH 1/9] [NativeAOT] allow creating CCW for resurrected objects --- .../InteropServices/ComWrappers.NativeAot.cs | 60 ++++++++++++------- .../Interop/COM/ComWrappers/API/Program.cs | 56 +++++++++++++++++ src/tests/Interop/COM/ComWrappers/Common.cs | 10 +++- 3 files changed, 102 insertions(+), 24 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index 0bb2fb389dd13d..f69a1a26c05b08 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -410,36 +410,57 @@ static bool IsRootedCallback(IntPtr pObj) } private ManagedObjectWrapper* _wrapper; - private object _wrappedObject; + private object? _strongWrappedObject; + private GCHandle _weakWrappedObject; + private bool _exposed; public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedObject) { _wrapper = wrapper; - _wrappedObject = wrappedObject; + _strongWrappedObject = wrappedObject; + _weakWrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); + _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(this); } - public void InitializeHandle() + public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); + + // If the wrapped object is resurrected, restore our strong reference. + public object WrappedObject => _strongWrappedObject ??= _weakWrappedObject.Target!; + + public uint AddRef() { - IntPtr handle = RuntimeImports.RhHandleAllocRefCounted(this); - IntPtr prev = Interlocked.CompareExchange(ref _wrapper->HolderHandle, handle, IntPtr.Zero); - if (prev != IntPtr.Zero) + uint ret = _wrapper->AddRef(); + if (ret == 1) { - RuntimeImports.RhHandleFree(handle); + _exposed = true; + // In case the wrapped object was resurrected, + // restore our strong reference to it. + _strongWrappedObject ??= _weakWrappedObject.Target!; } + return ret; } - public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); - - public object WrappedObject => _wrappedObject; - - public uint AddRef() => _wrapper->AddRef(); - ~ManagedObjectWrapperHolder() { + // release our strong reference to the object + _strongWrappedObject = null; + + if (_exposed && _weakWrappedObject.Target != null) + { + // The wrapped object has not been fully collected, so it is still + // potentially reachable via the CWT. Keep ourselves alive in case + // the wrapped object is resurrected. + GC.ReRegisterForFinalize(this); + return; + } + // Release GC handle created when MOW was built. if (_wrapper->Destroy()) { NativeMemory.Free(_wrapper); + _wrapper = null; + if (_weakWrappedObject.IsAllocated) + _weakWrappedObject.Free(); } else { @@ -519,19 +540,12 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom { ArgumentNullException.ThrowIfNull(instance); - ManagedObjectWrapperHolder? ccwValue; - if (_ccwTable.TryGetValue(instance, out ccwValue)) - { - ccwValue.AddRef(); - return ccwValue.ComIp; - } - - ccwValue = _ccwTable.GetValue(instance, (c) => + ManagedObjectWrapperHolder ccwValue = _ccwTable.GetValue(instance, (c) => { ManagedObjectWrapper* value = CreateCCW(c, flags); return new ManagedObjectWrapperHolder(value, c); }); - ccwValue.InitializeHandle(); + ccwValue.AddRef(); return ccwValue.ComIp; } @@ -581,7 +595,7 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom } mow->HolderHandle = IntPtr.Zero; - mow->RefCount = 1; + mow->RefCount = 0; mow->UserDefinedCount = userDefinedCount; mow->UserDefined = userDefined; mow->Flags = (CreateComInterfaceFlagsEx)flags; diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 56c2748087e37f..373b52021f7ace 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -336,6 +336,62 @@ unsafe static void CallSetValue(TestComWrappers wrappers, Test testInstance, int } } + [MethodImpl(MethodImplOptions.NoInlining)] + [Fact] + public void ValidateResurrection() + { + Console.WriteLine($"Running {nameof(ValidateResurrection)}..."); + + var wrappers = new TestComWrappers(); + + try + { + CreateResurrectingTestInstance(wrappers); + + ForceGC(); + + CallSetValue(wrappers); + } + finally + { + Test.Resurrected = null; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CreateResurrectingTestInstance(ComWrappers wrapper) + { + Test testInstance = new Test() + { + EnableResurrection = true, + }; + IntPtr nativeInstance = wrapper.GetOrCreateComInterfaceForObject(testInstance, CreateComInterfaceFlags.None); + Assert.Equal(0, Marshal.Release(nativeInstance)); + } + + unsafe static void CallSetValue(ComWrappers wrappers) + { + Assert.NotEqual(null, Test.Resurrected); + IntPtr nativeInstance = wrappers.GetOrCreateComInterfaceForObject(Test.Resurrected, CreateComInterfaceFlags.None); + Assert.NotEqual(IntPtr.Zero, nativeInstance); + + var iid = typeof(ITest).GUID; + IntPtr itestPtr; + Assert.Equal(0, Marshal.QueryInterface(nativeInstance, ref iid, out itestPtr)); + + var inst = Marshal.PtrToStructure(itestPtr); + var vtbl = Marshal.PtrToStructure(inst.Vtbl); + var setValue = (delegate* unmanaged)vtbl.SetValue; + + Assert.Equal(0, setValue(itestPtr, 42)); + Assert.Equal(42, Test.Resurrected.GetValue()); + + // release for QueryInterface + Assert.Equal(1, Marshal.Release(itestPtr)); + // release for GetOrCreateComInterfaceForObject + Assert.Equal(0, Marshal.Release(itestPtr)); + } + } + [MethodImpl(MethodImplOptions.NoInlining)] [Fact] public void ValidateFallbackQueryInterface() diff --git a/src/tests/Interop/COM/ComWrappers/Common.cs b/src/tests/Interop/COM/ComWrappers/Common.cs index 41fdf348053dba..748c5f9c5c701e 100644 --- a/src/tests/Interop/COM/ComWrappers/Common.cs +++ b/src/tests/Interop/COM/ComWrappers/Common.cs @@ -19,17 +19,25 @@ interface ITest class Test : ITest, ICustomQueryInterface { + public static Test Resurrected; public static int InstanceCount = 0; private int id; private int value = -1; public Test() { id = Interlocked.Increment(ref InstanceCount); } - ~Test() { Interlocked.Decrement(ref InstanceCount); id = -1; } + ~Test() + { + Interlocked.Decrement(ref InstanceCount); + id = -1; + if (EnableResurrection) + Resurrected = this; + } public void SetValue(int i) => this.value = i; public int GetValue() => this.value; public bool EnableICustomQueryInterface { get; set; } = false; + public bool EnableResurrection { get; set; } = false; public Guid ICustomQueryInterface_GetInterfaceIID { get; set; } public IntPtr ICustomQueryInterface_GetInterfaceResult { get; set; } From 66245fc8ea0f3dfc5f5e880fc96d46a7d0ee0b96 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Sat, 10 Jun 2023 14:39:47 -0700 Subject: [PATCH 2/9] Split the responsbility of keeping the objects alive The WrappedObjectHolder will be used to keep the managed object alive when the COM reference count is positive. And the ManagedObjectWrapper object will be responsible for cleaning up the unmanaged MOW when the wrapped object is truly dead. The design is that as long as there is a postive reference count, the WrappedObjectHolder should stay alive. Any time the ref count is increased from the managed side from 0 to 1, the WrappedObjectHolder will be restored if it was collected. It should not be possible for the MOW to observe a null WrappedObjectHolder. --- .../InteropServices/ComWrappers.NativeAot.cs | 64 +++++++++++-------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index f69a1a26c05b08..a36a92fea56cd4 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -152,7 +152,7 @@ public bool IsRooted } } - public ManagedObjectWrapperHolder? Holder + public WrappedObjectHolder? Holder { get { @@ -160,7 +160,7 @@ public ManagedObjectWrapperHolder? Holder if (handle == IntPtr.Zero) return null; else - return Unsafe.As(GCHandle.FromIntPtr(handle).Target); + return Unsafe.As(GCHandle.FromIntPtr(handle).Target); } } @@ -388,12 +388,12 @@ private static bool IsMarkedToDestroy(ulong c) } } - internal unsafe class ManagedObjectWrapperHolder + internal unsafe class WrappedObjectHolder { - static ManagedObjectWrapperHolder() + static WrappedObjectHolder() { delegate* unmanaged callback = &IsRootedCallback; - if (!RuntimeImports.RhRegisterRefCountedHandleCallback((nint)callback, typeof(ManagedObjectWrapperHolder).GetEEType())) + if (!RuntimeImports.RhRegisterRefCountedHandleCallback((nint)callback, typeof(WrappedObjectHolder).GetEEType())) { throw new OutOfMemoryException(); } @@ -404,48 +404,58 @@ static bool IsRootedCallback(IntPtr pObj) { // We are paused in the GC, so this is safe. #pragma warning disable CS8500 // Takes a pointer to a managed type - ManagedObjectWrapperHolder* holder = (ManagedObjectWrapperHolder*)&pObj; + WrappedObjectHolder* holder = (WrappedObjectHolder*)&pObj; return holder->_wrapper->IsRooted; #pragma warning restore CS8500 } + public object WrappedObject { get; } + private readonly ManagedObjectWrapper* _wrapper; + + public WrappedObjectHolder(object wrappedObject, ManagedObjectWrapper* wrapper) + { + WrappedObject = wrappedObject; + _wrapper = wrapper; + } + } + + internal unsafe class ManagedObjectWrapperHolder + { private ManagedObjectWrapper* _wrapper; - private object? _strongWrappedObject; - private GCHandle _weakWrappedObject; + private GCHandle _wrappedObject; private bool _exposed; public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedObject) { _wrapper = wrapper; - _strongWrappedObject = wrappedObject; - _weakWrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); - _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(this); + _wrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); + _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(null); } public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); - // If the wrapped object is resurrected, restore our strong reference. - public object WrappedObject => _strongWrappedObject ??= _weakWrappedObject.Target!; - - public uint AddRef() + public uint AddRef(object wrappedObject) { + _exposed = true; uint ret = _wrapper->AddRef(); - if (ret == 1) + // Now that the MOW's ref-counted handle is rooting its object, + // ensure that the handle points to something. + WrappedObjectHolder? holder = (WrappedObjectHolder?)RuntimeImports.RhHandleGet(_wrapper->HolderHandle); + if (holder is null) { - _exposed = true; - // In case the wrapped object was resurrected, - // restore our strong reference to it. - _strongWrappedObject ??= _weakWrappedObject.Target!; + RuntimeImports.RhHandleSet(_wrapper->HolderHandle, new WrappedObjectHolder(wrappedObject, _wrapper)); } + else + { + Debug.Assert(ReferenceEquals(wrappedObject, holder.WrappedObject)); + } + GC.KeepAlive(this); return ret; } ~ManagedObjectWrapperHolder() { - // release our strong reference to the object - _strongWrappedObject = null; - - if (_exposed && _weakWrappedObject.Target != null) + if (_exposed && _wrappedObject.Target != null) { // The wrapped object has not been fully collected, so it is still // potentially reachable via the CWT. Keep ourselves alive in case @@ -459,8 +469,8 @@ public uint AddRef() { NativeMemory.Free(_wrapper); _wrapper = null; - if (_weakWrappedObject.IsAllocated) - _weakWrappedObject.Free(); + if (_wrappedObject.IsAllocated) + _wrappedObject.Free(); } else { @@ -545,7 +555,7 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom ManagedObjectWrapper* value = CreateCCW(c, flags); return new ManagedObjectWrapperHolder(value, c); }); - ccwValue.AddRef(); + ccwValue.AddRef(instance); return ccwValue.ComIp; } From 7285288a1388ed605238a12123e473e4f109b2a2 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Sun, 11 Jun 2023 11:15:18 -0700 Subject: [PATCH 3/9] Use a DependentHandle to keep the WrappedObjectHolder alive. I'm still not 100% sure that _exposed is thread safe. --- .../InteropServices/ComWrappers.NativeAot.cs | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index a36a92fea56cd4..3315fd478a04b0 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -422,14 +422,15 @@ public WrappedObjectHolder(object wrappedObject, ManagedObjectWrapper* wrapper) internal unsafe class ManagedObjectWrapperHolder { private ManagedObjectWrapper* _wrapper; - private GCHandle _wrappedObject; + private DependentHandle _wrappedObject; private bool _exposed; public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedObject) { _wrapper = wrapper; - _wrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); - _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(null); + var wrappedObjectHolder = new WrappedObjectHolder(wrappedObject, _wrapper); + _wrappedObject = new DependentHandle(wrappedObject, wrappedObjectHolder); + _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(wrappedObjectHolder); } public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); @@ -438,17 +439,6 @@ public uint AddRef(object wrappedObject) { _exposed = true; uint ret = _wrapper->AddRef(); - // Now that the MOW's ref-counted handle is rooting its object, - // ensure that the handle points to something. - WrappedObjectHolder? holder = (WrappedObjectHolder?)RuntimeImports.RhHandleGet(_wrapper->HolderHandle); - if (holder is null) - { - RuntimeImports.RhHandleSet(_wrapper->HolderHandle, new WrappedObjectHolder(wrappedObject, _wrapper)); - } - else - { - Debug.Assert(ReferenceEquals(wrappedObject, holder.WrappedObject)); - } GC.KeepAlive(this); return ret; } @@ -469,8 +459,7 @@ public uint AddRef(object wrappedObject) { NativeMemory.Free(_wrapper); _wrapper = null; - if (_wrappedObject.IsAllocated) - _wrappedObject.Free(); + _wrappedObject.Dispose(); } else { From adc81cdd4ff099218dbfa58e18ad4944f152771e Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Wed, 14 Jun 2023 09:09:13 -0700 Subject: [PATCH 4/9] Respond to feedback --- .../Runtime/InteropServices/ComWrappers.NativeAot.cs | 4 ++-- src/tests/Interop/COM/ComWrappers/API/Program.cs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index 3315fd478a04b0..e5c5ea5e28e28f 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -448,8 +448,8 @@ public uint AddRef(object wrappedObject) if (_exposed && _wrappedObject.Target != null) { // The wrapped object has not been fully collected, so it is still - // potentially reachable via the CWT. Keep ourselves alive in case - // the wrapped object is resurrected. + // potentially reachable via the Conditional Weak Table. + // Keep ourselves alive in case the wrapped object is resurrected. GC.ReRegisterForFinalize(this); return; } diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 373b52021f7ace..fee467f6799b0d 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -319,12 +319,12 @@ unsafe static void CallSetValue(TestComWrappers wrappers, Test testInstance, int Assert.NotEqual(IntPtr.Zero, nativeInstance); var iid = typeof(ITest).GUID; - IntPtr itestPtr; + nint itestPtr; Assert.Equal(0, Marshal.QueryInterface(nativeInstance, ref iid, out itestPtr)); - var inst = Marshal.PtrToStructure(itestPtr); - var vtbl = Marshal.PtrToStructure(inst.Vtbl); - var setValue = (delegate* unmanaged)vtbl.SetValue; + var inst = (ComWrappers.ComInterfaceDispatch*)itestPtr; + var vtbl = (ITestVtbl*)(inst->Vtable); + var setValue = (delegate* unmanaged)vtbl->SetValue; Assert.Equal(0, setValue(itestPtr, value)); Assert.Equal(value, testInstance.GetValue()); @@ -332,7 +332,7 @@ unsafe static void CallSetValue(TestComWrappers wrappers, Test testInstance, int // release for QueryInterface Assert.Equal(1, Marshal.Release(itestPtr)); // release for GetOrCreateComInterfaceForObject - Assert.Equal(0, Marshal.Release(itestPtr)); + Assert.Equal(0, Marshal.Release(nativeInstance)); } } From 7cab84a891f5e597d57688b71cfd9a39a60bf147 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Wed, 14 Jun 2023 09:21:16 -0700 Subject: [PATCH 5/9] Simplify object ownership structure. --- .../InteropServices/ComWrappers.NativeAot.cs | 59 +++++++++---------- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index e5c5ea5e28e28f..d9571791c334a8 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -152,7 +152,7 @@ public bool IsRooted } } - public WrappedObjectHolder? Holder + public ManagedObjectWrapperHolder? Holder { get { @@ -160,7 +160,7 @@ public WrappedObjectHolder? Holder if (handle == IntPtr.Zero) return null; else - return Unsafe.As(GCHandle.FromIntPtr(handle).Target); + return Unsafe.As(GCHandle.FromIntPtr(handle).Target); } } @@ -388,12 +388,12 @@ private static bool IsMarkedToDestroy(ulong c) } } - internal unsafe class WrappedObjectHolder + internal unsafe class ManagedObjectWrapperHolder { - static WrappedObjectHolder() + static ManagedObjectWrapperHolder() { delegate* unmanaged callback = &IsRootedCallback; - if (!RuntimeImports.RhRegisterRefCountedHandleCallback((nint)callback, typeof(WrappedObjectHolder).GetEEType())) + if (!RuntimeImports.RhRegisterRefCountedHandleCallback((nint)callback, typeof(ManagedObjectWrapperHolder).GetEEType())) { throw new OutOfMemoryException(); } @@ -404,48 +404,44 @@ static bool IsRootedCallback(IntPtr pObj) { // We are paused in the GC, so this is safe. #pragma warning disable CS8500 // Takes a pointer to a managed type - WrappedObjectHolder* holder = (WrappedObjectHolder*)&pObj; + ManagedObjectWrapperHolder* holder = (ManagedObjectWrapperHolder*)&pObj; return holder->_wrapper->IsRooted; #pragma warning restore CS8500 } - public object WrappedObject { get; } - private readonly ManagedObjectWrapper* _wrapper; + private ManagedObjectWrapper* _wrapper; + private readonly ManagedObjectWrapperReleaser _releaser; + private readonly object _wrappedObject; - public WrappedObjectHolder(object wrappedObject, ManagedObjectWrapper* wrapper) + public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedObject) { - WrappedObject = wrappedObject; _wrapper = wrapper; + _wrappedObject = wrappedObject; + _releaser = new ManagedObjectWrapperReleaser(wrapper, wrappedObject); + _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(this); } + + public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); + + public object WrappedObject => _wrappedObject; + + public uint AddRef() => _wrapper->AddRef(); } - internal unsafe class ManagedObjectWrapperHolder + internal unsafe class ManagedObjectWrapperReleaser { private ManagedObjectWrapper* _wrapper; - private DependentHandle _wrappedObject; - private bool _exposed; + private GCHandle _wrappedObject; - public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedObject) + public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper, object wrappedObject) { _wrapper = wrapper; - var wrappedObjectHolder = new WrappedObjectHolder(wrappedObject, _wrapper); - _wrappedObject = new DependentHandle(wrappedObject, wrappedObjectHolder); - _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(wrappedObjectHolder); - } - - public unsafe IntPtr ComIp => _wrapper->As(in ComWrappers.IID_IUnknown); - - public uint AddRef(object wrappedObject) - { - _exposed = true; - uint ret = _wrapper->AddRef(); - GC.KeepAlive(this); - return ret; + _wrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); } - ~ManagedObjectWrapperHolder() + ~ManagedObjectWrapperReleaser() { - if (_exposed && _wrappedObject.Target != null) + if (_wrappedObject.IsAllocated && _wrappedObject.Target != null) { // The wrapped object has not been fully collected, so it is still // potentially reachable via the Conditional Weak Table. @@ -459,7 +455,8 @@ public uint AddRef(object wrappedObject) { NativeMemory.Free(_wrapper); _wrapper = null; - _wrappedObject.Dispose(); + if (_wrappedObject.IsAllocated) + _wrappedObject.Free(); } else { @@ -544,7 +541,7 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom ManagedObjectWrapper* value = CreateCCW(c, flags); return new ManagedObjectWrapperHolder(value, c); }); - ccwValue.AddRef(instance); + ccwValue.AddRef(); return ccwValue.ComIp; } From 9339dcaa7fdbe6a5cab71997d268dc0632b5a306 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Wed, 14 Jun 2023 09:26:11 -0700 Subject: [PATCH 6/9] Restore the call to _ccwTable.TryGetValue. This is to avoid allocating a closure if possible. --- .../Runtime/InteropServices/ComWrappers.NativeAot.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index d9571791c334a8..cab4df3fd022b2 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -536,7 +536,14 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom { ArgumentNullException.ThrowIfNull(instance); - ManagedObjectWrapperHolder ccwValue = _ccwTable.GetValue(instance, (c) => + ManagedObjectWrapperHolder? ccwValue; + if (_ccwTable.TryGetValue(instance, out ccwValue)) + { + ccwValue.AddRef(); + return ccwValue.ComIp; + } + + ccwValue = _ccwTable.GetValue(instance, (c) => { ManagedObjectWrapper* value = CreateCCW(c, flags); return new ManagedObjectWrapperHolder(value, c); From c5e98856649b5798af2039fd6a93c5458c15cc23 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Wed, 14 Jun 2023 09:40:51 -0700 Subject: [PATCH 7/9] Also apply the feedback to the correct unit test. This code was copy-pasted. --- src/tests/Interop/COM/ComWrappers/API/Program.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index fee467f6799b0d..fef40e8d5d0d1d 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -375,12 +375,12 @@ unsafe static void CallSetValue(ComWrappers wrappers) Assert.NotEqual(IntPtr.Zero, nativeInstance); var iid = typeof(ITest).GUID; - IntPtr itestPtr; + nint itestPtr; Assert.Equal(0, Marshal.QueryInterface(nativeInstance, ref iid, out itestPtr)); - var inst = Marshal.PtrToStructure(itestPtr); - var vtbl = Marshal.PtrToStructure(inst.Vtbl); - var setValue = (delegate* unmanaged)vtbl.SetValue; + var inst = (ComWrappers.ComInterfaceDispatch*)itestPtr; + var vtbl = (ITestVtbl*)(inst->Vtable); + var setValue = (delegate* unmanaged)vtbl->SetValue; Assert.Equal(0, setValue(itestPtr, 42)); Assert.Equal(42, Test.Resurrected.GetValue()); @@ -388,7 +388,7 @@ unsafe static void CallSetValue(ComWrappers wrappers) // release for QueryInterface Assert.Equal(1, Marshal.Release(itestPtr)); // release for GetOrCreateComInterfaceForObject - Assert.Equal(0, Marshal.Release(itestPtr)); + Assert.Equal(0, Marshal.Release(nativeInstance)); } } From 532d13025fac4e03d79bfd0e32935d5772ded165 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Wed, 14 Jun 2023 09:48:16 -0700 Subject: [PATCH 8/9] readonly --- .../src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index cab4df3fd022b2..e0fe8b02819b2d 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -409,7 +409,7 @@ static bool IsRootedCallback(IntPtr pObj) #pragma warning restore CS8500 } - private ManagedObjectWrapper* _wrapper; + private readonly ManagedObjectWrapper* _wrapper; private readonly ManagedObjectWrapperReleaser _releaser; private readonly object _wrappedObject; From 8e5bfa8a4f4474c1d07367de22d7442b250676fa Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Fri, 16 Jun 2023 19:57:50 -0700 Subject: [PATCH 9/9] Use refcounted handle to asses the lifetime of the holder This saves a handle. The refcounted handle behaves like a long-weak, so this should not get cleared out until the conditional weak table is also cleared out. --- .../InteropServices/ComWrappers.NativeAot.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs index e0fe8b02819b2d..85fa9a7277f5be 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs @@ -417,7 +417,7 @@ public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedO { _wrapper = wrapper; _wrappedObject = wrappedObject; - _releaser = new ManagedObjectWrapperReleaser(wrapper, wrappedObject); + _releaser = new ManagedObjectWrapperReleaser(wrapper); _wrapper->HolderHandle = RuntimeImports.RhHandleAllocRefCounted(this); } @@ -431,19 +431,18 @@ public ManagedObjectWrapperHolder(ManagedObjectWrapper* wrapper, object wrappedO internal unsafe class ManagedObjectWrapperReleaser { private ManagedObjectWrapper* _wrapper; - private GCHandle _wrappedObject; - public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper, object wrappedObject) + public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper) { _wrapper = wrapper; - _wrappedObject = GCHandle.Alloc(wrappedObject, GCHandleType.WeakTrackResurrection); } ~ManagedObjectWrapperReleaser() { - if (_wrappedObject.IsAllocated && _wrappedObject.Target != null) + IntPtr refCountedHandle = _wrapper->HolderHandle; + if (refCountedHandle != IntPtr.Zero && RuntimeImports.RhHandleGet(refCountedHandle) != null) { - // The wrapped object has not been fully collected, so it is still + // The ManagedObjectWrapperHolder has not been fully collected, so it is still // potentially reachable via the Conditional Weak Table. // Keep ourselves alive in case the wrapped object is resurrected. GC.ReRegisterForFinalize(this); @@ -455,8 +454,6 @@ public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper, object wrappe { NativeMemory.Free(_wrapper); _wrapper = null; - if (_wrappedObject.IsAllocated) - _wrappedObject.Free(); } else {