From 4a1e2e8595f5287d8e6f75431622f0de8fcad8bf Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Wed, 18 Dec 2024 15:58:35 -0800 Subject: [PATCH 01/16] Exit the lock before we call into user code and handle losing the race for the RCW table. --- .../InteropServices/ComWrappers.NativeAot.cs | 60 +++++++++++++------ 1 file changed, 41 insertions(+), 19 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 75a49c362ff2a0..532862497b0525 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 @@ -968,21 +968,40 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( _rcwCache.Remove(identity); } } + } + + if (wrapperMaybe is not null) + { + retValue = wrapperMaybe; - if (wrapperMaybe is not null) + using (_lock.EnterScope()) { - retValue = wrapperMaybe; - NativeObjectWrapper wrapper = NativeObjectWrapper.Create( - identity, - inner, - this, - retValue, - flags); - if (!s_rcwTable.TryAdd(retValue, wrapper)) + if (!s_rcwTable.TryGetValue(retValue, out NativeObjectWrapper? wrapper)) { - wrapper.Release(); - throw new NotSupportedException(); + // Exit the lock before we create the native object wrapper + // as we may execute arbitrary user code. + _lock.Exit(); + try + { + NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( + identity, + inner, + this, + retValue, + flags); + + wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); + if (wrapper != newWrapper) + { + newWrapper.Release(); + } + } + finally + { + _lock.Enter(); + } } + _rcwCache.Add(identity, wrapper._proxyHandle); AddWrapperToReferenceTrackerHandleCache(wrapper); return true; @@ -1026,16 +1045,17 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( if (flags.HasFlag(CreateObjectFlags.UniqueInstance)) { - NativeObjectWrapper wrapper = NativeObjectWrapper.Create( + NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( identity, inner, null, // No need to cache NativeObjectWrapper for unique instances. They are not cached. retValue, flags); - if (!s_rcwTable.TryAdd(retValue, wrapper)) + + NativeObjectWrapper wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); + if (wrapper != newWrapper) { - wrapper.Release(); - throw new NotSupportedException(); + newWrapper.Release(); } AddWrapperToReferenceTrackerHandleCache(wrapper); return true; @@ -1061,17 +1081,19 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( } else { - NativeObjectWrapper wrapper = NativeObjectWrapper.Create( + NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( identity, inner, this, retValue, flags); - if (!s_rcwTable.TryAdd(retValue, wrapper)) + + NativeObjectWrapper wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); + if (wrapper != newWrapper) { - wrapper.Release(); - throw new NotSupportedException(); + newWrapper.Release(); } + _rcwCache.Add(identity, wrapper._proxyHandle); AddWrapperToReferenceTrackerHandleCache(wrapper); } From b59f2f71061a92b59d015014c0b821019ed54143 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 16:47:25 -0800 Subject: [PATCH 02/16] Better encapsulate logic for the RCW table and the RCW cache. --- .../InteropServices/ComWrappers.NativeAot.cs | 240 +++++++++++------- 1 file changed, 148 insertions(+), 92 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 532862497b0525..d6de406820d057 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 @@ -44,7 +44,7 @@ public abstract partial class ComWrappers private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); - private readonly Lock _lock = new Lock(useTrivialWaits: true); + private readonly Lock _rcwCacheLock = new Lock(useTrivialWaits: true); private readonly Dictionary _rcwCache = new Dictionary(); internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId) @@ -799,7 +799,7 @@ public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateOb if (!TryGetOrCreateObjectForComInstanceInternal(externalComObject, IntPtr.Zero, flags, null, out obj)) throw new ArgumentNullException(nameof(externalComObject)); - return obj!; + return obj; } /// @@ -841,7 +841,7 @@ public object GetOrRegisterObjectForComInstance(IntPtr externalComObject, Create if (!TryGetOrCreateObjectForComInstanceInternal(externalComObject, inner, flags, wrapper, out obj)) throw new ArgumentNullException(nameof(externalComObject)); - return obj!; + return obj; } private static unsafe ComInterfaceDispatch* TryGetComInterfaceDispatch(IntPtr comObject) @@ -932,7 +932,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( IntPtr innerMaybe, CreateObjectFlags flags, object? wrapperMaybe, - out object? retValue) + [NotNullWhen(true)] out object? retValue) { if (externalComObject == IntPtr.Zero) throw new ArgumentNullException(nameof(externalComObject)); @@ -951,62 +951,42 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) { - using (_lock.EnterScope()) + // If we have a live cached wrapper currently, + // return that. + if (FindRCWInCache(identity) is object liveCachedWrapper) { - if (_rcwCache.TryGetValue(identity, out GCHandle handle)) - { - object? cachedWrapper = handle.Target; - if (cachedWrapper is not null) - { - retValue = cachedWrapper; - return true; - } - else - { - // The GCHandle has been clear out but the NativeObjectWrapper - // finalizer has not yet run to remove the entry from _rcwCache - _rcwCache.Remove(identity); - } - } + retValue = liveCachedWrapper; + return true; } + // If the user tried to provide a pre-created managed wrapper, try to register + // that object as the wrapper. if (wrapperMaybe is not null) { retValue = wrapperMaybe; - using (_lock.EnterScope()) - { - if (!s_rcwTable.TryGetValue(retValue, out NativeObjectWrapper? wrapper)) - { - // Exit the lock before we create the native object wrapper - // as we may execute arbitrary user code. - _lock.Exit(); - try - { - NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( - identity, - inner, - this, - retValue, - flags); - - wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); - if (wrapper != newWrapper) - { - newWrapper.Release(); - } - } - finally - { - _lock.Enter(); - } - } + NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( + identity, + inner, + this, + retValue, + flags, + checkForExistingWrapper: true); - _rcwCache.Add(identity, wrapper._proxyHandle); - AddWrapperToReferenceTrackerHandleCache(wrapper); - return true; - } + // At this point, wrapper is the registered NativeObjectWrapper for retValue, + // Even if two threads are racing to register retValue as a wrapper for + // a COM instance. + // + // Now we can try to actually register this wrapper as the NativeObjectWrapper for identity. + // AddRCWToCache will return the user object corresponding to the wrapper that wins the race. + + retValue = AddRCWToCache(identity, wrapper); + + return true; } + + // Check if the provided COM instance is actually a managed object wrapper from this + // ComWrappers instance, and use it if it is. if (flags.HasFlag(CreateObjectFlags.Unwrap)) { ComInterfaceDispatch* comInterfaceDispatch = TryGetComInterfaceDispatch(identity); @@ -1045,64 +1025,86 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( if (flags.HasFlag(CreateObjectFlags.UniqueInstance)) { - NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( + // We didn't find a cached wrapper for the identity, so we need to create one. + // Don't check for an existing wrapper as we just created the object + // and a wrapper already existing would be exceedingly rare. + NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( identity, inner, null, // No need to cache NativeObjectWrapper for unique instances. They are not cached. retValue, - flags); + flags, + checkForExistingWrapper: false); - NativeObjectWrapper wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); - if (wrapper != newWrapper) - { - newWrapper.Release(); - } + // We don't need to add the wrapper to the RCW cache because we don't cache unique instances. + // But we still need to add it to the reference tracker handle cache for correct tracker support. AddWrapperToReferenceTrackerHandleCache(wrapper); return true; } - using (_lock.EnterScope()) + // Now that we've called into user code to create the user object wrapper, + // another thread may have beaten us to creating a cached wrapper for the same identity. + // Check the cache again to avoid creating a NativeObjectWrapper when we don't need to. + if (FindRCWInCache(identity) is object cachedWrapper) { - object? cachedWrapper = null; - if (_rcwCache.TryGetValue(identity, out var existingHandle)) - { - cachedWrapper = existingHandle.Target; - if (cachedWrapper is null) - { - // The GCHandle has been clear out but the NativeObjectWrapper - // finalizer has not yet run to remove the entry from _rcwCache - _rcwCache.Remove(identity); - } - } - - if (cachedWrapper is not null) - { - retValue = cachedWrapper; - } - else - { - NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( - identity, - inner, - this, - retValue, - flags); + retValue = cachedWrapper; + return true; + } - NativeObjectWrapper wrapper = s_rcwTable.GetValue(retValue, _ => newWrapper); - if (wrapper != newWrapper) - { - newWrapper.Release(); - } + { + // We didn't find a cached wrapper for the identity, so we need to create one. + // Don't check for an existing wrapper as we just created the object + // and a wrapper already existing would be exceedingly rare. + NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( + identity, + inner, + this, + retValue, + flags, + checkForExistingWrapper: false); - _rcwCache.Add(identity, wrapper._proxyHandle); - AddWrapperToReferenceTrackerHandleCache(wrapper); - } + retValue = AddRCWToCache(identity, wrapper); } return true; } #pragma warning restore IDE0060 + private static NativeObjectWrapper GetOrCreateWrapperForComProxy(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags, bool checkForExistingWrapper) + { + if (checkForExistingWrapper && s_rcwTable.TryGetValue(comProxy, out NativeObjectWrapper? existingWrapper)) + { + return existingWrapper; + } + + NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( + externalComObject, + inner, + comWrappers, + comProxy, + flags); + + NativeObjectWrapper registeredWrapper = GetValueFromRcwTable(comProxy, newWrapper); + if (registeredWrapper != newWrapper) + { + // We've lost the race to register the native object wrapper for this managed object. + // However, this may not have been a race. + // The user may have just tried to register the same managed object as a wrapper for two different + // COM objects. Check for this condition and throw the documented exception + // after releasing the wrapper that lost the race. + bool wrappingSameUnderlyingComObject = registeredWrapper._externalComObject == newWrapper._externalComObject; + newWrapper.Release(); + if (!wrappingSameUnderlyingComObject) + { + throw new NotSupportedException(); + } + } + return registeredWrapper; + + // Separate out into a local function to avoid the closure and delegate allocation unless we need it. + static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_rcwTable.GetValue(userObject, _ => newWrapper); + } + private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) { if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) @@ -1111,9 +1113,63 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } + private object AddRCWToCache(IntPtr comPointer, NativeObjectWrapper wrapper) + { + using (_rcwCacheLock.EnterScope()) + { + ref GCHandle handle = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); + if (!exists) + { + // Someone else didn't beat us to adding the entry to the cache. + // Add our entry here. + handle = wrapper._proxyHandle; + + // Also, add the wrapper to the reference tracker handle cache here. + AddWrapperToReferenceTrackerHandleCache(wrapper); + } + else + { + // If someone else beat us to adding an entry to the RCW cache, + // remove the entry from the RCW table. + // The object we created will not be an RCW, + // so we need to remove its entry from the RCW table. + s_rcwTable.Remove(wrapper._proxyHandle.Target!); + + // We don't call Release here on the wrapper as we're within the rcwCacheLock + // and the Release call can call into external code. + // Let the wrapper get cleaned up by the finalizer to avoid a deadlock. + } + + // Either we added our entry to the cache or + // someone else beat us to adding our entry to the cache. + // In either case, we want to return the wrapper that one the race. + return handle.Target!; + } + } + + private object FindRCWInCache(IntPtr comPointer) + { + using (_rcwCacheLock.EnterScope()) + { + if (_rcwCache.TryGetValue(comPointer, out var existingHandle)) + { + object? cachedWrapper = existingHandle.Target; + if (cachedWrapper is null) + { + // The GCHandle has been clear out but the NativeObjectWrapper + // finalizer has not yet run to remove the entry from _rcwCache + _rcwCache.Remove(comPointer); + } + return cachedWrapper; + } + + return null; + } + } + private void RemoveRCWFromCache(IntPtr comPointer, GCHandle expectedValue) { - using (_lock.EnterScope()) + using (_rcwCacheLock.EnterScope()) { // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache // in the time between the GC cleared the contents of the GC handle but before the From 98fea757085581b79690fbe818103885b2fbf689 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 17:40:36 -0800 Subject: [PATCH 03/16] Flip the order of operations to match CoreCLR: 1. Register that we've created a managed object as a wrapper for a COM object first (equivalent to ExtObjCxtCache in CoreCLR) 2. Register the NativeObjectWrapper for lifetime management (equivalent to the sync-block interop info in CoreCLR). This ensures that we have the same behavior as CoreCLR. --- .../InteropServices/ComWrappers.NativeAot.cs | 156 +++++++++--------- 1 file changed, 75 insertions(+), 81 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 d6de406820d057..9fa666d49527d9 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 @@ -40,7 +40,7 @@ public abstract partial class ComWrappers private static readonly Guid IID_IInspectable = new Guid(0xAF86E2E0, 0xB12D, 0x4c6a, 0x9C, 0x5A, 0xD7, 0xAA, 0x65, 0x10, 0x1E, 0x90); private static readonly Guid IID_IWeakReferenceSource = new Guid(0x00000038, 0, 0, 0xC0, 0, 0, 0, 0, 0, 0, 0x46); - private static readonly ConditionalWeakTable s_rcwTable = new ConditionalWeakTable(); + private static readonly ConditionalWeakTable s_nativeObjectWrapperTable = new ConditionalWeakTable(); private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); @@ -50,7 +50,7 @@ public abstract partial class ComWrappers internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId) { if (obj == null - || !s_rcwTable.TryGetValue(obj, out NativeObjectWrapper? wrapper)) + || !s_nativeObjectWrapperTable.TryGetValue(obj, out NativeObjectWrapper? wrapper)) { unknown = IntPtr.Zero; wrapperId = 0; @@ -65,7 +65,7 @@ public static unsafe bool TryGetComInstance(object obj, out IntPtr unknown) { unknown = IntPtr.Zero; if (obj == null - || !s_rcwTable.TryGetValue(obj, out NativeObjectWrapper? wrapper)) + || !s_nativeObjectWrapperTable.TryGetValue(obj, out NativeObjectWrapper? wrapper)) { return false; } @@ -497,9 +497,9 @@ internal unsafe class NativeObjectWrapper internal IntPtr _externalComObject; private IntPtr _inner; internal ComWrappers _comWrappers; - internal readonly GCHandle _proxyHandle; internal readonly GCHandle _proxyHandleTrackingResurrection; internal readonly bool _aggregatedManagedObjectWrapper; + private bool _maybeCached; static NativeObjectWrapper() { @@ -509,25 +509,34 @@ static NativeObjectWrapper() ComAwareWeakReference.InitializeCallbacks(&ComWeakRefToObject, &PossiblyComObject, &ObjectToComWeakRef); } - public static NativeObjectWrapper Create(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags) + public static NativeObjectWrapper Create(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, GCHandle proxyHandle, CreateObjectFlags flags) { + Debug.Assert(comProxy == proxyHandle.Target); if (flags.HasFlag(CreateObjectFlags.TrackerObject) && Marshal.QueryInterface(externalComObject, IID_IReferenceTracker, out IntPtr trackerObject) == HResults.S_OK) { - return new ReferenceTrackerNativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags, trackerObject); + return new ReferenceTrackerNativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags, trackerObject) + { + ProxyHandle = proxyHandle + }; } else { - return new NativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags); + return new NativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags) + { + ProxyHandle = proxyHandle + }; } } - public NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags) + internal required GCHandle ProxyHandle { get; init; } + + protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags) { _externalComObject = externalComObject; _inner = inner; _comWrappers = comWrappers; - _proxyHandle = GCHandle.Alloc(comProxy, GCHandleType.Weak); + _maybeCached = !flags.HasFlag(CreateObjectFlags.UniqueInstance); // 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 @@ -550,15 +559,18 @@ public NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrappers c public virtual void Release() { - if (_comWrappers != null) + if (_maybeCached) { - _comWrappers.RemoveRCWFromCache(_externalComObject, _proxyHandle); - _comWrappers = null; + if (ProxyHandle.IsAllocated) + { + _comWrappers.RemoveRCWFromCache(_externalComObject, ProxyHandle); + } + _maybeCached = false; } - if (_proxyHandle.IsAllocated) + if (ProxyHandle.IsAllocated) { - _proxyHandle.Free(); + ProxyHandle.Free(); } if (_proxyHandleTrackingResurrection.IsAllocated) @@ -917,7 +929,6 @@ private static void DetermineIdentityAndInner( } } -#pragma warning disable IDE0060 /// /// Get the currently registered managed object or creates a new managed object and registers it. /// @@ -965,22 +976,16 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( { retValue = wrapperMaybe; - NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( + retValue = AddRCWToCache(identity, retValue, out GCHandle wrapperWeakRef); + + // At this point, retValue is the RCW for the identity. + // Create a NativeObjectWrapper to handle lifetime tracking of the references to the COM object. + CreateWrapperForComProxy( identity, inner, - this, retValue, - flags, - checkForExistingWrapper: true); - - // At this point, wrapper is the registered NativeObjectWrapper for retValue, - // Even if two threads are racing to register retValue as a wrapper for - // a COM instance. - // - // Now we can try to actually register this wrapper as the NativeObjectWrapper for identity. - // AddRCWToCache will return the user object corresponding to the wrapper that wins the race. - - retValue = AddRCWToCache(identity, wrapper); + wrapperWeakRef, + flags); return true; } @@ -1028,17 +1033,12 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // We didn't find a cached wrapper for the identity, so we need to create one. // Don't check for an existing wrapper as we just created the object // and a wrapper already existing would be exceedingly rare. - NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( + CreateWrapperForComProxy( identity, inner, - null, // No need to cache NativeObjectWrapper for unique instances. They are not cached. retValue, - flags, - checkForExistingWrapper: false); - - // We don't need to add the wrapper to the RCW cache because we don't cache unique instances. - // But we still need to add it to the reference tracker handle cache for correct tracker support. - AddWrapperToReferenceTrackerHandleCache(wrapper); + GCHandle.Alloc(retValue, GCHandleType.Weak), + flags); return true; } @@ -1052,57 +1052,65 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( } { - // We didn't find a cached wrapper for the identity, so we need to create one. - // Don't check for an existing wrapper as we just created the object - // and a wrapper already existing would be exceedingly rare. - NativeObjectWrapper wrapper = GetOrCreateWrapperForComProxy( + retValue = AddRCWToCache(identity, retValue, out GCHandle wrapperWeakRef); + + // At this point, retValue is the RCW for the identity. + // Create a NativeObjectWrapper to handle lifetime tracking of the references to the COM object. + CreateWrapperForComProxy( identity, inner, - this, retValue, - flags, - checkForExistingWrapper: false); - - retValue = AddRCWToCache(identity, wrapper); + wrapperWeakRef, + flags); } return true; } -#pragma warning restore IDE0060 - private static NativeObjectWrapper GetOrCreateWrapperForComProxy(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags, bool checkForExistingWrapper) + private void CreateWrapperForComProxy(IntPtr externalComObject, IntPtr inner, object comProxy, GCHandle proxyWeakRef, CreateObjectFlags flags) { - if (checkForExistingWrapper && s_rcwTable.TryGetValue(comProxy, out NativeObjectWrapper? existingWrapper)) + if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? existingWrapper)) { - return existingWrapper; + // A native wrapper already exists for this managed object. + // We don't need to do any work. + return; } NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( externalComObject, inner, - comWrappers, + this, comProxy, + proxyWeakRef, flags); NativeObjectWrapper registeredWrapper = GetValueFromRcwTable(comProxy, newWrapper); if (registeredWrapper != newWrapper) { // We've lost the race to register the native object wrapper for this managed object. - // However, this may not have been a race. - // The user may have just tried to register the same managed object as a wrapper for two different - // COM objects. Check for this condition and throw the documented exception - // after releasing the wrapper that lost the race. + // If the user is not trying to register the same managed object as the managed wrapper + // for two different COM instances, we need to throw an exception. + // Otherwise, we can just release the wrapper we created. bool wrappingSameUnderlyingComObject = registeredWrapper._externalComObject == newWrapper._externalComObject; - newWrapper.Release(); if (!wrappingSameUnderlyingComObject) { + RemoveRCWFromCache(externalComObject, newWrapper.ProxyHandle); + newWrapper.Release(); throw new NotSupportedException(); } + else + { + newWrapper.Release(); + } } - return registeredWrapper; + + // Add the wrapper to the reference tracker handle cache. + // We must always do this whether we won or lost the race to add the wrapper to the table + // as we can't synchronize with the thread that won the race. + AddWrapperToReferenceTrackerHandleCache(registeredWrapper); // Separate out into a local function to avoid the closure and delegate allocation unless we need it. - static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_rcwTable.GetValue(userObject, _ => newWrapper); + static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_nativeObjectWrapperTable.GetValue(userObject, _ => newWrapper); } private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) @@ -1113,37 +1121,23 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } - private object AddRCWToCache(IntPtr comPointer, NativeObjectWrapper wrapper) + private object AddRCWToCache(IntPtr comPointer, object wrapper, out GCHandle wrapperWeakRef) { using (_rcwCacheLock.EnterScope()) { - ref GCHandle handle = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); + ref GCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); if (!exists) { // Someone else didn't beat us to adding the entry to the cache. // Add our entry here. - handle = wrapper._proxyHandle; - - // Also, add the wrapper to the reference tracker handle cache here. - AddWrapperToReferenceTrackerHandleCache(wrapper); - } - else - { - // If someone else beat us to adding an entry to the RCW cache, - // remove the entry from the RCW table. - // The object we created will not be an RCW, - // so we need to remove its entry from the RCW table. - s_rcwTable.Remove(wrapper._proxyHandle.Target!); - - // We don't call Release here on the wrapper as we're within the rcwCacheLock - // and the Release call can call into external code. - // Let the wrapper get cleaned up by the finalizer to avoid a deadlock. + rcwEntry = GCHandle.Alloc(wrapper, GCHandleType.Weak); } // Either we added our entry to the cache or // someone else beat us to adding our entry to the cache. // In either case, we want to return the wrapper that one the race. - return handle.Target!; + wrapperWeakRef = rcwEntry; + return rcwEntry.Target!; } } @@ -1174,7 +1168,7 @@ private void RemoveRCWFromCache(IntPtr comPointer, GCHandle expectedValue) // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache // in the time between the GC cleared the contents of the GC handle but before the // NativeObjectWrapper finalizer ran. - if (_rcwCache.TryGetValue(comPointer, out GCHandle cachedValue) && expectedValue.Equals(cachedValue)) + if (_rcwCache.TryGetValue(comPointer, out GCHandle cachedValue) && expectedValue == cachedValue) { _rcwCache.Remove(comPointer); } @@ -1311,7 +1305,7 @@ internal static void ReleaseExternalObjectsFromCurrentThread() if (nativeObjectWrapper != null && nativeObjectWrapper._contextToken == contextToken) { - object? target = nativeObjectWrapper._proxyHandle.Target; + object? target = nativeObjectWrapper.ProxyHandle.Target; if (target != null) { objects.Add(target); @@ -1338,7 +1332,7 @@ internal static unsafe void WalkExternalTrackerObjects() if (nativeObjectWrapper != null && nativeObjectWrapper.TrackerObject != IntPtr.Zero) { - FindReferenceTargetsCallback.s_currentRootObjectHandle = nativeObjectWrapper._proxyHandle; + FindReferenceTargetsCallback.s_currentRootObjectHandle = nativeObjectWrapper.ProxyHandle; if (IReferenceTracker.FindTrackerTargets(nativeObjectWrapper.TrackerObject, TrackerObjectManager.s_findReferencesTargetCallback) != HResults.S_OK) { walkFailed = true; @@ -1365,7 +1359,7 @@ internal static void DetachNonPromotedObjects() ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); if (nativeObjectWrapper != null && nativeObjectWrapper.TrackerObject != IntPtr.Zero && - !RuntimeImports.RhIsPromoted(nativeObjectWrapper._proxyHandle.Target)) + !RuntimeImports.RhIsPromoted(nativeObjectWrapper.ProxyHandle.Target)) { // Notify the wrapper it was not promoted and is being collected. TrackerObjectManager.BeforeWrapperFinalized(nativeObjectWrapper.TrackerObject); @@ -1686,7 +1680,7 @@ private static unsafe bool PossiblyComObject(object target) // If the RCW is an aggregated RCW, then the managed object cannot be recreated from the IUnknown // as the outer IUnknown wraps the managed object. In this case, don't create a weak reference backed // by a COM weak reference. - return s_rcwTable.TryGetValue(target, out NativeObjectWrapper? wrapper) && !wrapper._aggregatedManagedObjectWrapper; + return s_nativeObjectWrapperTable.TryGetValue(target, out NativeObjectWrapper? wrapper) && !wrapper._aggregatedManagedObjectWrapper; } private static unsafe IntPtr ObjectToComWeakRef(object target, out long wrapperId) From 493a0acc7b6d4e7042907c3b0d60050606971023 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Thu, 19 Dec 2024 17:55:08 -0800 Subject: [PATCH 04/16] Remove outdated comment --- .../System/Runtime/InteropServices/ComWrappers.NativeAot.cs | 3 --- 1 file changed, 3 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 9fa666d49527d9..f7d86d3a27bf2b 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 @@ -484,9 +484,6 @@ public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper) // There are still outstanding references on the COM side. // This case should only be hit when an outstanding // tracker refcount exists from AddRefFromReferenceTracker. - // When implementing IReferenceTrackerHost, this should be - // reconsidered. - // https://github.com/dotnet/runtime/issues/85137 GC.ReRegisterForFinalize(this); } } From c1994ed745601ed9f0cc5f3a4f4dce45e7482184 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 14:08:12 -0800 Subject: [PATCH 05/16] Like CoreCLR, record the NativeObjectWrapper (CoreCLR's ExtObjCxt) in the "RCW address -> wrapper" cache. Use a WeakReference as we don't want to keep the wrapper alive any longer with it being in the cache. --- .../InteropServices/ComWrappers.NativeAot.cs | 229 ++++++++---------- 1 file changed, 107 insertions(+), 122 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 f7d86d3a27bf2b..2384c5428a0c66 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 @@ -43,9 +43,9 @@ public abstract partial class ComWrappers private static readonly ConditionalWeakTable s_nativeObjectWrapperTable = new ConditionalWeakTable(); private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); - private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); + private readonly ConditionalWeakTable _managedObjectWrapperTable = new ConditionalWeakTable(); private readonly Lock _rcwCacheLock = new Lock(useTrivialWaits: true); - private readonly Dictionary _rcwCache = new Dictionary(); + private readonly Dictionary> _rcwCache = new Dictionary>(); internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId) { @@ -57,8 +57,8 @@ internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr un return false; } - wrapperId = wrapper._comWrappers.id; - return Marshal.QueryInterface(wrapper._externalComObject, iid, out unknown) == HResults.S_OK; + wrapperId = wrapper.ComWrappers.id; + return Marshal.QueryInterface(wrapper.ExternalComObject, iid, out unknown) == HResults.S_OK; } public static unsafe bool TryGetComInstance(object obj, out IntPtr unknown) @@ -70,7 +70,7 @@ public static unsafe bool TryGetComInstance(object obj, out IntPtr unknown) return false; } - return Marshal.QueryInterface(wrapper._externalComObject, IID_IUnknown, out unknown) == HResults.S_OK; + return Marshal.QueryInterface(wrapper.ExternalComObject, IID_IUnknown, out unknown) == HResults.S_OK; } public static unsafe bool TryGetObject(IntPtr unknown, [NotNullWhen(true)] out object? obj) @@ -491,10 +491,11 @@ public ManagedObjectWrapperReleaser(ManagedObjectWrapper* wrapper) internal unsafe class NativeObjectWrapper { - internal IntPtr _externalComObject; + private IntPtr _externalComObject; private IntPtr _inner; - internal ComWrappers _comWrappers; - internal readonly GCHandle _proxyHandleTrackingResurrection; + private readonly ComWrappers _comWrappers; + private GCHandle _proxyHandle; + private GCHandle _proxyHandleTrackingResurrection; internal readonly bool _aggregatedManagedObjectWrapper; private bool _maybeCached; @@ -506,40 +507,32 @@ static NativeObjectWrapper() ComAwareWeakReference.InitializeCallbacks(&ComWeakRefToObject, &PossiblyComObject, &ObjectToComWeakRef); } - public static NativeObjectWrapper Create(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, GCHandle proxyHandle, CreateObjectFlags flags) + public static NativeObjectWrapper Create(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags) { - Debug.Assert(comProxy == proxyHandle.Target); if (flags.HasFlag(CreateObjectFlags.TrackerObject) && Marshal.QueryInterface(externalComObject, IID_IReferenceTracker, out IntPtr trackerObject) == HResults.S_OK) { - return new ReferenceTrackerNativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags, trackerObject) - { - ProxyHandle = proxyHandle - }; + return new ReferenceTrackerNativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags, trackerObject); } else { - return new NativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags) - { - ProxyHandle = proxyHandle - }; + return new NativeObjectWrapper(externalComObject, inner, comWrappers, comProxy, flags); } } - internal required GCHandle ProxyHandle { get; init; } - protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrappers comWrappers, object comProxy, CreateObjectFlags flags) { _externalComObject = externalComObject; _inner = inner; _comWrappers = comWrappers; _maybeCached = !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 RCW cache to - // see the object as not alive once it is eligible for finalization. + // 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); // If this is an aggregation scenario and the identity object @@ -554,20 +547,21 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper } } + internal IntPtr ExternalComObject => _externalComObject; + internal ComWrappers ComWrappers => _comWrappers; + internal GCHandle ProxyHandle => _proxyHandle; + public virtual void Release() { if (_maybeCached) { - if (ProxyHandle.IsAllocated) - { - _comWrappers.RemoveRCWFromCache(_externalComObject, ProxyHandle); - } + _comWrappers.RemoveRCWFromCache(_externalComObject, this); _maybeCached = false; } - if (ProxyHandle.IsAllocated) + if (_proxyHandle.IsAllocated) { - ProxyHandle.Free(); + _proxyHandle.Free(); } if (_proxyHandleTrackingResurrection.IsAllocated) @@ -721,23 +715,23 @@ public unsafe IntPtr GetOrCreateComInterfaceForObject(object instance, CreateCom { ArgumentNullException.ThrowIfNull(instance); - ManagedObjectWrapperHolder? ccwValue; - if (_ccwTable.TryGetValue(instance, out ccwValue)) + ManagedObjectWrapperHolder? managedObjectWrapper; + if (_managedObjectWrapperTable.TryGetValue(instance, out managedObjectWrapper)) { - ccwValue.AddRef(); - return ccwValue.ComIp; + managedObjectWrapper.AddRef(); + return managedObjectWrapper.ComIp; } - ccwValue = _ccwTable.GetValue(instance, (c) => + managedObjectWrapper = _managedObjectWrapperTable.GetValue(instance, (c) => { - ManagedObjectWrapper* value = CreateCCW(c, flags); + ManagedObjectWrapper* value = CreateManagedObjectWrapper(c, flags); return new ManagedObjectWrapperHolder(value, c); }); - ccwValue.AddRef(); - return ccwValue.ComIp; + managedObjectWrapper.AddRef(); + return managedObjectWrapper.ComIp; } - private unsafe ManagedObjectWrapper* CreateCCW(object instance, CreateComInterfaceFlags flags) + private unsafe ManagedObjectWrapper* CreateManagedObjectWrapper(object instance, CreateComInterfaceFlags flags) { ComInterfaceEntry* userDefined = ComputeVtables(instance, flags, out int userDefinedCount); if ((userDefined == null && userDefinedCount != 0) || userDefinedCount < 0) @@ -971,19 +965,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // that object as the wrapper. if (wrapperMaybe is not null) { - retValue = wrapperMaybe; - - retValue = AddRCWToCache(identity, retValue, out GCHandle wrapperWeakRef); - - // At this point, retValue is the RCW for the identity. - // Create a NativeObjectWrapper to handle lifetime tracking of the references to the COM object. - CreateWrapperForComProxy( - identity, - inner, - retValue, - wrapperWeakRef, - flags); - + retValue = RegisterObjectForComInstance(identity, inner, wrapperMaybe, flags); return true; } @@ -1004,7 +986,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // If we are asked to create an EOC for B1 with the unwrap flag on the C2 ComWrappers instance, // we will create a new wrapper. In this scenario, we'll only unwrap B2. object unwrapped = ComInterfaceDispatch.GetInstance(comInterfaceDispatch); - if (_ccwTable.TryGetValue(unwrapped, out ManagedObjectWrapperHolder? unwrappedWrapperInThisContext)) + if (_managedObjectWrapperTable.TryGetValue(unwrapped, out ManagedObjectWrapperHolder? unwrappedWrapperInThisContext)) { // The unwrapped object has a CCW in this context. Compare with identity // so we can see if it's the CCW for the unwrapped object in this context. @@ -1025,89 +1007,85 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( return false; } - if (flags.HasFlag(CreateObjectFlags.UniqueInstance)) - { - // We didn't find a cached wrapper for the identity, so we need to create one. - // Don't check for an existing wrapper as we just created the object - // and a wrapper already existing would be exceedingly rare. - CreateWrapperForComProxy( - identity, - inner, - retValue, - GCHandle.Alloc(retValue, GCHandleType.Weak), - flags); - return true; - } - // Now that we've called into user code to create the user object wrapper, // another thread may have beaten us to creating a cached wrapper for the same identity. // Check the cache again to avoid creating a NativeObjectWrapper when we don't need to. - if (FindRCWInCache(identity) is object cachedWrapper) + if (!flags.HasFlag(CreateObjectFlags.UniqueInstance) && FindRCWInCache(identity) is object cachedWrapper) { retValue = cachedWrapper; return true; } - { - retValue = AddRCWToCache(identity, retValue, out GCHandle wrapperWeakRef); + retValue = RegisterObjectForComInstance(identity, inner, retValue, flags); + return true; + } - // At this point, retValue is the RCW for the identity. - // Create a NativeObjectWrapper to handle lifetime tracking of the references to the COM object. - CreateWrapperForComProxy( - identity, - inner, - retValue, - wrapperWeakRef, - flags); + private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, object comProxy, CreateObjectFlags flags) + { + NativeObjectWrapper nativeObjectWrapper = NativeObjectWrapper.Create( + identity, + inner, + this, + comProxy, + flags); + + object actualProxy = comProxy; + if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) + { + // Add our entry to the cache here, using an already existing entry if someone else beat us to it. + actualProxy = AddRCWToCache(identity, nativeObjectWrapper); } - return true; + // At this point, actualProxy is the RCW object for the identity. + // Register the NativeObjectWrapper to handle lifetime tracking of the references to the COM object. + RegisterWrapperForObject(actualProxy, nativeObjectWrapper); + + return actualProxy; } - private void CreateWrapperForComProxy(IntPtr externalComObject, IntPtr inner, object comProxy, GCHandle proxyWeakRef, CreateObjectFlags flags) + private void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWrapper) { - if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? existingWrapper)) + if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper)) { // A native wrapper already exists for this managed object. - // We don't need to do any work. + // Make sure it is for the same COM instance. + CheckForDifferentComInstances(registeredWrapper, newWrapper); return; } - NativeObjectWrapper newWrapper = NativeObjectWrapper.Create( - externalComObject, - inner, - this, - comProxy, - proxyWeakRef, - flags); - - NativeObjectWrapper registeredWrapper = GetValueFromRcwTable(comProxy, newWrapper); + registeredWrapper = GetValueFromRcwTable(comProxy, newWrapper); if (registeredWrapper != newWrapper) { - // We've lost the race to register the native object wrapper for this managed object. - // If the user is not trying to register the same managed object as the managed wrapper - // for two different COM instances, we need to throw an exception. - // Otherwise, we can just release the wrapper we created. - bool wrappingSameUnderlyingComObject = registeredWrapper._externalComObject == newWrapper._externalComObject; - if (!wrappingSameUnderlyingComObject) - { - RemoveRCWFromCache(externalComObject, newWrapper.ProxyHandle); - newWrapper.Release(); - throw new NotSupportedException(); - } - else - { - newWrapper.Release(); - } + // We can race here if the user is trying to register the same managed object as the managed wrapper + // on two different threads. This is okay only if the managed object is being registered as wrapper + // for the same COM instance. + CheckForDifferentComInstances(registeredWrapper, newWrapper); } - // Add the wrapper to the reference tracker handle cache. - // We must always do this whether we won or lost the race to add the wrapper to the table - // as we can't synchronize with the thread that won the race. + // Always register our wrapper to the reference tracker handle cache here. + // We may not be the thread that registered the handle, but we need to ensure that the wrapper + // is registered before we return to user code. Otherwise the wrapper won't be walked by the GC + // and we could end up missing a section of the object graph. + // This cache handles duplicates, so it is okay that the wrapper will be registered multiple times. AddWrapperToReferenceTrackerHandleCache(registeredWrapper); // Separate out into a local function to avoid the closure and delegate allocation unless we need it. static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_nativeObjectWrapperTable.GetValue(userObject, _ => newWrapper); + + void CheckForDifferentComInstances(NativeObjectWrapper registeredWrapper, NativeObjectWrapper newWrapper) + { + if (registeredWrapper.ExternalComObject != newWrapper.ExternalComObject) + { + // We can only race here if the user is trying to register the same managed object as the managed wrapper + // for two different COM instances. + // In this case, we need to throw an exception. + + // Remove this entry from the cache as comProxy won't be the RCW for externalComObject. + RemoveRCWFromCache(newWrapper.ExternalComObject, newWrapper); + newWrapper.Release(); + throw new NotSupportedException(); + } + } } private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) @@ -1118,54 +1096,61 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } - private object AddRCWToCache(IntPtr comPointer, object wrapper, out GCHandle wrapperWeakRef) + private object AddRCWToCache(IntPtr comPointer, NativeObjectWrapper wrapper) { using (_rcwCacheLock.EnterScope()) { - ref GCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); + ref WeakReference? rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); if (!exists) { // Someone else didn't beat us to adding the entry to the cache. // Add our entry here. - rcwEntry = GCHandle.Alloc(wrapper, GCHandleType.Weak); + rcwEntry = new WeakReference(wrapper); + } + + if (!rcwEntry.TryGetTarget(out NativeObjectWrapper? target)) + { + // The target was collected, so we need to update the cache entry. + rcwEntry.SetTarget(target = wrapper); } // Either we added our entry to the cache or // someone else beat us to adding our entry to the cache. - // In either case, we want to return the wrapper that one the race. - wrapperWeakRef = rcwEntry; - return rcwEntry.Target!; + // In either case, we want to return the wrapper that won the race. + return target.ProxyHandle.Target; } } - private object FindRCWInCache(IntPtr comPointer) + private object? FindRCWInCache(IntPtr comPointer) { using (_rcwCacheLock.EnterScope()) { - if (_rcwCache.TryGetValue(comPointer, out var existingHandle)) + if (_rcwCache.TryGetValue(comPointer, out WeakReference? existingHandle)) { - object? cachedWrapper = existingHandle.Target; - if (cachedWrapper is null) + if (!existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) + || cachedWrapper.ProxyHandle.Target == null) { - // The GCHandle has been clear out but the NativeObjectWrapper - // finalizer has not yet run to remove the entry from _rcwCache + // The target was collected, so we need to remove the entry from the cache. _rcwCache.Remove(comPointer); + return null; } - return cachedWrapper; + return cachedWrapper.ProxyHandle.Target; } return null; } } - private void RemoveRCWFromCache(IntPtr comPointer, GCHandle expectedValue) + private void RemoveRCWFromCache(IntPtr comPointer, NativeObjectWrapper expectedValue) { using (_rcwCacheLock.EnterScope()) { // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache // in the time between the GC cleared the contents of the GC handle but before the // NativeObjectWrapper finalizer ran. - if (_rcwCache.TryGetValue(comPointer, out GCHandle cachedValue) && expectedValue == cachedValue) + if (_rcwCache.TryGetValue(comPointer, out WeakReference? cachedRef) + && cachedRef.TryGetTarget(out NativeObjectWrapper? cachedValue) + && expectedValue == cachedValue) { _rcwCache.Remove(comPointer); } From b457f63e26ed24a55e673e812108510ed5872469 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 14:29:38 -0800 Subject: [PATCH 06/16] Extract the RCW cache to its own type and do some more cleanup. --- .../InteropServices/ComWrappers.NativeAot.cs | 130 +++++++++++------- 1 file changed, 77 insertions(+), 53 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 2384c5428a0c66..6e473c366086bf 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 @@ -44,8 +44,7 @@ public abstract partial class ComWrappers private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); private readonly ConditionalWeakTable _managedObjectWrapperTable = new ConditionalWeakTable(); - private readonly Lock _rcwCacheLock = new Lock(useTrivialWaits: true); - private readonly Dictionary> _rcwCache = new Dictionary>(); + private readonly RCWCache _rcwCache = new(); internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId) { @@ -555,7 +554,7 @@ public virtual void Release() { if (_maybeCached) { - _comWrappers.RemoveRCWFromCache(_externalComObject, this); + _comWrappers._rcwCache.Remove(_externalComObject, this); _maybeCached = false; } @@ -955,7 +954,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( { // If we have a live cached wrapper currently, // return that. - if (FindRCWInCache(identity) is object liveCachedWrapper) + if (_rcwCache.FindProxyForComInstance(identity) is object liveCachedWrapper) { retValue = liveCachedWrapper; return true; @@ -1010,7 +1009,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // Now that we've called into user code to create the user object wrapper, // another thread may have beaten us to creating a cached wrapper for the same identity. // Check the cache again to avoid creating a NativeObjectWrapper when we don't need to. - if (!flags.HasFlag(CreateObjectFlags.UniqueInstance) && FindRCWInCache(identity) is object cachedWrapper) + if (!flags.HasFlag(CreateObjectFlags.UniqueInstance) && _rcwCache.FindProxyForComInstance(identity) is object cachedWrapper) { retValue = cachedWrapper; return true; @@ -1033,23 +1032,28 @@ private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, objec if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) { // Add our entry to the cache here, using an already existing entry if someone else beat us to it. - actualProxy = AddRCWToCache(identity, nativeObjectWrapper); + actualProxy = _rcwCache.GetOrAddProxyForComInstance(identity, nativeObjectWrapper); + + // Explicitly keep alive our proxy instance while inserting the wrapper (which only keeps a weak reference) + // into the cache. This ensures that the proxy instance is kept alive until we have the actualProxy instance + // updated to whatever object we are going to use. + GC.KeepAlive(comProxy); } // At this point, actualProxy is the RCW object for the identity. // Register the NativeObjectWrapper to handle lifetime tracking of the references to the COM object. - RegisterWrapperForObject(actualProxy, nativeObjectWrapper); + RegisterWrapperForObject(actualProxy, nativeObjectWrapper, _rcwCache); return actualProxy; } - private void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWrapper) + private static void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWrapper, RCWCache rcwCache) { if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper)) { // A native wrapper already exists for this managed object. // Make sure it is for the same COM instance. - CheckForDifferentComInstances(registeredWrapper, newWrapper); + CheckForDifferentComInstances(registeredWrapper, newWrapper, rcwCache); return; } @@ -1059,7 +1063,7 @@ private void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWr // We can race here if the user is trying to register the same managed object as the managed wrapper // on two different threads. This is okay only if the managed object is being registered as wrapper // for the same COM instance. - CheckForDifferentComInstances(registeredWrapper, newWrapper); + CheckForDifferentComInstances(registeredWrapper, newWrapper, rcwCache); } // Always register our wrapper to the reference tracker handle cache here. @@ -1072,7 +1076,7 @@ private void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWr // Separate out into a local function to avoid the closure and delegate allocation unless we need it. static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_nativeObjectWrapperTable.GetValue(userObject, _ => newWrapper); - void CheckForDifferentComInstances(NativeObjectWrapper registeredWrapper, NativeObjectWrapper newWrapper) + static void CheckForDifferentComInstances(NativeObjectWrapper registeredWrapper, NativeObjectWrapper newWrapper, RCWCache rcwCache) { if (registeredWrapper.ExternalComObject != newWrapper.ExternalComObject) { @@ -1081,7 +1085,7 @@ void CheckForDifferentComInstances(NativeObjectWrapper registeredWrapper, Native // In this case, we need to throw an exception. // Remove this entry from the cache as comProxy won't be the RCW for externalComObject. - RemoveRCWFromCache(newWrapper.ExternalComObject, newWrapper); + rcwCache.Remove(newWrapper.ExternalComObject, newWrapper); newWrapper.Release(); throw new NotSupportedException(); } @@ -1096,63 +1100,83 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } - private object AddRCWToCache(IntPtr comPointer, NativeObjectWrapper wrapper) + internal sealed class RCWCache { - using (_rcwCacheLock.EnterScope()) + private readonly Lock _lock = new Lock(useTrivialWaits: true); + private readonly Dictionary> _cache = []; + + public object GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper) { - ref WeakReference? rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_rcwCache, comPointer, out bool exists); - if (!exists) + lock (_lock) { - // Someone else didn't beat us to adding the entry to the cache. - // Add our entry here. - rcwEntry = new WeakReference(wrapper); - } + object targetObject = wrapper.ProxyHandle.Target!; + Debug.Assert(targetObject is not null, "targetObject should be kept alive in the caller."); + ref WeakReference? rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); + if (!exists) + { + // Someone else didn't beat us to adding the entry to the cache. + // Add our entry here. + rcwEntry = new WeakReference(wrapper); + } + else if (!rcwEntry.TryGetTarget(out NativeObjectWrapper? cachedWrapper)) + { + // The target was collected, so we need to update the cache entry. + rcwEntry.SetTarget(wrapper); + } + 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) + { + // The existing proxy object is still alive, we will use that. + return existingProxy; + } - if (!rcwEntry.TryGetTarget(out NativeObjectWrapper? target)) - { - // The target was collected, so we need to update the cache entry. - rcwEntry.SetTarget(target = wrapper); - } + // The proxy object was collected, so we need to update the cache entry. + rcwEntry.SetTarget(wrapper); + } - // Either we added our entry to the cache or - // someone else beat us to adding our entry to the cache. - // In either case, we want to return the wrapper that won the race. - return target.ProxyHandle.Target; + // We either added an entry to the cache or updated an existing entry that was dead. + // Return our target object. + return targetObject; + } } - } - private object? FindRCWInCache(IntPtr comPointer) - { - using (_rcwCacheLock.EnterScope()) + public object? FindProxyForComInstance(IntPtr comPointer) { - if (_rcwCache.TryGetValue(comPointer, out WeakReference? existingHandle)) + lock (_lock) { - if (!existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) - || cachedWrapper.ProxyHandle.Target == null) + if (_cache.TryGetValue(comPointer, out WeakReference? existingHandle)) { - // The target was collected, so we need to remove the entry from the cache. - _rcwCache.Remove(comPointer); - return null; + if (!existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) + || cachedWrapper.ProxyHandle.Target == null) + { + // The target was collected, so we need to remove the entry from the cache. + _cache.Remove(comPointer); + return null; + } + return cachedWrapper.ProxyHandle.Target; } - return cachedWrapper.ProxyHandle.Target; - } - return null; + return null; + } } - } - private void RemoveRCWFromCache(IntPtr comPointer, NativeObjectWrapper expectedValue) - { - using (_rcwCacheLock.EnterScope()) + public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) { - // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache - // in the time between the GC cleared the contents of the GC handle but before the - // NativeObjectWrapper finalizer ran. - if (_rcwCache.TryGetValue(comPointer, out WeakReference? cachedRef) - && cachedRef.TryGetTarget(out NativeObjectWrapper? cachedValue) - && expectedValue == cachedValue) + lock (_lock) { - _rcwCache.Remove(comPointer); + // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache + // in the time between the GC cleared the contents of the GC handle but before the + // NativeObjectWrapper finalizer ran. + if (_cache.TryGetValue(comPointer, out WeakReference? cachedRef) + && cachedRef.TryGetTarget(out NativeObjectWrapper? cachedValue) + && wrapper == cachedValue) + { + _cache.Remove(comPointer); + } } } } From d78d87091b78c629fd3800bed80ae0687bf41716 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 15:03:06 -0800 Subject: [PATCH 07/16] Pass the NativeObjectWrapper and comProxy as a pair and ensure that there's only ever one NativeObjectWrapper we try to register, the one that's in the RCW cache --- .../InteropServices/ComWrappers.NativeAot.cs | 93 ++++++++++--------- 1 file changed, 49 insertions(+), 44 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 6e473c366086bf..5beef0b990e8d2 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 @@ -1029,67 +1029,66 @@ private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, objec flags); object actualProxy = comProxy; + NativeObjectWrapper actualWrapper = nativeObjectWrapper; if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) { // Add our entry to the cache here, using an already existing entry if someone else beat us to it. - actualProxy = _rcwCache.GetOrAddProxyForComInstance(identity, nativeObjectWrapper); - - // Explicitly keep alive our proxy instance while inserting the wrapper (which only keeps a weak reference) - // into the cache. This ensures that the proxy instance is kept alive until we have the actualProxy instance - // updated to whatever object we are going to use. - GC.KeepAlive(comProxy); + (actualWrapper, actualProxy) = _rcwCache.GetOrAddProxyForComInstance(identity, nativeObjectWrapper, comProxy); + if (actualWrapper != nativeObjectWrapper) + { + // We raced with another thread to map identity to nativeObjectWrapper + // and lost the race. We will use the other thread's nativeObjectWrapper, so we can release ours. + nativeObjectWrapper.Release(); + } } - // At this point, actualProxy is the RCW object for the identity. + // At this point, actualProxy is the RCW object for the identity + // and actualWrapper is the NativeObjectWrapper that is in the RCW cache that associates the identity with actualProxy. // Register the NativeObjectWrapper to handle lifetime tracking of the references to the COM object. - RegisterWrapperForObject(actualProxy, nativeObjectWrapper, _rcwCache); + RegisterWrapperForObject(actualWrapper, actualProxy); return actualProxy; } - private static void RegisterWrapperForObject(object comProxy, NativeObjectWrapper newWrapper, RCWCache rcwCache) + private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comProxy) { - if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper)) + // When we call into RegisterWrapperForObject, there is only one valid wrapper for a given + // COM instance. If we find a wrapper in the table that is a different NativeObjectWrapper instance + // then it must be for a different COM instance. + // It's possible that we could race here with another thread that is trying to register the same comProxy + // for the same COM instance, but in that case we'll be pased 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(_rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy); + + if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper) + && registeredWrapper != wrapper) { - // A native wrapper already exists for this managed object. - // Make sure it is for the same COM instance. - CheckForDifferentComInstances(registeredWrapper, newWrapper, rcwCache); - return; + Debug.Assert(registeredWrapper.ExternalComObject != wrapper.ExternalComObject); + _rcwCache.Remove(wrapper.ExternalComObject, wrapper); + wrapper.Release(); + throw new NotSupportedException(); } - registeredWrapper = GetValueFromRcwTable(comProxy, newWrapper); - if (registeredWrapper != newWrapper) + registeredWrapper = GetValueFromRcwTable(comProxy, wrapper); + if (registeredWrapper != wrapper) { - // We can race here if the user is trying to register the same managed object as the managed wrapper - // on two different threads. This is okay only if the managed object is being registered as wrapper - // for the same COM instance. - CheckForDifferentComInstances(registeredWrapper, newWrapper, rcwCache); + Debug.Assert(registeredWrapper.ExternalComObject != wrapper.ExternalComObject); + _rcwCache.Remove(wrapper.ExternalComObject, wrapper); + wrapper.Release(); + throw new NotSupportedException(); } // Always register our wrapper to the reference tracker handle cache here. // We may not be the thread that registered the handle, but we need to ensure that the wrapper - // is registered before we return to user code. Otherwise the wrapper won't be walked by the GC - // and we could end up missing a section of the object graph. - // This cache handles duplicates, so it is okay that the wrapper will be registered multiple times. + // is registered before we return to user code. Otherwise the wrapper won't be walked by the + // TrackerObjectManager and we could end up missing a section of the object graph. + // This cache deduplicates, so it is okay that the wrapper will be registered multiple times. AddWrapperToReferenceTrackerHandleCache(registeredWrapper); // Separate out into a local function to avoid the closure and delegate allocation unless we need it. static NativeObjectWrapper GetValueFromRcwTable(object userObject, NativeObjectWrapper newWrapper) => s_nativeObjectWrapperTable.GetValue(userObject, _ => newWrapper); - - static void CheckForDifferentComInstances(NativeObjectWrapper registeredWrapper, NativeObjectWrapper newWrapper, RCWCache rcwCache) - { - if (registeredWrapper.ExternalComObject != newWrapper.ExternalComObject) - { - // We can only race here if the user is trying to register the same managed object as the managed wrapper - // for two different COM instances. - // In this case, we need to throw an exception. - - // Remove this entry from the cache as comProxy won't be the RCW for externalComObject. - rcwCache.Remove(newWrapper.ExternalComObject, newWrapper); - newWrapper.Release(); - throw new NotSupportedException(); - } - } } private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) @@ -1100,17 +1099,23 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } - internal sealed class RCWCache + private sealed class RCWCache { private readonly Lock _lock = new Lock(useTrivialWaits: true); private readonly Dictionary> _cache = []; - public object GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper) + /// + /// Gets the current RCW proxy object for if it exists in the cache or inserts a new entry with . + /// + /// The com instance we want to get or record an RCW for. + /// The for . + /// The proxy object that is associated with . + /// The proxy object currently in the cache for or the proxy object owned by if no entry exists and the corresponding native wrapper. + public (NativeObjectWrapper actualWrapper, object actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { lock (_lock) { - object targetObject = wrapper.ProxyHandle.Target!; - Debug.Assert(targetObject is not null, "targetObject should be kept alive in the caller."); + Debug.Assert(wrapper.ProxyHandle.Target == comProxy); ref WeakReference? rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); if (!exists) { @@ -1131,7 +1136,7 @@ public object GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper if (existingProxy is not null) { // The existing proxy object is still alive, we will use that. - return existingProxy; + return (cachedWrapper, existingProxy); } // The proxy object was collected, so we need to update the cache entry. @@ -1140,7 +1145,7 @@ public object GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper // We either added an entry to the cache or updated an existing entry that was dead. // Return our target object. - return targetObject; + return (wrapper, comProxy); } } From 0e9dd87c3be67968169a6c692557be35a398621e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 15:14:59 -0800 Subject: [PATCH 08/16] Reorganize code to reduce indentation --- .../InteropServices/ComWrappers.NativeAot.cs | 90 +++++++++++-------- 1 file changed, 51 insertions(+), 39 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 5beef0b990e8d2..038017072a951d 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 @@ -950,50 +950,63 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( using ComHolder releaseIdentity = new ComHolder(identity); - if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) + // If the user has requested a unique instance, + // we will immediately create the object, register it, + // and return. + if (flags.HasFlag(CreateObjectFlags.UniqueInstance)) { - // If we have a live cached wrapper currently, - // return that. - if (_rcwCache.FindProxyForComInstance(identity) is object liveCachedWrapper) + retValue = CreateObject(identity, flags); + if (retValue == null) { - retValue = liveCachedWrapper; - return true; + // If ComWrappers instance cannot create wrapper, we can do nothing here. + return false; } - // If the user tried to provide a pre-created managed wrapper, try to register - // that object as the wrapper. - if (wrapperMaybe is not null) - { - retValue = RegisterObjectForComInstance(identity, inner, wrapperMaybe, flags); - return true; - } + retValue = RegisterObjectForComInstance(identity, inner, retValue, flags); + return true; + } + + // If we have a live cached wrapper currently, + // return that. + if (_rcwCache.FindProxyForComInstance(identity) is object liveCachedWrapper) + { + retValue = liveCachedWrapper; + return true; + } + + // If the user tried to provide a pre-created managed wrapper, try to register + // that object as the wrapper. + if (wrapperMaybe is not null) + { + retValue = RegisterObjectForComInstance(identity, inner, wrapperMaybe, flags); + return true; + } - // Check if the provided COM instance is actually a managed object wrapper from this - // ComWrappers instance, and use it if it is. - if (flags.HasFlag(CreateObjectFlags.Unwrap)) + // Check if the provided COM instance is actually a managed object wrapper from this + // ComWrappers instance, and use it if it is. + if (flags.HasFlag(CreateObjectFlags.Unwrap)) + { + ComInterfaceDispatch* comInterfaceDispatch = TryGetComInterfaceDispatch(identity); + if (comInterfaceDispatch != null) { - ComInterfaceDispatch* comInterfaceDispatch = TryGetComInterfaceDispatch(identity); - if (comInterfaceDispatch != null) + // If we found a managed object wrapper in this ComWrappers instance + // and it's has the same identity pointer as the one we're creating a NativeObjectWrapper for, + // unwrap it. We don't AddRef the wrapper as we don't take a reference to it. + // + // A managed object can have multiple managed object wrappers, with a max of one per context. + // Let's say we have a managed object A and ComWrappers instances C1 and C2. Let B1 and B2 be the + // managed object wrappers for A created with C1 and C2 respectively. + // If we are asked to create an EOC for B1 with the unwrap flag on the C2 ComWrappers instance, + // we will create a new wrapper. In this scenario, we'll only unwrap B2. + object unwrapped = ComInterfaceDispatch.GetInstance(comInterfaceDispatch); + if (_managedObjectWrapperTable.TryGetValue(unwrapped, out ManagedObjectWrapperHolder? unwrappedWrapperInThisContext)) { - // If we found a managed object wrapper in this ComWrappers instance - // and it's has the same identity pointer as the one we're creating a NativeObjectWrapper for, - // unwrap it. We don't AddRef the wrapper as we don't take a reference to it. - // - // A managed object can have multiple managed object wrappers, with a max of one per context. - // Let's say we have a managed object A and ComWrappers instances C1 and C2. Let B1 and B2 be the - // managed object wrappers for A created with C1 and C2 respectively. - // If we are asked to create an EOC for B1 with the unwrap flag on the C2 ComWrappers instance, - // we will create a new wrapper. In this scenario, we'll only unwrap B2. - object unwrapped = ComInterfaceDispatch.GetInstance(comInterfaceDispatch); - if (_managedObjectWrapperTable.TryGetValue(unwrapped, out ManagedObjectWrapperHolder? unwrappedWrapperInThisContext)) + // The unwrapped object has a CCW in this context. Compare with identity + // so we can see if it's the CCW for the unwrapped object in this context. + if (unwrappedWrapperInThisContext.ComIp == identity) { - // The unwrapped object has a CCW in this context. Compare with identity - // so we can see if it's the CCW for the unwrapped object in this context. - if (unwrappedWrapperInThisContext.ComIp == identity) - { - retValue = unwrapped; - return true; - } + retValue = unwrapped; + return true; } } } @@ -1009,7 +1022,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // Now that we've called into user code to create the user object wrapper, // another thread may have beaten us to creating a cached wrapper for the same identity. // Check the cache again to avoid creating a NativeObjectWrapper when we don't need to. - if (!flags.HasFlag(CreateObjectFlags.UniqueInstance) && _rcwCache.FindProxyForComInstance(identity) is object cachedWrapper) + if (_rcwCache.FindProxyForComInstance(identity) is object cachedWrapper) { retValue = cachedWrapper; return true; @@ -1043,7 +1056,7 @@ private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, objec } // At this point, actualProxy is the RCW object for the identity - // and actualWrapper is the NativeObjectWrapper that is in the RCW cache that associates the identity with actualProxy. + // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. // Register the NativeObjectWrapper to handle lifetime tracking of the references to the COM object. RegisterWrapperForObject(actualWrapper, actualProxy); @@ -1060,7 +1073,6 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro // 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(_rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy); if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper) && registeredWrapper != wrapper) From f1e41f8d9c2bb4ef44c591f563ad4c12672a0913 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 15:28:56 -0800 Subject: [PATCH 09/16] A little more refactoring --- .../InteropServices/ComWrappers.NativeAot.cs | 58 ++++++++----------- 1 file changed, 25 insertions(+), 33 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 038017072a951d..f17435c2bb3be4 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 @@ -492,11 +492,11 @@ internal unsafe class NativeObjectWrapper { private IntPtr _externalComObject; private IntPtr _inner; - private readonly ComWrappers _comWrappers; + private ComWrappers _comWrappers; private GCHandle _proxyHandle; private GCHandle _proxyHandleTrackingResurrection; internal readonly bool _aggregatedManagedObjectWrapper; - private bool _maybeCached; + private readonly bool _uniqueInstance; static NativeObjectWrapper() { @@ -524,7 +524,7 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper _externalComObject = externalComObject; _inner = inner; _comWrappers = comWrappers; - _maybeCached = !flags.HasFlag(CreateObjectFlags.UniqueInstance); + _uniqueInstance = !flags.HasFlag(CreateObjectFlags.UniqueInstance); _proxyHandle = GCHandle.Alloc(comProxy, GCHandleType.Weak); // We have a separate handle tracking resurrection as we want to make sure @@ -549,13 +549,14 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper internal IntPtr ExternalComObject => _externalComObject; internal ComWrappers ComWrappers => _comWrappers; internal GCHandle ProxyHandle => _proxyHandle; + internal bool IsUniqueInstance => _uniqueInstance; public virtual void Release() { - if (_maybeCached) + if (!_uniqueInstance && _comWrappers is not null) { _comWrappers._rcwCache.Remove(_externalComObject, this); - _maybeCached = false; + _comWrappers = null; } if (_proxyHandle.IsAllocated) @@ -955,15 +956,8 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( // and return. if (flags.HasFlag(CreateObjectFlags.UniqueInstance)) { - retValue = CreateObject(identity, flags); - if (retValue == null) - { - // If ComWrappers instance cannot create wrapper, we can do nothing here. - return false; - } - - retValue = RegisterObjectForComInstance(identity, inner, retValue, flags); - return true; + retValue = CreateAndRegisterObjectForComInstance(identity, inner, flags); + return retValue is not null; } // If we have a live cached wrapper currently, @@ -1012,24 +1006,22 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( } } - retValue = CreateObject(identity, flags); - if (retValue == null) - { - // If ComWrappers instance cannot create wrapper, we can do nothing here. - return false; - } + // If the user didn't provide a wrapper and couldn't unwrap a managed object wrapper, + // create a new wrapper. + retValue = CreateAndRegisterObjectForComInstance(identity, inner, flags); + return retValue is not null; + } - // Now that we've called into user code to create the user object wrapper, - // another thread may have beaten us to creating a cached wrapper for the same identity. - // Check the cache again to avoid creating a NativeObjectWrapper when we don't need to. - if (_rcwCache.FindProxyForComInstance(identity) is object cachedWrapper) + private object? CreateAndRegisterObjectForComInstance(IntPtr identity, IntPtr inner, CreateObjectFlags flags) + { + object? retValue = CreateObject(identity, flags); + if (retValue is null) { - retValue = cachedWrapper; - return true; + // If ComWrappers instance cannot create wrapper, we can do nothing here. + return null; } - retValue = RegisterObjectForComInstance(identity, inner, retValue, flags); - return true; + return RegisterObjectForComInstance(identity, inner, retValue, flags); } private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, object comProxy, CreateObjectFlags flags) @@ -1043,7 +1035,7 @@ private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, objec object actualProxy = comProxy; NativeObjectWrapper actualWrapper = nativeObjectWrapper; - if (!flags.HasFlag(CreateObjectFlags.UniqueInstance)) + if (!nativeObjectWrapper.IsUniqueInstance) { // Add our entry to the cache here, using an already existing entry if someone else beat us to it. (actualWrapper, actualProxy) = _rcwCache.GetOrAddProxyForComInstance(identity, nativeObjectWrapper, comProxy); @@ -1065,20 +1057,21 @@ private object RegisterObjectForComInstance(IntPtr identity, IntPtr inner, objec private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comProxy) { - // When we call into RegisterWrapperForObject, there is only one valid wrapper for a given - // COM instance. If we find a wrapper in the table that is a different NativeObjectWrapper instance + // When we call into RegisterWrapperForObject, there is only one valid non-"unique instance" wrapper for a given + // COM instance, which is already registered in the RCW cache. + // If we find a wrapper in the table that is a different NativeObjectWrapper instance // then it must be for a different COM instance. // It's possible that we could race here with another thread that is trying to register the same comProxy // for the same COM instance, but in that case we'll be pased 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.IsUniqueInstance || _rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy); if (s_nativeObjectWrapperTable.TryGetValue(comProxy, out NativeObjectWrapper? registeredWrapper) && registeredWrapper != wrapper) { Debug.Assert(registeredWrapper.ExternalComObject != wrapper.ExternalComObject); - _rcwCache.Remove(wrapper.ExternalComObject, wrapper); wrapper.Release(); throw new NotSupportedException(); } @@ -1087,7 +1080,6 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro if (registeredWrapper != wrapper) { Debug.Assert(registeredWrapper.ExternalComObject != wrapper.ExternalComObject); - _rcwCache.Remove(wrapper.ExternalComObject, wrapper); wrapper.Release(); throw new NotSupportedException(); } From 9a0f7a63fcef225f7a897af89d0af447c64d9629 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 20 Dec 2024 16:10:27 -0800 Subject: [PATCH 10/16] Rename based on our naming conventions. --- .../System/Runtime/InteropServices/ComWrappers.NativeAot.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 f17435c2bb3be4..7996b22a518bfa 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 @@ -44,7 +44,7 @@ public abstract partial class ComWrappers private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); private readonly ConditionalWeakTable _managedObjectWrapperTable = new ConditionalWeakTable(); - private readonly RCWCache _rcwCache = new(); + private readonly RcwCache _rcwCache = new(); internal static bool TryGetComInstanceForIID(object obj, Guid iid, out IntPtr unknown, out long wrapperId) { @@ -1103,7 +1103,7 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper } } - private sealed class RCWCache + private sealed class RcwCache { private readonly Lock _lock = new Lock(useTrivialWaits: true); private readonly Dictionary> _cache = []; From 068b9130fade78d892e5bddb895bd15813113b70 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Dec 2024 12:04:04 -0800 Subject: [PATCH 11/16] Fix UniqueInstance flag --- .../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 7996b22a518bfa..2430ad5d11c611 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 @@ -524,7 +524,7 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper _externalComObject = externalComObject; _inner = inner; _comWrappers = comWrappers; - _uniqueInstance = !flags.HasFlag(CreateObjectFlags.UniqueInstance); + _uniqueInstance = flags.HasFlag(CreateObjectFlags.UniqueInstance); _proxyHandle = GCHandle.Alloc(comProxy, GCHandleType.Weak); // We have a separate handle tracking resurrection as we want to make sure From d5fa98124eea605456953c2c1afcbc70a1ea5a3d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Dec 2024 12:33:46 -0800 Subject: [PATCH 12/16] Add a test for the deadlock --- .../Interop/COM/ComWrappers/API/Program.cs | 69 +++++++++++++++++++ src/tests/Interop/COM/ComWrappers/Common.cs | 2 +- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 7ef5d29c0ab635..c3d02c31cc1174 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -10,6 +10,7 @@ namespace ComWrappersTests using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Runtime.InteropServices.Marshalling; + using System.Threading; using ComWrappersTests.Common; using TestLibrary; @@ -962,6 +963,74 @@ public void ValidateAggregationWithReferenceTrackerObject() Assert.Equal(0, allocTracker.GetCount()); } + + [Fact] + public void ComWrappersNoLockAroundQueryInterface() + { + Console.WriteLine($"Running {nameof(ComWrappersNoLockAroundQueryInterface)}..."); + + var cw = new RecursiveSimpleComWrappers(); + + IntPtr comObject = cw.GetOrCreateComInterfaceForObject(new RecursiveCrossThreadQI(cw), CreateComInterfaceFlags.None); + try + { + _ = cw.GetOrCreateObjectForComInstance(comObject, CreateObjectFlags.TrackerObject); + } + finally + { + Marshal.Release(comObject); + } + } + + private class RecursiveCrossThreadQI(ComWrappers? wrappers) : ICustomQueryInterface + { + CustomQueryInterfaceResult ICustomQueryInterface.GetInterface(ref Guid iid, out IntPtr ppv) + { + ppv = IntPtr.Zero; + if (iid == ComWrappersHelper.IID_IReferenceTracker && wrappers is not null) + { + Console.WriteLine("Attempting to create a new COM object on a different thread."); + Thread thread = new Thread(() => + { + IntPtr comObject = wrappers.GetOrCreateComInterfaceForObject(new RecursiveCrossThreadQI(null), CreateComInterfaceFlags.None); + try + { + // Make sure that ComWrappers isn't locking in GetOrCreateObjectForComInstance + // around the QI call by calling it on a different thread from within a QI call to register a new managed wrapper + // for a COM object representing "this". + _ = wrappers.GetOrCreateObjectForComInstance(comObject, CreateObjectFlags.None); + } + finally + { + Marshal.Release(comObject); + } + }); + thread.Start(); + thread.Join(TimeSpan.FromSeconds(20)); // 20 seconds should be more than long enough for the thread to complete + } + + return CustomQueryInterfaceResult.Failed; + } + } + + private unsafe class RecursiveSimpleComWrappers : ComWrappers + { + protected override ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) + { + count = 0; + return null; + } + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) + { + return new object(); + } + + protected override void ReleaseObjects(IEnumerable objects) + { + throw new NotImplementedException(); + } + } } } diff --git a/src/tests/Interop/COM/ComWrappers/Common.cs b/src/tests/Interop/COM/ComWrappers/Common.cs index 96d21a349d69dc..dbcefa47ac175e 100644 --- a/src/tests/Interop/COM/ComWrappers/Common.cs +++ b/src/tests/Interop/COM/ComWrappers/Common.cs @@ -339,7 +339,7 @@ CustomQueryInterfaceResult ICustomQueryInterface.GetInterface(ref Guid iid, out class ComWrappersHelper { - private static Guid IID_IReferenceTracker = new Guid("11d3b13a-180e-4789-a8be-7712882893e6"); + public static readonly Guid IID_IReferenceTracker = new Guid("11d3b13a-180e-4789-a8be-7712882893e6"); [Flags] public enum ReleaseFlags From ebdead56c3eef1322ac12d28ca198df8d39f072f Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 23 Dec 2024 15:05:26 -0800 Subject: [PATCH 13/16] Use manually-managed weak GCHandles to reduce allocations. Co-authored-by: Sergio Pedri --- .../InteropServices/ComWrappers.NativeAot.cs | 37 +++++++++++-------- 1 file changed, 21 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 2430ad5d11c611..551c1a37aa9e85 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 @@ -1106,7 +1106,7 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper private sealed class RcwCache { private readonly Lock _lock = new Lock(useTrivialWaits: true); - private readonly Dictionary> _cache = []; + private readonly Dictionary _cache = []; /// /// Gets the current RCW proxy object for if it exists in the cache or inserts a new entry with . @@ -1120,17 +1120,18 @@ private sealed class RcwCache lock (_lock) { Debug.Assert(wrapper.ProxyHandle.Target == comProxy); - ref WeakReference? rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); + ref GCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); if (!exists) { // Someone else didn't beat us to adding the entry to the cache. // Add our entry here. - rcwEntry = new WeakReference(wrapper); + rcwEntry = GCHandle.Alloc(wrapper, GCHandleType.Weak); } - else if (!rcwEntry.TryGetTarget(out NativeObjectWrapper? cachedWrapper)) + else if (rcwEntry.Target is not (NativeObjectWrapper cachedWrapper)) { + Debug.Assert(rcwEntry.IsAllocated); // The target was collected, so we need to update the cache entry. - rcwEntry.SetTarget(wrapper); + rcwEntry.Target = wrapper; } else { @@ -1144,7 +1145,7 @@ private sealed class RcwCache } // The proxy object was collected, so we need to update the cache entry. - rcwEntry.SetTarget(wrapper); + rcwEntry.Target = wrapper; } // We either added an entry to the cache or updated an existing entry that was dead. @@ -1157,16 +1158,17 @@ private sealed class RcwCache { lock (_lock) { - if (_cache.TryGetValue(comPointer, out WeakReference? existingHandle)) + if (_cache.TryGetValue(comPointer, out GCHandle existingHandle)) { - if (!existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) - || cachedWrapper.ProxyHandle.Target == null) + if (existingHandle.Target is NativeObjectWrapper { ProxyHandle.Target: object cachedProxy }) { - // The target was collected, so we need to remove the entry from the cache. - _cache.Remove(comPointer); - return null; + // The target exists and is still alive. Return it. + return cachedProxy; } - return cachedWrapper.ProxyHandle.Target; + + // The target was collected, so we need to remove the entry from the cache. + _cache.Remove(comPointer); + existingHandle.Free(); } return null; @@ -1180,11 +1182,14 @@ public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) // TryGetOrCreateObjectForComInstanceInternal may have put a new entry into the cache // in the time between the GC cleared the contents of the GC handle but before the // NativeObjectWrapper finalizer ran. - if (_cache.TryGetValue(comPointer, out WeakReference? cachedRef) - && cachedRef.TryGetTarget(out NativeObjectWrapper? cachedValue) - && wrapper == cachedValue) + // Only remove the entry if the target of the GC handle is the NativeObjectWrapper + // or is null (indicating that the corresponding NativeObjectWrapper has been scheduled for finalization). + if (_cache.TryGetValue(comPointer, out GCHandle cachedRef) + && (wrapper == cachedRef.Target + || cachedRef.Target is null)) { _cache.Remove(comPointer); + cachedRef.Free(); } } } From 40b994ebae9e7042b3fefef723b4bd1ab85934ec Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 27 Dec 2024 11:07:08 -0800 Subject: [PATCH 14/16] PR feedback --- .../Runtime/InteropServices/ComWrappers.NativeAot.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 551c1a37aa9e85..ae549fc05545d1 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 @@ -495,7 +495,7 @@ internal unsafe class NativeObjectWrapper private ComWrappers _comWrappers; private GCHandle _proxyHandle; private GCHandle _proxyHandleTrackingResurrection; - internal readonly bool _aggregatedManagedObjectWrapper; + private readonly bool _aggregatedManagedObjectWrapper; private readonly bool _uniqueInstance; static NativeObjectWrapper() @@ -550,6 +550,7 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper internal ComWrappers ComWrappers => _comWrappers; internal GCHandle ProxyHandle => _proxyHandle; internal bool IsUniqueInstance => _uniqueInstance; + internal bool IsAggregatedWithManagedObjectWrapper => _aggregatedManagedObjectWrapper; public virtual void Release() { @@ -984,7 +985,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( if (comInterfaceDispatch != null) { // If we found a managed object wrapper in this ComWrappers instance - // and it's has the same identity pointer as the one we're creating a NativeObjectWrapper for, + // and it has the same identity pointer as the one we're creating a NativeObjectWrapper for, // unwrap it. We don't AddRef the wrapper as we don't take a reference to it. // // A managed object can have multiple managed object wrappers, with a max of one per context. @@ -1700,7 +1701,7 @@ private static unsafe bool PossiblyComObject(object target) // If the RCW is an aggregated RCW, then the managed object cannot be recreated from the IUnknown // as the outer IUnknown wraps the managed object. In this case, don't create a weak reference backed // by a COM weak reference. - return s_nativeObjectWrapperTable.TryGetValue(target, out NativeObjectWrapper? wrapper) && !wrapper._aggregatedManagedObjectWrapper; + return s_nativeObjectWrapperTable.TryGetValue(target, out NativeObjectWrapper? wrapper) && !wrapper.IsAggregatedWithManagedObjectWrapper; } private static unsafe IntPtr ObjectToComWeakRef(object target, out long wrapperId) From b46e3a8546bb4b9fb5f53848aa3353468760e571 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 6 Jan 2025 10:57:24 -0800 Subject: [PATCH 15/16] Add a cross-apartment test (Windows-only obviously) --- .../Interop/COM/ComWrappers/API/Program.cs | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index c3d02c31cc1174..3ed01e8604ad3e 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -1031,6 +1031,99 @@ protected override void ReleaseObjects(IEnumerable objects) throw new NotImplementedException(); } } + + [Fact] + [PlatformSpecific(TestPlatforms.Windows)] // COM apartments are Windows-specific + public unsafe void CrossApartmentQueryInterface_NoDeadlock() + { + Console.WriteLine($"Running {nameof(CrossApartmentQueryInterface_NoDeadlock)}..."); + using ManualResetEvent hasAgileReference = new(false); + using ManualResetEvent testCompleted = new(false); + + IntPtr agileReference = IntPtr.Zero; + try + { + Thread staThread = new(() => + { + var cw = new RecursiveSimpleComWrappers(); + IntPtr comObject = cw.GetOrCreateComInterfaceForObject(new RecursiveQI(cw), CreateComInterfaceFlags.None); + try + { + Marshal.ThrowExceptionForHR(RoGetAgileReference(0, IUnknownVtbl.IID_IUnknown, comObject, out agileReference)); + } + finally + { + Marshal.Release(comObject); + } + hasAgileReference.Set(); + testCompleted.WaitOne(); + }); + staThread.SetApartmentState(ApartmentState.STA); + + Thread mtaThread = new(() => + { + hasAgileReference.WaitOne(); + IntPtr comObject; + int hr = ((delegate* unmanaged)(*(*(void***)agileReference + 3 /* IAgileReference.Resolve slot */)))(agileReference, IUnknownVtbl.IID_IUnknown, out comObject); + Marshal.ThrowExceptionForHR(hr); + try + { + var cw = new RecursiveSimpleComWrappers(); + // Make sure that ComWrappers isn't locking in GetOrCreateObjectForComInstance + // across the QI call + // by forcing marshalling across COM apartments. + _ = cw.GetOrCreateObjectForComInstance(comObject, CreateObjectFlags.TrackerObject); + } + finally + { + Marshal.Release(comObject); + } + testCompleted.Set(); + }); + mtaThread.SetApartmentState(ApartmentState.MTA); + + staThread.Start(); + mtaThread.Start(); + } + finally + { + if (agileReference != IntPtr.Zero) + { + Marshal.Release(agileReference); + } + } + + testCompleted.WaitOne(); + } + + [DllImport("ole32.dll")] + private static extern int RoGetAgileReference(int options, in Guid iid, IntPtr unknown, out IntPtr agileReference); + + private class RecursiveQI(ComWrappers? wrappers) : ICustomQueryInterface + { + CustomQueryInterfaceResult ICustomQueryInterface.GetInterface(ref Guid iid, out IntPtr ppv) + { + ppv = IntPtr.Zero; + if (wrappers is not null) + { + Console.WriteLine("Attempting to create a new COM object on the same thread."); + IntPtr comObject = wrappers.GetOrCreateComInterfaceForObject(new RecursiveQI(null), CreateComInterfaceFlags.None); + try + { + // Make sure that ComWrappers isn't locking in GetOrCreateObjectForComInstance + // around the QI call by calling it on a different thread from within a QI call to register a new managed wrapper + // for a COM object representing "this". + _ = wrappers.GetOrCreateObjectForComInstance(comObject, CreateObjectFlags.None); + } + finally + { + Marshal.Release(comObject); + } + } + + return CustomQueryInterfaceResult.Failed; + } + } } } From 9cc64e711f428120ec4035dfde0c5ca1e6d23667 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 6 Jan 2025 11:30:42 -0800 Subject: [PATCH 16/16] PR feedback --- .../System/Runtime/InteropServices/ComWrappers.NativeAot.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 ae549fc05545d1..87eb31d58022fe 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 @@ -974,7 +974,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( if (wrapperMaybe is not null) { retValue = RegisterObjectForComInstance(identity, inner, wrapperMaybe, flags); - return true; + return retValue is not null; } // Check if the provided COM instance is actually a managed object wrapper from this @@ -1063,7 +1063,7 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro // If we find a wrapper in the table that is a different NativeObjectWrapper instance // then it must be for a different COM instance. // It's possible that we could race here with another thread that is trying to register the same comProxy - // for the same COM instance, but in that case we'll be pased the same NativeObjectWrapper instance + // 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);