From 3f5dd34a6ae93df1d9229385468fe772a904028e Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Tue, 4 May 2021 13:18:48 -0700 Subject: [PATCH] Fix InvalidCastExceptions from RCW reuse (#833) * Attempt to fix issue where RCW gets cleaned up from cache after finalizer by delaying last release. * Change to reregister for finalization instead. * Cleanup * Fix new line * PR feedback. --- .../UnitTest/TestComponentCSharp_Tests.cs | 36 +++++++++++-------- src/WinRT.Runtime/ComWrappersSupport.net5.cs | 17 +++++---- src/WinRT.Runtime/IWinRTObject.net5.cs | 1 + src/WinRT.Runtime/ObjectReference.cs | 30 ++++++++++++++-- 4 files changed, 61 insertions(+), 23 deletions(-) diff --git a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs index c881ab4e3..fece2b2da 100644 --- a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs +++ b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs @@ -2200,7 +2200,15 @@ internal interface IInitializeWithWindow [Fact] unsafe public void TestComImports() - { + { + TestObject(); + GCCollect(); + Assert.Equal(0, ComImports.NumObjects); + + TestImports(); + GCCollect(); + Assert.Equal(0, ComImports.NumObjects); + static Object MakeObject() { Assert.Equal(0, ComImports.NumObjects); @@ -2221,25 +2229,25 @@ static Object MakeObject() static void TestImports() { - var (initializeWithWindow, windowNative) = MakeImports(); - - GC.Collect(); - GC.WaitForPendingFinalizers(); + var (initializeWithWindow, windowNative) = MakeImports(); + + GCCollect(); var hwnd = new IntPtr(0x12345678); initializeWithWindow.Initialize(hwnd); Assert.Equal(windowNative.WindowHandle, hwnd); } - TestObject(); - GC.Collect(); - GC.WaitForPendingFinalizers(); - Assert.Equal(0, ComImports.NumObjects); - - TestImports(); - GC.Collect(); - GC.WaitForPendingFinalizers(); - Assert.Equal(0, ComImports.NumObjects); + static void GCCollect() + { + // Require multiple GC collects due to + // the final release is not done immediately. + for(int idx = 0; idx < 3; idx++) + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } } [Fact] diff --git a/src/WinRT.Runtime/ComWrappersSupport.net5.cs b/src/WinRT.Runtime/ComWrappersSupport.net5.cs index 003782ec7..a71f4ae1b 100644 --- a/src/WinRT.Runtime/ComWrappersSupport.net5.cs +++ b/src/WinRT.Runtime/ComWrappersSupport.net5.cs @@ -99,12 +99,12 @@ public static T CreateRcwForComObject(IntPtr ptr) winrtObj.Resurrect(); } - return rcw switch - { - ABI.System.Nullable ns => (T)(object) ns.Value, - ABI.System.Nullable nt => (T)(object) nt.Value, - _ => (T) rcw - }; + return rcw switch + { + ABI.System.Nullable ns => (T)(object)ns.Value, + ABI.System.Nullable nt => (T)(object)nt.Value, + _ => (T)rcw + }; } public static bool TryUnwrapObject(object o, out IObjectReference objRef) @@ -143,6 +143,7 @@ public static object TryRegisterObjectForInterface(object obj, IntPtr thisPtr) if (target is IWinRTObject winrtObj) { winrtObj.Resurrect(); + winrtObj.NativeObject.MarkCleanupRCW(); } return rcw; } @@ -205,7 +206,8 @@ public unsafe static void Init( IntPtr inner, out IObjectReference objRef) { - objRef = ComWrappersSupport.GetObjectReferenceForInterface(isAggregation ? inner : newInstance); + objRef = ComWrappersSupport.GetObjectReferenceForInterface(isAggregation ? inner : newInstance); + objRef.MarkCleanupRCW(); IntPtr referenceTracker; { @@ -464,6 +466,7 @@ protected override object CreateObject(IntPtr externalComObject, CreateObjectFla // on destruction as the CLR would do it. winrtObj.NativeObject.ReleaseFromTrackerSource(); winrtObj.NativeObject.PreventReleaseFromTrackerSourceOnDispose = true; + winrtObj.NativeObject.MarkCleanupRCW(); } return obj; diff --git a/src/WinRT.Runtime/IWinRTObject.net5.cs b/src/WinRT.Runtime/IWinRTObject.net5.cs index b7431d8f5..3cca966f6 100644 --- a/src/WinRT.Runtime/IWinRTObject.net5.cs +++ b/src/WinRT.Runtime/IWinRTObject.net5.cs @@ -169,6 +169,7 @@ internal void Resurrect() { if (NativeObject.Resurrect()) { + NativeObject.MarkCleanupRCW(); foreach (var cached in QueryInterfaceCache) { cached.Value.Resurrect(); diff --git a/src/WinRT.Runtime/ObjectReference.cs b/src/WinRT.Runtime/ObjectReference.cs index 7e5a999b7..321399fa0 100644 --- a/src/WinRT.Runtime/ObjectReference.cs +++ b/src/WinRT.Runtime/ObjectReference.cs @@ -17,7 +17,8 @@ public abstract class IObjectReference : IDisposable protected bool disposed; private readonly IntPtr _thisPtr; private object _disposedLock = new object(); - private IntPtr _referenceTrackerPtr; + private IntPtr _referenceTrackerPtr; + private byte _rcwCleanupCounter; public IntPtr ThisPtr { @@ -195,7 +196,14 @@ protected void ThrowIfDisposed() } public void Dispose() - { + { + // If this is the object reference associated with the RCW, + // defer dispose to after the RCW has been finalized for .NET 5. + if (!Cleanup) + { + return; + } + Dispose(true); GC.SuppressFinalize(this); } @@ -208,6 +216,20 @@ protected virtual void Dispose(bool disposing) { return; } + + // If the object reference is associated with the RCW, we need to + // defer the final release on the ThisPtr until after the RCW has been + // finalized and it has been removed from the ComWrappers cache. + // In .NET 6, there will be a new API for us to use, but until then + // in .NET 5, we defer the finalization of this object until it + // has reached Gen 2 by reregistering for finalization. + if(!Cleanup) + { + _rcwCleanupCounter--; + GC.ReRegisterForFinalize(this); + return; + } + #if DEBUG if (BreakOnDispose && System.Diagnostics.Debugger.IsAttached) { @@ -308,6 +330,10 @@ private unsafe void DisposeTrackerSource() ReferenceTracker.IUnknownVftbl.Release(ReferenceTrackerPtr); } } + + internal void MarkCleanupRCW() => _rcwCleanupCounter = 2; + + private bool Cleanup { get => _rcwCleanupCounter == 0; } } public class ObjectReference : IObjectReference