From 069bc4d7a28849e8358b2a38eaf02075e60379ff Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 1 Sep 2026 06:26:54 -0700 Subject: [PATCH 1/7] Let the RCW cache share the wrapper's handle to its RCW The cache stored a weak handle to the NativeObjectWrapper, which then held a second weak handle to the RCW itself. Every cached RCW therefore cost two GC handles, and finding one meant dereferencing both of them. The wrapper is reachable from the RCW it tracks, through the wrapper table, so a handle to the RCW keeps exactly the same entries alive as a handle to the wrapper did. The cache now stores a copy of the handle the wrapper already had, which halves the handles a cached RCW costs, takes the handle allocation out of the write lock, and removes an indirection from the lookup that every native to managed transition performs. Every handle in the cache belongs to the wrapper that created it, so the cache only ever drops entries and never frees them, and NativeObjectWrapper.Release removes its entry before freeing its handle, so an entry can never name a handle that has been freed. Entries are identified by the handle rather than by what it points at, because once an RCW is collected several dead entries are indistinguishable by their targets. An entry is resolved back to its wrapper through the wrapper table, so an RCW is registered there before its entry is published, which also closes a window where another thread could find an entry whose wrapper was about to be released. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Runtime/InteropServices/ComWrappers.cs | 190 +++++++++++------- 1 file changed, 113 insertions(+), 77 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs index 56b882e88c8c65..595b36499c2067 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs @@ -634,6 +634,12 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper internal IntPtr ExternalComObject => _externalComObject; internal ComWrappers ComWrappers => _comWrappers; + + /// + /// The handle to the RCW this wrapper tracks. This wrapper owns it, and is what frees it, but the RCW + /// cache stores copies of it as its entries rather than allocating a second handle per RCW, so it must + /// outlive every entry referring to it. + /// internal WeakGCHandle ProxyHandle => _proxyHandle; internal bool IsUniqueInstance => _uniqueInstance; internal bool IsAggregatedWithManagedObjectWrapper => _aggregatedManagedObjectWrapper; @@ -642,6 +648,8 @@ public virtual void Release() { if (!_uniqueInstance && _comWrappers is not null) { + // The RCW cache shares this handle rather than allocating one of its own, so the entry has to be + // dropped before the handle is freed below, or the cache would be left holding a freed handle. _comWrappers._rcwCache.Remove(_externalComObject, this); _comWrappers = null!; } @@ -1253,59 +1261,54 @@ private object RegisterObjectForComInstance( flags, ref referenceTrackerMaybe); - object actualProxy = comProxy; - NativeObjectWrapper actualWrapper = nativeObjectWrapper; - if (!nativeObjectWrapper.IsUniqueInstance) + NativeObjectWrapper actualWrapper; + object? actualProxy; + + if (nativeObjectWrapper.IsUniqueInstance) + { + // A unique instance is never published in the RCW cache, so its registration has nothing to be + // atomic with, and there is no cache entry for another thread to win. + actualWrapper = s_nativeObjectWrapperTable.GetOrAdd(comProxy, nativeObjectWrapper); + actualProxy = actualWrapper == nativeObjectWrapper ? comProxy : null; + } + else { // Add our entry to the cache here, using an already existing entry if someone else beat us to it. + // The RCW is registered in the wrapper table in there as well, so that no thread can ever find the + // entry before the registration it needs to resolve that entry back to a wrapper. (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 - // 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); + if (actualProxy is null) + { + // The object 'CreateObject' handed back is already the RCW for another COM instance, which is not + // something ComWrappers can represent. This is reported out here rather than where it is detected, + // because releasing the wrapper takes the same cache lock the registration above runs under. + Debug.Assert(actualWrapper.ExternalComObject != nativeObjectWrapper.ExternalComObject); - return actualProxy; - } + nativeObjectWrapper.Release(); - private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comProxy) - { - // 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 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.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy); - Debug.Assert(wrapper.IsUniqueInstance || _rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy); - - // Add the input wrapper bound to the COM proxy, if there isn't one already. If another thread raced - // against this one and this lost, we'd get the wrapper added from that thread instead. - NativeObjectWrapper registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(comProxy, wrapper); - - // We lost the race, so we cannot register the incoming wrapper with the target object - if (registeredWrapper != wrapper) - { - Debug.Assert(registeredWrapper.ExternalComObject != wrapper.ExternalComObject); - wrapper.Release(); throw new NotSupportedException(); } + 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 + // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. + // // 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 // 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); + AddWrapperToReferenceTrackerHandleCache(actualWrapper); + + return actualProxy; } private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) @@ -1322,13 +1325,21 @@ internal void RemoveWrappersFromCache(IEnumerable wrappers) } /// - /// The cache mapping COM instances to the objects tracking their RCWs. + /// The cache mapping COM instances to the RCWs wrapping them. /// /// + /// + /// An entry names the RCW rather than the tracking it, and does so through + /// a copy of the handle that wrapper already keeps to it, so that a cached RCW costs no handle of its own. + /// The wrapper for a cached RCW is resolved through , which is why + /// an RCW is put in there before its entry is published rather than after. + /// + /// /// The cache is partitioned into several independent buckets, each with its own lock, so that operations on /// COM instances that map to different buckets don't contend with one another. Reducing that contention is /// important because the cache is consulted on essentially every transition from native to managed code, and /// because the finalizer thread concurrently takes write locks to remove entries for collected RCWs. + /// /// private readonly struct RcwCache { @@ -1386,8 +1397,11 @@ private unsafe ref readonly Bucket GetBucket(IntPtr comPointer) /// 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) + /// + /// The proxy object currently in the cache for or the proxy object owned by if no entry exists and the corresponding native wrapper. + /// The proxy object is if is already registered with another wrapper, in which case that wrapper is returned. + /// + public (NativeObjectWrapper actualWrapper, object? actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { return GetBucket(comPointer).GetOrAddProxyForComInstance(comPointer, wrapper, comProxy); } @@ -1441,7 +1455,7 @@ public void RemoveAll(IEnumerable wrappers) private readonly struct Bucket { private readonly ReaderWriterLockSlim _lock; - private readonly Dictionary> _cache; + private readonly Dictionary> _cache; public Bucket() { @@ -1450,41 +1464,67 @@ public Bucket() } /// - public (NativeObjectWrapper actualWrapper, object actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) + public (NativeObjectWrapper actualWrapper, object? actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { _lock.EnterWriteLock(); try { Debug.Assert(wrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy); - ref WeakGCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); - if (!exists) + + ref WeakGCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); + + if (exists && rcwEntry.TryGetTarget(out object? existingProxy)) + { + // Someone else beat us to adding the entry and their RCW is still alive, so that is the + // one to use. Its wrapper was put in the table under this same lock before the entry was + // published, so it is guaranteed to be visible here. + bool found = s_nativeObjectWrapperTable.TryGetValue(existingProxy, out NativeObjectWrapper? existingWrapper); + + Debug.Assert(found); + + return (existingWrapper!, existingProxy); + } + + // Register the RCW and publish the entry as a single step. The entry only names the RCW, and + // whoever finds it resolves the wrapper through the table, so no thread may ever be able to + // observe one of the two without the other. Reserving the entry above is what makes that + // possible: growing the dictionary is the only part of publishing that can fail on its own, + // and it has already happened by the time the registration is made. All that remains is to + // undo the reservation if the registration doesn't go through. + NativeObjectWrapper registeredWrapper; + + try { - // Someone else didn't beat us to adding the entry to the cache. - // Add our entry here. - rcwEntry = new WeakGCHandle(wrapper); + registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(comProxy, wrapper); } - else if (!rcwEntry.TryGetTarget(out NativeObjectWrapper? cachedWrapper)) + catch { - Debug.Assert(rcwEntry.IsAllocated); - // The target was collected, so we need to update the cache entry. - rcwEntry.SetTarget(wrapper); + if (!exists) + { + _cache.Remove(comPointer); + } + + throw; } - else + + if (registeredWrapper != wrapper) { - // The target NativeObjectWrapper was not collected, but we need to make sure - // that the proxy object is still alive. - if (cachedWrapper.ProxyHandle.TryGetTarget(out object? existingProxy)) + // This RCW is already the proxy for another COM instance, so it cannot be published. The + // caller reports that once it is out of this lock, as rejecting it releases the wrapper, + // which would take this lock again to remove an entry that was never added. + if (!exists) { - // The existing proxy object is still alive, we will use that. - return (cachedWrapper, existingProxy); + _cache.Remove(comPointer); } - // The proxy object was collected, so we need to update the cache entry. - rcwEntry.SetTarget(wrapper); + return (registeredWrapper, null); } - // We either added an entry to the cache or updated an existing entry that was dead. - // Return our target object. + // There was either no entry, or one whose RCW has been collected, so ours takes its place. + // The handle that gets overwritten is not freed here: every handle in this cache belongs to + // the wrapper that created it, which is what frees it, from 'NativeObjectWrapper.Release'. + rcwEntry = wrapper.ProxyHandle; + return (wrapper, comProxy); } finally @@ -1499,13 +1539,12 @@ public Bucket() _lock.EnterReadLock(); try { - if (!_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle)) + if (!_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle)) { // No entry in the cache. return null; } - if (existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper) - && cachedWrapper.ProxyHandle.TryGetTarget(out object? cachedProxy)) + if (existingHandle.TryGetTarget(out object? cachedProxy)) { // The target exists and is still alive. Return it. return cachedProxy; @@ -1525,13 +1564,12 @@ public Bucket() { // Someone else could have removed the entry or added a new one in the time // between us releasing the read lock and acquiring the write lock. - if (_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle) + if (_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle) && !existingHandle.TryGetTarget(out _)) { - // There's still a dead entry in the cache, - // remove it. + // There's still a dead entry in the cache, remove it. Only the entry is dropped, as + // the handle belongs to the wrapper that created it and is freed along with it. _cache.Remove(comPointer); - existingHandle.Dispose(); } } finally @@ -1549,16 +1587,14 @@ public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) try { // 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. - // 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 WeakGCHandle cachedRef) - && (!cachedRef.TryGetTarget(out NativeObjectWrapper? cachedWrapper) - || cachedWrapper == wrapper)) + // in the time between the GC clearing the contents of the GC handle and the + // NativeObjectWrapper finalizer running. Only the entry this wrapper published may be + // removed, and comparing the handles rather than their targets identifies it exactly, + // including once the RCW it refers to has been collected. + if (_cache.TryGetValue(comPointer, out WeakGCHandle cachedHandle) + && cachedHandle.Equals(wrapper.ProxyHandle)) { _cache.Remove(comPointer); - cachedRef.Dispose(); } } finally From 437c5cdcce9662ee7f6d3df6b1348046f2328d49 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 1 Sep 2026 06:26:55 -0700 Subject: [PATCH 2/7] Add tests for the RCW cache entry lifetime Three gaps, all on paths the cache change touches: A rejected registration has to leave nothing behind for the COM instance it was rejected for, so that instance can still get a wrapper afterwards. A dead entry replaced by a later RCW belongs to a different wrapper than the one about to finalize, and that wrapper must remove only the entry it published. Publishing, finding and removing entries all have to hold up when several threads do them at once while collections and finalizers run underneath, which is what would surface an entry being read after its owner freed it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop/COM/ComWrappers/API/Program.cs | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 233a476c4d747f..fccaada9ef652b 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -5,6 +5,7 @@ namespace ComWrappersTests { using System; using System.Collections; + using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -538,6 +539,111 @@ public void ValidateCreateObjectCachingScenario() Assert.NotEqual(trackerObj1, trackerObj3); } + // The RCW cache shares the GC handle that the NativeObjectWrapper keeps to its RCW, rather than + // allocating one of its own. That handle is freed by the wrapper, so this hammers the paths that + // publish, read and remove those entries from several threads at once, while collections and + // finalizers run underneath, to catch a cache entry ever being read after its owner freed it. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateCreateObjectConcurrentCacheAccess() + { + Console.WriteLine($"Running {nameof(ValidateCreateObjectConcurrentCacheAccess)}..."); + + const int ThreadCount = 8; + const int IterationCount = 300; + const int InstanceCount = 4; + + var cw = new TestComWrappers(); + + IntPtr[] instances = new IntPtr[InstanceCount]; + IntPtr[] identities = new IntPtr[InstanceCount]; + + for (int i = 0; i < instances.Length; i++) + { + instances[i] = MockReferenceTrackerRuntime.CreateTrackerObject(); + + // ComWrappers keys the cache on the identity IUnknown, which is what a round trip back to + // native produces, and that is not necessarily the pointer the object was created as. + Assert.Equal(0, Marshal.QueryInterface(instances[i], IUnknownVtbl.IID_IUnknown, out identities[i])); + } + + using var start = new Barrier(ThreadCount + 1); + var failures = new ConcurrentQueue(); + var threads = new Thread[ThreadCount]; + + for (int t = 0; t < threads.Length; t++) + { + int index = t; + + threads[t] = new Thread(() => + { + start.SignalAndWait(); + + try + { + for (int i = 0; i < IterationCount; i++) + { + // Every thread walks the instances from a different offset, so the threads are + // spread over the buckets rather than all queueing on one of them. + int slot = (i + index) % instances.Length; + + var wrapper = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(instances[slot], CreateObjectFlags.None); + + Assert.NotNull(wrapper); + + // Asking again while this thread still holds the wrapper has to produce the same + // object, which is the guarantee the cache exists to provide. + var again = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(instances[slot], CreateObjectFlags.None); + + Assert.Same(wrapper, again); + + // Round trip it back to a native pointer, which reads the wrapper the cache entry + // resolves to, and has to name the instance this thread asked for. + Assert.True(ComWrappers.TryGetComInstance(wrapper, out IntPtr unknown)); + Assert.Equal(identities[slot], unknown); + Marshal.Release(unknown); + + if ((i % 16) == index % 16) + { + // Drop everything this thread is holding and collect, so that entries go dead + // and wrapper finalizers run while the other threads are still using the cache. + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } + } + catch (Exception e) + { + failures.Enqueue(e); + } + }) + { IsBackground = true, Name = $"ComWrappers cache {index}" }; + + threads[t].Start(); + } + + start.SignalAndWait(); + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock in the RCW cache."); + } + + Assert.Empty(failures); + + ForceGC(); + + foreach (IntPtr identity in identities) + { + Marshal.Release(identity); + } + + foreach (IntPtr instance in instances) + { + Marshal.Release(instance); + } + } + // Verify that if a GC nulls the contents of a weak GCHandle but has not yet // run finializers to remove that GCHandle from the cache, the state of the system is valid. [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] @@ -575,6 +681,53 @@ static void CreateObject(ComWrappers cw, IntPtr trackerObj) } } + // A dead cache entry is replaced in place by the next RCW created for the same COM instance, so for a + // while the entry under that key belongs to a different wrapper than the one that is about to finalize. + // The old wrapper must remove only the entry it published, which is why entries are identified by the + // handle itself rather than by what it points at, since by then both point at nothing. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateReplacedCacheEntrySurvivesOldWrapperCleanUp() + { + Console.WriteLine($"Running {nameof(ValidateReplacedCacheEntrySurvivesOldWrapperCleanUp)}..."); + + var cw = new TestComWrappers(); + + IntPtr trackerObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + CreateAndAbandonWrapper(cw, trackerObjRaw); + + // Collect without draining finalizers, so the entry goes dead while the wrapper that owns it has + // most likely not run its finalizer yet. + GC.Collect(); + + // This takes over the dead entry, replacing the handle stored under that key. + var replacement = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.None); + Assert.NotNull(replacement); + + // Now let the abandoned wrapper finalize and release, which removes its own cache entry. + ForceGC(); + + // The replacement is still alive, so it has to still be cached. + var lookup = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.None); + Assert.Same(replacement, lookup); + + // It also has to still resolve back to the COM instance it wraps. + Assert.True(ComWrappers.TryGetComInstance(replacement, out IntPtr unknown)); + Marshal.Release(unknown); + + GC.KeepAlive(replacement); + + Marshal.Release(trackerObjRaw); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CreateAndAbandonWrapper(ComWrappers cw, IntPtr instance) + { + var obj = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(instance, CreateObjectFlags.None); + Assert.NotNull(obj); + } + } + [MethodImpl(MethodImplOptions.NoInlining)] [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] [Fact] @@ -774,6 +927,18 @@ public void ValidatePrecreatedExternalWrapper() { cw.GetOrRegisterObjectForComInstance(trackerObjRaw2, CreateObjectFlags.None, nativeWrapper2); }); + + // The rejected registration must not leave anything behind for that COM instance, so creating a + // wrapper for it now has to work and has to produce a usable object. + var recovered = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(trackerObjRaw2, CreateObjectFlags.None); + Assert.NotNull(recovered); + Assert.NotEqual(nativeWrapper2, recovered); + + // Asking again returns the same wrapper, which confirms the entry that was just created is the live + // one rather than something left over from the rejected attempt. + var recoveredAgain = (ITrackerObjectWrapper)cw.GetOrCreateObjectForComInstance(trackerObjRaw2, CreateObjectFlags.None); + Assert.Equal(recovered, recoveredAgain); + Marshal.Release(trackerObjRaw2); // Validate passing null wrapper fails. From 8a56e97065d8e8e17e739f9c01b459423f44fe7b Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 1 Sep 2026 12:31:34 -0700 Subject: [PATCH 3/7] Add tests for the races that pick which RCW gets cached Three tests, covering paths that the existing suite left uncovered and that any change to how an RCW is registered has to keep working. Two of them pin down what happens when several threads miss the cache for one COM instance at the same time and all call CreateObject. They are held on a barrier so they are guaranteed to be racing rather than finding each other's entries. One covers an implementation that hands every caller the same object, which is what one with its own cache would do, and the other covers distinct objects, where the ones that lost have to be left exactly as they were: not registered, not throwing from TryGetComInstance or from a weak reference, and still usable as the wrapper for some other COM instance. The third covers an RCW with no finalizer. Every other test in this file uses a type that has one, and a wrapper allocates a second GC handle for those. Without a finalizer there is a single handle that tracks resurrection, and that is the handle the cache holds, so its entries go dead at a different point in a collection than the ones already covered. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop/COM/ComWrappers/API/Program.cs | 328 ++++++++++++++++++ 1 file changed, 328 insertions(+) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index fccaada9ef652b..2707fa4b8ebb59 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -644,6 +644,251 @@ public void ValidateCreateObjectConcurrentCacheAccess() } } + // Hands every caller the same object, and holds them all inside 'CreateObject' until they have + // all arrived, so that they are guaranteed to have missed the cache and to be racing to publish. + private sealed unsafe class SharedProxyComWrappers : ComWrappers + { + private readonly Barrier _barrier; + + public SharedProxyComWrappers(Barrier barrier) => _barrier = barrier; + + public object Proxy { get; } = new(); + + /// How many callers reached , so a test can prove they all raced. + public int CreateObjectCount; + + protected override ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) + { + count = 0; + return null; + } + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) + { + Interlocked.Increment(ref CreateObjectCount); + + // Bounded rather than infinite so that a failure shows up as a failed assertion on the + // count below rather than as a hung test run. + _barrier.SignalAndWait(TimeSpan.FromMinutes(1)); + + return Proxy; + } + + protected override void ReleaseObjects(IEnumerable objects) => throw new NotImplementedException(); + } + + // Several threads can all miss the cache for one COM instance and all call 'CreateObject', and an + // implementation is free to hand each of them the same object. Only one of those threads publishes + // it and the rest release the wrapper they built, so this checks that losing that race leaves the + // object usable and still mapped to the COM instance it was created for. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateCreateObjectRaceReturningSameObject() + { + Console.WriteLine($"Running {nameof(ValidateCreateObjectRaceReturningSameObject)}..."); + + const int ThreadCount = 4; + + IntPtr trackerObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + // The cache is keyed on the identity IUnknown, which is what this round trip produces. + Assert.Equal(0, Marshal.QueryInterface(trackerObjRaw, IUnknownVtbl.IID_IUnknown, out IntPtr identity)); + + using var barrier = new Barrier(ThreadCount); + + var cw = new SharedProxyComWrappers(barrier); + var failures = new ConcurrentQueue(); + + object[] results = new object[ThreadCount]; + var threads = new Thread[ThreadCount]; + + for (int t = 0; t < threads.Length; t++) + { + int index = t; + + threads[t] = new Thread(() => + { + try + { + results[index] = cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.None); + } + catch (Exception e) + { + failures.Enqueue(e); + } + }) + { IsBackground = true, Name = $"ComWrappers shared proxy {index}" }; + + threads[t].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock while racing to publish."); + } + + Assert.Empty(failures); + + // Every thread really did miss the cache and go through 'CreateObject', so they all raced to + // publish rather than most of them quietly finding an entry someone else had already added. + Assert.Equal(ThreadCount, cw.CreateObjectCount); + + // Winning or losing the race, every thread is handed the one object 'CreateObject' returned. + foreach (object result in results) + { + Assert.Same(cw.Proxy, result); + } + + // Losing the race releases a wrapper, and it has to be the loser's rather than the published + // one, so the object still has to round trip back to the instance it was created for. + Assert.True(ComWrappers.TryGetComInstance(cw.Proxy, out IntPtr unknown)); + Assert.Equal(identity, unknown); + Marshal.Release(unknown); + + // And the cache still has to hand out that same object afterwards. + Assert.Same(cw.Proxy, cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.None)); + + Marshal.Release(identity); + Marshal.Release(trackerObjRaw); + } + + // Hands every caller a distinct object and keeps all of them, so a test can inspect the ones that + // lost the race to publish. Callers are held on a barrier so they are all guaranteed to have missed + // the cache and to be racing. + private sealed unsafe class DistinctProxyComWrappers : ComWrappers + { + private readonly Barrier _barrier; + + public DistinctProxyComWrappers(Barrier barrier) => _barrier = barrier; + + public ConcurrentQueue Created { get; } = new(); + + protected override ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) + { + count = 0; + return null; + } + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) + { + _barrier.SignalAndWait(TimeSpan.FromMinutes(1)); + + object proxy = new(); + + Created.Enqueue(proxy); + + return proxy; + } + + protected override void ReleaseObjects(IEnumerable objects) => throw new NotImplementedException(); + } + + // Only one of the objects 'CreateObject' produces for a COM instance can be published, and the + // wrappers built for the others are released. Nothing may be left behind for those objects: an + // implementation is free to keep hold of everything it returned, and the ones that lost have to + // behave exactly like objects that were never handed to ComWrappers at all. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateCreateObjectRaceLeavesNothingBehindForLosers() + { + Console.WriteLine($"Running {nameof(ValidateCreateObjectRaceLeavesNothingBehindForLosers)}..."); + + const int ThreadCount = 4; + + IntPtr trackerObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + Assert.Equal(0, Marshal.QueryInterface(trackerObjRaw, IUnknownVtbl.IID_IUnknown, out IntPtr identity)); + + using var barrier = new Barrier(ThreadCount); + + var cw = new DistinctProxyComWrappers(barrier); + var failures = new ConcurrentQueue(); + + object[] results = new object[ThreadCount]; + var threads = new Thread[ThreadCount]; + + for (int t = 0; t < threads.Length; t++) + { + int index = t; + + threads[t] = new Thread(() => + { + try + { + results[index] = cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.None); + } + catch (Exception e) + { + failures.Enqueue(e); + } + }) + { IsBackground = true, Name = $"ComWrappers distinct proxy {index}" }; + + threads[t].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock while racing to publish."); + } + + Assert.Empty(failures); + + Assert.Equal(ThreadCount, cw.Created.Count); + + object winner = results[0]; + + // Everyone gets the one object that was published, whichever thread produced it. + foreach (object result in results) + { + Assert.Same(winner, result); + } + + Assert.True(ComWrappers.TryGetComInstance(winner, out IntPtr winnerUnknown)); + Assert.Equal(identity, winnerUnknown); + Marshal.Release(winnerUnknown); + + int winners = 0; + + foreach (object created in cw.Created) + { + if (ReferenceEquals(created, winner)) + { + winners++; + continue; + } + + // A loser must look like an ordinary managed object. If a wrapper were left registered for + // it, these would throw instead, because releasing a wrapper zeroes the COM pointer that + // both of these paths hand to 'Marshal.QueryInterface'. + Assert.False(ComWrappers.TryGetComInstance(created, out IntPtr loserUnknown)); + Assert.Equal(IntPtr.Zero, loserUnknown); + + _ = new WeakReference(created); + } + + Assert.Equal(1, winners); + + // And a loser must still be usable as the wrapper for some other COM instance, rather than + // being permanently associated with the one it lost the race for. + IntPtr otherObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + foreach (object created in cw.Created) + { + if (ReferenceEquals(created, winner)) + { + continue; + } + + Assert.Same(created, cw.GetOrRegisterObjectForComInstance(otherObjRaw, CreateObjectFlags.None, created)); + break; + } + + Marshal.Release(otherObjRaw); + Marshal.Release(identity); + Marshal.Release(trackerObjRaw); + } + // Verify that if a GC nulls the contents of a weak GCHandle but has not yet // run finializers to remove that GCHandle from the cache, the state of the system is valid. [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] @@ -1000,6 +1245,89 @@ static WeakReference CreateAndRegisterWrapper(ComWrappers } } + // Every other test in this file uses an RCW type that has a finalizer, and a wrapper allocates a + // second GC handle for those. An RCW with no finalizer gets a single handle that tracks + // resurrection instead, and that is the handle the cache holds, so its entries go dead at a + // different point in a collection than the ones covered above. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateExternalWrapperCacheCleanUpWithoutFinalizer() + { + Console.WriteLine($"Running {nameof(ValidateExternalWrapperCacheCleanUpWithoutFinalizer)}..."); + + var cw = new TestComWrappers() + { + UseManualReleaseITestObjectWrapper = true, + }; + + var test = new Test(); + + IntPtr comWrapper = cw.GetOrCreateComInterfaceForObject(test, CreateComInterfaceFlags.None); + + Assert.NotEqual(IntPtr.Zero, comWrapper); + + WeakReference first = CreateAndAbandonWrapper(cw, comWrapper); + + // Collect without draining finalizers, so the entry is dead but the wrapper that owns it has + // not run yet and so has not removed it. Creating again has to see through the dead entry. + GC.Collect(); + + // This RCW type has no finalizer, so a single collection is enough for it to be gone. That is + // the property this test exists for: such an RCW gets one handle that tracks resurrection, and + // the cache holds that handle, so the entry dies here rather than after finalization. + Assert.False(first.TryGetTarget(out _)); + + WeakReference second = CreateAndAbandonWrapper(cw, comWrapper); + + // The dead entry did not prevent a new RCW being created and cached for the same instance. + // Checked in a separate frame so that the strong reference it needs does not outlive it and + // keep the RCW alive through the collection below. + AssertUsableAndDrop(second); + + // Now let the wrapper finalizers run, which is what removes entries, and check the cache is + // still able to hand out a working RCW afterwards. + ForceGC(); + + Assert.False(second.TryGetTarget(out _)); + + var third = (ManualReleaseITestObjectWrapper)cw.GetOrCreateObjectForComInstance(comWrapper, CreateObjectFlags.None); + + Assert.True(ComWrappers.TryGetComInstance(third, out IntPtr unknown)); + + Assert.Equal(0, Marshal.QueryInterface(comWrapper, IUnknownVtbl.IID_IUnknown, out IntPtr identity)); + Assert.Equal(identity, unknown); + + Marshal.Release(identity); + Marshal.Release(unknown); + + third.FinalRelease(); + + GC.KeepAlive(test); + + Marshal.Release(comWrapper); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void AssertUsableAndDrop(WeakReference reference) + { + Assert.True(reference.TryGetTarget(out object? target)); + Assert.True(ComWrappers.TryGetComInstance(target, out IntPtr unknown)); + + Marshal.Release(unknown); + } + + // This wrapper type releases the interface pointer its constructor took by hand rather than + // from a finalizer, so it is released here before the wrapper is dropped. + [MethodImpl(MethodImplOptions.NoInlining)] + static WeakReference CreateAndAbandonWrapper(ComWrappers cw, IntPtr comWrapper) + { + var wrapper = (ManualReleaseITestObjectWrapper)cw.GetOrCreateObjectForComInstance(comWrapper, CreateObjectFlags.None); + + wrapper.FinalRelease(); + + return new WeakReference(wrapper); + } + } + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] [Fact] public void ValidateSuppliedInnerNotAggregation() From ed3a15c7da0fd160a851fb909be4fe8c5a57ea7c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 1 Sep 2026 13:47:36 -0700 Subject: [PATCH 4/7] Keep the wrapper table registration out of the RCW cache locks Resolving a cache entry back to the wrapper tracking its RCW goes through the wrapper table, so the previous commit registered the RCW there while holding the bucket write lock. That table is a ConditionalWeakTable, and every registration is a new key, so every one of them takes the single lock covering the whole table. Holding a bucket lock across that put every bucket behind one process wide lock, which is exactly what partitioning the cache into buckets was meant to avoid. Creating RCWs on 32 threads went from 1003ns to 1276ns because of it. An entry is now reserved under the bucket lock and only carries the wrapper that reserved it until that wrapper has registered its RCW, which happens with no lock held. Threads that come across the entry during that window take the wrapper from it, and everything after the window resolves through the table as before. Reserving the slot up front keeps the property that the only part of publishing that can fail on its own, growing the dictionary, has already happened by the time it matters. Completing a reservation only overwrites one reference field of one entry and never changes the dictionary's layout, so it takes the read lock rather than the write lock and concurrent creations don't serialize on it. Lookups now read entries in place instead of copying them out, which more than pays for the entry having grown. Creating RCWs is 6 to 22% faster than before this series on 8 to 32 threads, and unchanged on one thread. Looking one up is 7 to 9% faster. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Runtime/InteropServices/ComWrappers.cs | 234 +++++++++++------- 1 file changed, 150 insertions(+), 84 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs index 595b36499c2067..200d864724baac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs @@ -641,6 +641,7 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper /// outlive every entry referring to it. /// internal WeakGCHandle ProxyHandle => _proxyHandle; + internal bool IsUniqueInstance => _uniqueInstance; internal bool IsAggregatedWithManagedObjectWrapper => _aggregatedManagedObjectWrapper; @@ -1261,43 +1262,43 @@ private object RegisterObjectForComInstance( flags, ref referenceTrackerMaybe); - NativeObjectWrapper actualWrapper; - object? actualProxy; + NativeObjectWrapper actualWrapper = nativeObjectWrapper; + object actualProxy = comProxy; + bool reserved = false; - if (nativeObjectWrapper.IsUniqueInstance) - { - // A unique instance is never published in the RCW cache, so its registration has nothing to be - // atomic with, and there is no cache entry for another thread to win. - actualWrapper = s_nativeObjectWrapperTable.GetOrAdd(comProxy, nativeObjectWrapper); - actualProxy = actualWrapper == nativeObjectWrapper ? comProxy : null; - } - else + if (!nativeObjectWrapper.IsUniqueInstance) { - // Add our entry to the cache here, using an already existing entry if someone else beat us to it. - // The RCW is registered in the wrapper table in there as well, so that no thread can ever find the - // entry before the registration it needs to resolve that entry back to a wrapper. - (actualWrapper, actualProxy) = _rcwCache.GetOrAddProxyForComInstance(identity, nativeObjectWrapper, comProxy); + // Reserve the entry for this COM instance, or pick up the one another thread got in first + // with. The entry stays marked as pending until the registration below has run, because + // that is what lets an entry be resolved back to the wrapper tracking its RCW. + (actualWrapper, actualProxy, reserved) = _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(); + } } - if (actualProxy is null) + // Register the RCW so it can be resolved back to the wrapper tracking it. Every thread that gets + // here for the same COM instance does this with the same pair, so whichever arrives first wins and + // the rest are no-ops. This is deliberately not done while holding a cache lock: it takes a lock + // covering the whole table, and holding a bucket lock across that would put every bucket behind it. + NativeObjectWrapper registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(actualProxy, actualWrapper); + + if (registeredWrapper != actualWrapper) { - // The object 'CreateObject' handed back is already the RCW for another COM instance, which is not - // something ComWrappers can represent. This is reported out here rather than where it is detected, - // because releasing the wrapper takes the same cache lock the registration above runs under. - Debug.Assert(actualWrapper.ExternalComObject != nativeObjectWrapper.ExternalComObject); + // The object 'CreateObject' handed back is already the RCW for another COM instance, which is + // not something ComWrappers can represent. Releasing the wrapper also drops the entry reserved + // for it above, if this thread is the one that reserved it. + Debug.Assert(registeredWrapper.ExternalComObject != actualWrapper.ExternalComObject); - nativeObjectWrapper.Release(); + actualWrapper.Release(); throw new NotSupportedException(); } - 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 // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. // @@ -1308,6 +1309,12 @@ private object RegisterObjectForComInstance( // This cache deduplicates, so it is okay that the wrapper will be registered multiple times. AddWrapperToReferenceTrackerHandleCache(actualWrapper); + if (reserved) + { + // The RCW resolves back to this wrapper now, so the entry no longer has to carry it. + _rcwCache.CommitProxyForComInstance(identity, nativeObjectWrapper); + } + return actualProxy; } @@ -1331,8 +1338,10 @@ internal void RemoveWrappersFromCache(IEnumerable wrappers) /// /// An entry names the RCW rather than the tracking it, and does so through /// a copy of the handle that wrapper already keeps to it, so that a cached RCW costs no handle of its own. - /// The wrapper for a cached RCW is resolved through , which is why - /// an RCW is put in there before its entry is published rather than after. + /// The wrapper for a cached RCW is resolved through , so an entry is + /// reserved before its RCW is registered there and only becomes usable once it has been. Registering is + /// deliberately left outside the bucket lock: it takes a lock covering that whole table, and holding a + /// bucket lock across it would serialize every bucket behind a single process wide lock. /// /// /// The cache is partitioned into several independent buckets, each with its own lock, so that operations on @@ -1392,20 +1401,33 @@ private unsafe ref readonly Bucket GetBucket(IntPtr comPointer) } /// - /// Gets the current RCW proxy object for if it exists in the cache or inserts a new entry with . + /// Reserves the entry for for , or gets the RCW another thread has already put there. /// /// 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. - /// The proxy object is if is already registered with another wrapper, in which case that wrapper is returned. + /// The proxy object currently in the cache for and the wrapper tracking it, or + /// and if this call is the one that reserved the entry. + /// The last item says which of the two happened, and is when the entry was reserved here, + /// in which case the caller must complete it with or release + /// . /// - public (NativeObjectWrapper actualWrapper, object? actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) + public (NativeObjectWrapper actualWrapper, object actualProxy, bool reserved) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { return GetBucket(comPointer).GetOrAddProxyForComInstance(comPointer, wrapper, comProxy); } + /// + /// Marks the entry reserved for as usable, once its RCW has been registered. + /// + /// The com instance to complete the entry for. + /// The that reserved the entry. + public void CommitProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper) + { + GetBucket(comPointer).CommitProxyForComInstance(comPointer, wrapper); + } + /// /// Gets the current RCW proxy object for , if it exists in the cache and is still alive. /// @@ -1455,7 +1477,7 @@ public void RemoveAll(IEnumerable wrappers) private readonly struct Bucket { private readonly ReaderWriterLockSlim _lock; - private readonly Dictionary> _cache; + private readonly Dictionary _cache; public Bucket() { @@ -1463,69 +1485,64 @@ public Bucket() _cache = []; } + /// + /// An entry in a bucket: the RCW cached for a COM instance, plus the wrapper publishing it for as + /// long as that publication is still in flight. + /// + private struct Entry + { + /// + /// A copy of the handle its wrapper keeps to the RCW. That wrapper owns it and is what frees it. + /// + public WeakGCHandle ProxyHandle; + + /// + /// The wrapper that reserved this entry, until it has registered its RCW in the wrapper table, + /// and from then on. It is carried here for the threads that come across + /// the entry during that window, as they cannot resolve it through the table yet. It has to be + /// dropped afterwards, because the cache may not be what keeps a wrapper alive: one that + /// outlived its RCW would never be finalized, and its finalizer is what removes this entry. + /// + public NativeObjectWrapper? PendingWrapper; + } + /// - public (NativeObjectWrapper actualWrapper, object? actualProxy) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) + public (NativeObjectWrapper actualWrapper, object actualProxy, bool reserved) GetOrAddProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper, object comProxy) { _lock.EnterWriteLock(); try { Debug.Assert(wrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy); - ref WeakGCHandle rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); + ref Entry rcwEntry = ref CollectionsMarshal.GetValueRefOrAddDefault(_cache, comPointer, out bool exists); - if (exists && rcwEntry.TryGetTarget(out object? existingProxy)) + if (exists && rcwEntry.ProxyHandle.TryGetTarget(out object? existingProxy)) { - // Someone else beat us to adding the entry and their RCW is still alive, so that is the - // one to use. Its wrapper was put in the table under this same lock before the entry was - // published, so it is guaranteed to be visible here. - bool found = s_nativeObjectWrapperTable.TryGetValue(existingProxy, out NativeObjectWrapper? existingWrapper); - - Debug.Assert(found); - - return (existingWrapper!, existingProxy); - } - - // Register the RCW and publish the entry as a single step. The entry only names the RCW, and - // whoever finds it resolves the wrapper through the table, so no thread may ever be able to - // observe one of the two without the other. Reserving the entry above is what makes that - // possible: growing the dictionary is the only part of publishing that can fail on its own, - // and it has already happened by the time the registration is made. All that remains is to - // undo the reservation if the registration doesn't go through. - NativeObjectWrapper registeredWrapper; + // Someone else beat us to the entry and their RCW is still alive, so that is the one to + // use. While they are still registering it, their wrapper is on the entry, because it + // cannot be reached through the table yet. Reading that table here is fine even though + // this lock is held: only mutating it takes its own lock, lookups are free of it. + NativeObjectWrapper? existingWrapper = rcwEntry.PendingWrapper; - try - { - registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(comProxy, wrapper); - } - catch - { - if (!exists) + if (existingWrapper is null) { - _cache.Remove(comPointer); - } + bool found = s_nativeObjectWrapperTable.TryGetValue(existingProxy, out existingWrapper); - throw; - } - - if (registeredWrapper != wrapper) - { - // This RCW is already the proxy for another COM instance, so it cannot be published. The - // caller reports that once it is out of this lock, as rejecting it releases the wrapper, - // which would take this lock again to remove an entry that was never added. - if (!exists) - { - _cache.Remove(comPointer); + Debug.Assert(found); } - return (registeredWrapper, null); + return (existingWrapper!, existingProxy, false); } // There was either no entry, or one whose RCW has been collected, so ours takes its place. // The handle that gets overwritten is not freed here: every handle in this cache belongs to // the wrapper that created it, which is what frees it, from 'NativeObjectWrapper.Release'. - rcwEntry = wrapper.ProxyHandle; + // Reserving the slot now rather than once the registration has run means the only part of + // publishing that can fail on its own, growing the dictionary, has already happened by then. + rcwEntry.ProxyHandle = wrapper.ProxyHandle; + rcwEntry.PendingWrapper = wrapper; - return (wrapper, comProxy); + return (wrapper, comProxy, true); } finally { @@ -1533,18 +1550,61 @@ public Bucket() } } + /// + public void CommitProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wrapper) + { + // Completing a reservation only overwrites one reference field of one entry. It never adds or + // removes anything, so the dictionary's layout doesn't change and this doesn't have to exclude + // the lookups running alongside it, only the writers that could move the entry out from under + // it. A lookup racing this either reads the wrapper, which is the right one anyway, or reads + // 'null' and resolves the same wrapper through the table, which by now certainly has it. Taking + // the read lock rather than the write lock is what keeps concurrent creations from serializing + // here after they have just been let through the write lock above. + _lock.EnterReadLock(); + try + { + ref Entry rcwEntry = ref CollectionsMarshal.GetValueRefOrNullRef(_cache, comPointer); + + // Only ever clear the reservation this wrapper made. Its RCW may have been collected while + // the registration ran, letting another wrapper take the entry over, and that one is still + // pending on its own registration. + if (!Unsafe.IsNullRef(ref rcwEntry) && ReferenceEquals(rcwEntry.PendingWrapper, wrapper)) + { + rcwEntry.PendingWrapper = null; + } + } + finally + { + _lock.ExitReadLock(); + } + } + /// public object? FindProxyForComInstance(IntPtr comPointer) { _lock.EnterReadLock(); try { - if (!_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle)) + // Read the entry in place rather than copying it out. It carries a reference as well as + // the handle now, and this runs on essentially every transition from native code, so the + // copy is worth avoiding. Holding the read lock is what makes the reference safe to use: + // it keeps out the writers that could move the entry. + ref Entry existingEntry = ref CollectionsMarshal.GetValueRefOrNullRef(_cache, comPointer); + + if (Unsafe.IsNullRef(ref existingEntry)) { // No entry in the cache. return null; } - if (existingHandle.TryGetTarget(out object? cachedProxy)) + if (existingEntry.PendingWrapper is not null) + { + // The entry is reserved but its RCW is not registered yet, so it cannot be handed out: + // the thread that reserved it may still fail and release the wrapper backing it. + // Reporting it as absent sends the caller down the creation path, which resolves the + // entry under the write lock, where the wrapper that reserved it is reachable. + return null; + } + if (existingEntry.ProxyHandle.TryGetTarget(out object? cachedProxy)) { // The target exists and is still alive. Return it. return cachedProxy; @@ -1564,8 +1624,11 @@ public Bucket() { // Someone else could have removed the entry or added a new one in the time // between us releasing the read lock and acquiring the write lock. - if (_cache.TryGetValue(comPointer, out WeakGCHandle existingHandle) - && !existingHandle.TryGetTarget(out _)) + ref Entry existingEntry = ref CollectionsMarshal.GetValueRefOrNullRef(_cache, comPointer); + + if (!Unsafe.IsNullRef(ref existingEntry) + && existingEntry.PendingWrapper is null + && !existingEntry.ProxyHandle.TryGetTarget(out _)) { // There's still a dead entry in the cache, remove it. Only the entry is dropped, as // the handle belongs to the wrapper that created it and is freed along with it. @@ -1590,9 +1653,12 @@ public void Remove(IntPtr comPointer, NativeObjectWrapper wrapper) // in the time between the GC clearing the contents of the GC handle and the // NativeObjectWrapper finalizer running. Only the entry this wrapper published may be // removed, and comparing the handles rather than their targets identifies it exactly, - // including once the RCW it refers to has been collected. - if (_cache.TryGetValue(comPointer, out WeakGCHandle cachedHandle) - && cachedHandle.Equals(wrapper.ProxyHandle)) + // including once the RCW it refers to has been collected. That covers an entry this + // wrapper only reserved as well, as a reservation already carries its handle. + ref Entry cachedEntry = ref CollectionsMarshal.GetValueRefOrNullRef(_cache, comPointer); + + if (!Unsafe.IsNullRef(ref cachedEntry) + && cachedEntry.ProxyHandle.Equals(wrapper.ProxyHandle)) { _cache.Remove(comPointer); } From ff40398a50f534f84ddb89ae94e6445f6fb99a32 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 1 Sep 2026 16:13:30 -0700 Subject: [PATCH 5/7] Drop an RCW cache reservation even when publishing it fails A reservation carries a strong reference to the wrapper that made it, so leaving one behind keeps that wrapper alive, and a wrapper that outlives its RCW can never be finalized, which is what removes the entry. Anything that threw between reserving an entry and completing it therefore stranded the reservation: the COM instance kept its native reference forever and every later lookup for it reported a miss. Registering the RCW can throw, so can registering it with the reference tracker, and both run in that window. The reservation is now dropped whether publishing went through or not, which is right either way. If another thread picked the wrapper up out of the entry and registered it, the entry is usable and should be usable. If nobody did, nothing refers to the wrapper any more, so it is finalized and takes the entry with it, which is how this recovered before the cache started sharing handles. An entry naming an RCW that never made it into the wrapper table can be observed for as long as it takes that wrapper to be finalized, so resolving one no longer asserts that it must be there and replaces the entry instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Runtime/InteropServices/ComWrappers.cs | 73 +++++++++++-------- 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs index 200d864724baac..435dac24bc5964 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs @@ -1285,34 +1285,43 @@ private object RegisterObjectForComInstance( // here for the same COM instance does this with the same pair, so whichever arrives first wins and // the rest are no-ops. This is deliberately not done while holding a cache lock: it takes a lock // covering the whole table, and holding a bucket lock across that would put every bucket behind it. - NativeObjectWrapper registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(actualProxy, actualWrapper); - - if (registeredWrapper != actualWrapper) + try { - // The object 'CreateObject' handed back is already the RCW for another COM instance, which is - // not something ComWrappers can represent. Releasing the wrapper also drops the entry reserved - // for it above, if this thread is the one that reserved it. - Debug.Assert(registeredWrapper.ExternalComObject != actualWrapper.ExternalComObject); + NativeObjectWrapper registeredWrapper = s_nativeObjectWrapperTable.GetOrAdd(actualProxy, actualWrapper); - actualWrapper.Release(); + if (registeredWrapper != actualWrapper) + { + // The object 'CreateObject' handed back is already the RCW for another COM instance, which is + // not something ComWrappers can represent. Releasing the wrapper also drops the entry reserved + // for it above, if this thread is the one that reserved it. + Debug.Assert(registeredWrapper.ExternalComObject != actualWrapper.ExternalComObject); - throw new NotSupportedException(); - } + actualWrapper.Release(); - // At this point, actualProxy is the RCW object for the identity - // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. - // - // 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 - // 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(actualWrapper); + throw new NotSupportedException(); + } - if (reserved) + // At this point, actualProxy is the RCW object for the identity + // and actualWrapper is the NativeObjectWrapper that is in the RCW cache (if not unique) that associates the identity with actualProxy. + // + // 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 + // 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(actualWrapper); + } + finally { - // The RCW resolves back to this wrapper now, so the entry no longer has to carry it. - _rcwCache.CommitProxyForComInstance(identity, nativeObjectWrapper); + if (reserved) + { + // The reservation is dropped whether the registration went through or not, because leaving + // one behind would keep its wrapper alive, and a wrapper that outlives its RCW can never be + // finalized, which is what removes the entry. Dropping it is right either way: if some other + // thread picked the wrapper up and registered it, the entry is now usable, and if nobody did, + // nothing is left referring to the wrapper, so it is finalized and takes the entry with it. + _rcwCache.CommitProxyForComInstance(identity, nativeObjectWrapper); + } } return actualProxy; @@ -1526,12 +1535,17 @@ private struct Entry if (existingWrapper is null) { - bool found = s_nativeObjectWrapperTable.TryGetValue(existingProxy, out existingWrapper); + _ = s_nativeObjectWrapperTable.TryGetValue(existingProxy, out existingWrapper); + } - Debug.Assert(found); + if (existingWrapper is not null) + { + return (existingWrapper, existingProxy, false); } - return (existingWrapper!, existingProxy, false); + // The entry names an RCW that never made it into the table, which is what a thread + // that failed part way through publishing leaves behind. It can't be resolved back to + // a wrapper, so it is of no use to anyone and ours replaces it below. } // There was either no entry, or one whose RCW has been collected, so ours takes its place. @@ -1556,10 +1570,11 @@ public void CommitProxyForComInstance(IntPtr comPointer, NativeObjectWrapper wra // Completing a reservation only overwrites one reference field of one entry. It never adds or // removes anything, so the dictionary's layout doesn't change and this doesn't have to exclude // the lookups running alongside it, only the writers that could move the entry out from under - // it. A lookup racing this either reads the wrapper, which is the right one anyway, or reads - // 'null' and resolves the same wrapper through the table, which by now certainly has it. Taking - // the read lock rather than the write lock is what keeps concurrent creations from serializing - // here after they have just been let through the write lock above. + // it. The lookup that can overlap this is 'FindProxyForComInstance', which either still sees + // the reservation and reports a miss, sending its caller down a path that takes the write lock + // and resolves the entry properly, or sees it gone and hands out an RCW that is by then + // registered. Taking the read lock rather than the write lock is what keeps concurrent + // creations from serializing here after they have just been let through the write lock above. _lock.EnterReadLock(); try { From 7d8bf75a08e8da7dd3c2bd1ab798f672e7708cb4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 2 Sep 2026 05:34:37 -0700 Subject: [PATCH 6/7] Add tests for resolving an RCW and for the tracker registration race An RCW is only resolvable back to its COM instance once its wrapper is in the wrapper table, and that registration cannot happen while a cache lock is held, so an entry is reserved rather than published outright and a reserved entry is not handed out. Nothing covered that. The nearest test reuses the same few COM instances, so after its first iteration there is no creation left to race and it only catches this by luck. The new one gives every round a fresh COM instance for all its threads to race over, which is the shape that hits it: it fails on main, where 'TryGetComInstance' comes up empty around 50 times in 8000, and passes here. The concurrent test that was already there uses CreateObjectFlags.None, so the wrapper it builds is not a reference tracker one and putting it in the tracker handle cache does nothing, leaving that registration untested under any concurrency at all. The second test runs the same race with tracker objects and then checks what the registration is for, that the wrapper is walked, by handing the native object a thousand managed objects and collecting. It hands every caller the one wrapper rather than one each, because ITrackerObjectWrapper's finalizer fails the run if its tracker object is still connected and the winner keeps it connected for as long as the test needs it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop/COM/ComWrappers/API/Program.cs | 228 ++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 2707fa4b8ebb59..0a192f96b152bc 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -539,6 +539,108 @@ public void ValidateCreateObjectCachingScenario() Assert.NotEqual(trackerObj1, trackerObj3); } + private sealed unsafe class PlainProxyComWrappers : ComWrappers + { + protected override ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count) + { + count = 0; + return null; + } + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) => new(); + + protected override void ReleaseObjects(IEnumerable objects) => throw new NotImplementedException(); + } + + // An RCW is only resolvable back to its COM instance once its wrapper is in the wrapper table, and + // that registration cannot be done while holding a cache lock. So an entry is reserved rather than + // published outright, and a reserved entry is not handed out, or a thread could be given an RCW + // that does not resolve yet. Every round here is a fresh COM instance that all the threads race to + // create the RCW for, which is the shape that hits it: without the reservation this fails in the + // low tens out of these several thousand attempts, and on a build with it, never. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateCreateObjectRaceResolvesImmediately() + { + Console.WriteLine($"Running {nameof(ValidateCreateObjectRaceResolvesImmediately)}..."); + + const int ThreadCount = 16; + const int RoundCount = 500; + + IntPtr[] instances = new IntPtr[RoundCount]; + + for (int i = 0; i < instances.Length; i++) + { + instances[i] = MockReferenceTrackerRuntime.CreateTrackerObject(); + } + + var cw = new PlainProxyComWrappers(); + var failures = new ConcurrentQueue(); + + // The RCWs are kept alive for the whole run, so that no wrapper is finalized underneath a + // round that is still being checked. + object[] proxies = new object[RoundCount]; + + using var barrier = new Barrier(ThreadCount); + + int unresolved = 0; + var threads = new Thread[ThreadCount]; + + for (int t = 0; t < threads.Length; t++) + { + threads[t] = new Thread(() => + { + for (int round = 0; round < RoundCount; round++) + { + try + { + // Line every thread up on the same instance, so they all miss the cache and + // race to publish it rather than finding each other's entries. + barrier.SignalAndWait(TimeSpan.FromMinutes(1)); + + object proxy = cw.GetOrCreateObjectForComInstance(instances[round], CreateObjectFlags.None); + + proxies[round] = proxy; + + // The RCW is in hand, so it has to name the COM instance it was created for. + if (ComWrappers.TryGetComInstance(proxy, out IntPtr unknown)) + { + Marshal.Release(unknown); + } + else + { + Interlocked.Increment(ref unresolved); + } + } + catch (Exception e) + { + // Carry on to the next round regardless, so that the other threads are not + // left waiting on a barrier this one has stopped arriving at. + failures.Enqueue(e); + } + } + }) + { IsBackground = true, Name = $"ComWrappers resolve {t}" }; + + threads[t].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock while racing to publish."); + } + + Assert.Empty(failures); + Assert.Equal(0, unresolved); + + GC.KeepAlive(proxies); + + foreach (IntPtr instance in instances) + { + Marshal.Release(instance); + } + } + // The RCW cache shares the GC handle that the NativeObjectWrapper keeps to its RCW, rather than // allocating one of its own. That handle is freed by the wrapper, so this hammers the paths that // publish, read and remove those entries from several threads at once, while collections and @@ -644,6 +746,132 @@ public void ValidateCreateObjectConcurrentCacheAccess() } } + // Hands every caller the one wrapper, so that only one native reference is taken however many + // threads race. Creating a wrapper per caller and abandoning the losers is not an option here: + // ITrackerObjectWrapper's finalizer fails the run if its tracker object is still connected, and + // the winner keeps it connected for as long as the test needs it. + private sealed class SharedTrackerComWrappers : TestComWrappers + { + private readonly Barrier _barrier; + private readonly object _lock = new(); + private object? _proxy; + + public SharedTrackerComWrappers(Barrier barrier) => _barrier = barrier; + + /// How many callers reached , so a test can prove they all raced. + public int CreateObjectCount; + + protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags) + { + Interlocked.Increment(ref CreateObjectCount); + + _barrier.SignalAndWait(TimeSpan.FromMinutes(1)); + + lock (_lock) + { + return _proxy ??= base.CreateObject(externalComObject, flags); + } + } + } + + // The concurrent test above uses CreateObjectFlags.None, so the wrapper it builds is not a + // reference tracker one and putting it in the tracker handle cache does nothing. This runs the + // same race with tracker objects, where one thread publishes the entry and the rest pick its + // wrapper up, and then checks what that registration is for: the wrapper has to be walked, or + // the managed objects the native object is holding are not kept alive through a collection. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateCreateObjectConcurrentTrackerRegistration() + { + Console.WriteLine($"Running {nameof(ValidateCreateObjectConcurrentTrackerRegistration)}..."); + + const int ThreadCount = 8; + + IntPtr trackerObjRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + using var barrier = new Barrier(ThreadCount); + + var cw = new SharedTrackerComWrappers(barrier); + var failures = new ConcurrentQueue(); + + object[] results = new object[ThreadCount]; + var threads = new Thread[ThreadCount]; + + for (int t = 0; t < threads.Length; t++) + { + int index = t; + + threads[t] = new Thread(() => + { + try + { + results[index] = cw.GetOrCreateObjectForComInstance(trackerObjRaw, CreateObjectFlags.TrackerObject); + } + catch (Exception e) + { + failures.Enqueue(e); + } + }) + { IsBackground = true, Name = $"ComWrappers tracker {index}" }; + + threads[t].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock while racing to publish."); + } + + Assert.Empty(failures); + + // Every thread really did miss the cache, so they all raced to publish rather than most of + // them quietly finding an entry someone else had already added. + Assert.Equal(ThreadCount, cw.CreateObjectCount); + + // Ownership has been transferred to the wrapper. + Marshal.Release(trackerObjRaw); + + var trackerObj = (ITrackerObjectWrapper)results[0]; + + foreach (object result in results) + { + Assert.Same(trackerObj, result); + } + + Assert.True(ComWrappers.TryGetComInstance(trackerObj, out IntPtr unknown)); + + Marshal.Release(unknown); + + // Whichever thread ended up publishing it, the wrapper has to have made it into the tracker + // handle cache, because that is what the runtime walks. If it had not, the managed objects + // reachable only through the native object would not be reported and would not survive here. + var testWrapperIds = new List(); + + for (int i = 0; i < 1000; ++i) + { + IntPtr testWrapper = cw.GetOrCreateComInterfaceForObject(new Test(), CreateComInterfaceFlags.TrackerSupport); + + testWrapperIds.Add(trackerObj.AddObjectRef(testWrapper)); + + Marshal.Release(testWrapper); + } + + ForceGC(); + + Assert.True(testWrapperIds.Count <= Test.InstanceCount); + + foreach (int id in testWrapperIds) + { + trackerObj.DropObjectRef(id); + } + + testWrapperIds.Clear(); + + ForceGC(); + + GC.KeepAlive(trackerObj); + } + // Hands every caller the same object, and holds them all inside 'CreateObject' until they have // all arrived, so that they are guaranteed to have missed the cache and to be racing to publish. private sealed unsafe class SharedProxyComWrappers : ComWrappers From 2dd3b384232e75e263b1749d47e108f9465994de Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 2 Sep 2026 06:04:40 -0700 Subject: [PATCH 7/7] Add tests for registering into a race and for rejecting over a dead entry Neither is a bug on main, and both pass there. They cover two shapes of the cache paths this series reworks that nothing exercised, so that a future change to how an entry is claimed cannot quietly break them. The first races callers that bring their own object to register against callers that ask for one to be created, over the same COM instance. Only one can win, every caller has to come back with it, and the objects that lost have to be left exactly as they were: not registered, and still usable as the wrapper for some other COM instance. The second rejects a registration for a COM instance whose entry is still there but whose RCW has been collected. The existing coverage rejects one for a COM instance the cache has never seen, which takes a different path, because an entry that is present has to be taken over rather than added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop/COM/ComWrappers/API/Program.cs | 154 ++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs index 0a192f96b152bc..89c79a95072810 100644 --- a/src/tests/Interop/COM/ComWrappers/API/Program.cs +++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs @@ -746,6 +746,160 @@ public void ValidateCreateObjectConcurrentCacheAccess() } } + // Registering a caller supplied object and creating one race through the same cache, and only one + // of them can win. Whichever does, every caller has to come back with it, and the objects that + // lost have to be left exactly as they were rather than half registered. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateRegisterAndCreateRaceForSameComInstance() + { + Console.WriteLine($"Running {nameof(ValidateRegisterAndCreateRaceForSameComInstance)}..."); + + const int ThreadCount = 8; + + IntPtr instanceRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + Assert.Equal(0, Marshal.QueryInterface(instanceRaw, IUnknownVtbl.IID_IUnknown, out IntPtr identity)); + + var cw = new PlainProxyComWrappers(); + var failures = new ConcurrentQueue(); + + object[] supplied = new object[ThreadCount]; + object[] results = new object[ThreadCount]; + var threads = new Thread[ThreadCount]; + + using var barrier = new Barrier(ThreadCount); + + for (int t = 0; t < threads.Length; t++) + { + int index = t; + + // Every other thread brings its own object to register, the rest ask for one to be created. + supplied[index] = (index % 2) == 0 ? new object() : null; + + threads[t] = new Thread(() => + { + try + { + barrier.SignalAndWait(TimeSpan.FromMinutes(1)); + + results[index] = supplied[index] is object toRegister + ? cw.GetOrRegisterObjectForComInstance(instanceRaw, CreateObjectFlags.None, toRegister) + : cw.GetOrCreateObjectForComInstance(instanceRaw, CreateObjectFlags.None); + } + catch (Exception e) + { + failures.Enqueue(e); + } + }) + { IsBackground = true, Name = $"ComWrappers register race {index}" }; + + threads[t].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromMinutes(2)), "A worker thread did not finish, which suggests a deadlock while racing to publish."); + } + + Assert.Empty(failures); + + object winner = results[0]; + + foreach (object result in results) + { + Assert.Same(winner, result); + } + + Assert.True(ComWrappers.TryGetComInstance(winner, out IntPtr winnerUnknown)); + Assert.Equal(identity, winnerUnknown); + Marshal.Release(winnerUnknown); + + // A supplied object that lost has to look like an object that was never handed over at all, + // and has to still be usable as the wrapper for some other COM instance. + IntPtr otherRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + bool reusedOne = false; + + foreach (object candidate in supplied) + { + if (candidate is null || ReferenceEquals(candidate, winner)) + { + continue; + } + + Assert.False(ComWrappers.TryGetComInstance(candidate, out IntPtr loserUnknown)); + Assert.Equal(IntPtr.Zero, loserUnknown); + + if (!reusedOne) + { + Assert.Same(candidate, cw.GetOrRegisterObjectForComInstance(otherRaw, CreateObjectFlags.None, candidate)); + reusedOne = true; + } + } + + Marshal.Release(otherRaw); + Marshal.Release(identity); + Marshal.Release(instanceRaw); + } + + // A registration is rejected when the object handed over is already the RCW for another COM + // instance. The existing coverage rejects one for a COM instance the cache has never seen; this + // one rejects it for a COM instance that still has an entry whose RCW has been collected, which + // is a different path through the cache because the entry is there and has to be taken over. + [ActiveIssue("Not supported on Mono", TestRuntimes.Mono)] + [Fact] + public void ValidateRejectedRegistrationOverDeadEntry() + { + Console.WriteLine($"Running {nameof(ValidateRejectedRegistrationOverDeadEntry)}..."); + + var cw = new PlainProxyComWrappers(); + + IntPtr firstRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + IntPtr secondRaw = MockReferenceTrackerRuntime.CreateTrackerObject(); + + // An RCW that belongs to the first COM instance, so registering it for another is rejected. + object owned = cw.GetOrCreateObjectForComInstance(firstRaw, CreateObjectFlags.None); + + // Leave a dead entry behind for the second instance: collect its RCW without draining + // finalizers, so the entry is still there but no longer names anything. + CreateAndAbandon(cw, secondRaw); + + GC.Collect(); + + Assert.Throws(() => cw.GetOrRegisterObjectForComInstance(secondRaw, CreateObjectFlags.None, owned)); + + // The rejection must not have disturbed the instance the object does belong to. + Assert.True(ComWrappers.TryGetComInstance(owned, out IntPtr ownedUnknown)); + + Assert.Equal(0, Marshal.QueryInterface(firstRaw, IUnknownVtbl.IID_IUnknown, out IntPtr firstIdentity)); + Assert.Equal(firstIdentity, ownedUnknown); + + Marshal.Release(firstIdentity); + Marshal.Release(ownedUnknown); + + // And the second instance has to be usable afterwards, rather than left holding whatever the + // rejected attempt put there. + object recovered = cw.GetOrCreateObjectForComInstance(secondRaw, CreateObjectFlags.None); + + Assert.NotSame(owned, recovered); + Assert.Same(recovered, cw.GetOrCreateObjectForComInstance(secondRaw, CreateObjectFlags.None)); + + Assert.True(ComWrappers.TryGetComInstance(recovered, out IntPtr recoveredUnknown)); + Marshal.Release(recoveredUnknown); + + GC.KeepAlive(owned); + GC.KeepAlive(recovered); + + Marshal.Release(secondRaw); + Marshal.Release(firstRaw); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void CreateAndAbandon(ComWrappers cw, IntPtr comInstance) + { + _ = cw.GetOrCreateObjectForComInstance(comInstance, CreateObjectFlags.None); + } + } + // Hands every caller the one wrapper, so that only one native reference is taken however many // threads race. Creating a wrapper per caller and abandoning the losers is not an option here: // ITrackerObjectWrapper's finalizer fails the run if its tracker object is still connected, and