Skip to content

Commit

Permalink
Fix InvalidCastExceptions from RCW reuse (#833)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
manodasanW authored May 4, 2021
1 parent 9359ad7 commit 3f5dd34
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 23 deletions.
36 changes: 22 additions & 14 deletions src/Tests/UnitTest/TestComponentCSharp_Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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]
Expand Down
17 changes: 10 additions & 7 deletions src/WinRT.Runtime/ComWrappersSupport.net5.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,12 @@ public static T CreateRcwForComObject<T>(IntPtr ptr)
winrtObj.Resurrect();
}

return rcw switch
{
ABI.System.Nullable<string> ns => (T)(object) ns.Value,
ABI.System.Nullable<Type> nt => (T)(object) nt.Value,
_ => (T) rcw
};
return rcw switch
{
ABI.System.Nullable<string> ns => (T)(object)ns.Value,
ABI.System.Nullable<Type> nt => (T)(object)nt.Value,
_ => (T)rcw
};
}

public static bool TryUnwrapObject(object o, out IObjectReference objRef)
Expand Down Expand Up @@ -143,6 +143,7 @@ public static object TryRegisterObjectForInterface(object obj, IntPtr thisPtr)
if (target is IWinRTObject winrtObj)
{
winrtObj.Resurrect();
winrtObj.NativeObject.MarkCleanupRCW();
}
return rcw;
}
Expand Down Expand Up @@ -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;
{
Expand Down Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions src/WinRT.Runtime/IWinRTObject.net5.cs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ internal void Resurrect()
{
if (NativeObject.Resurrect())
{
NativeObject.MarkCleanupRCW();
foreach (var cached in QueryInterfaceCache)
{
cached.Value.Resurrect();
Expand Down
30 changes: 28 additions & 2 deletions src/WinRT.Runtime/ObjectReference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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);
}
Expand All @@ -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)
{
Expand Down Expand Up @@ -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<T> : IObjectReference
Expand Down

0 comments on commit 3f5dd34

Please sign in to comment.