Sitelet https://github.com/dotnet/runtime/commit/4b8b9ba8e4c7b0b1c8f29951a4fb1d3543a7e35b
Skip to content

Commit 4b8b9ba

Browse files
Sergio0694Sergio0694
authored
Add ComWrappers RCW cache concurrency tests, and fix an RCW being handed out before it can be resolved (#133164)
1 parent aa9e74e commit 4b8b9ba

2 files changed

Lines changed: 913 additions & 5 deletions

File tree

  • src
    • libraries/System.Private.CoreLib/src/System/Runtime/InteropServices
    • tests/Interop/COM/ComWrappers/API

‎src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,7 @@ internal unsafe class NativeObjectWrapper
552552
private WeakGCHandle<object> _proxyHandleTrackingResurrection;
553553
private readonly bool _aggregatedManagedObjectWrapper;
554554
private readonly bool _uniqueInstance;
555+
private volatile bool _registered;
555556

556557
static NativeObjectWrapper()
557558
{
@@ -638,6 +639,22 @@ protected NativeObjectWrapper(IntPtr externalComObject, IntPtr inner, ComWrapper
638639
internal bool IsUniqueInstance => _uniqueInstance;
639640
internal bool IsAggregatedWithManagedObjectWrapper => _aggregatedManagedObjectWrapper;
640641

642+
/// <summary>
643+
/// Whether the RCW this wrapper tracks can be resolved back to it yet.
644+
/// </summary>
645+
/// <remarks>
646+
/// A wrapper is put in the RCW cache before it is registered in <see cref="s_nativeObjectWrapperTable"/>,
647+
/// because that registration takes a lock covering every RCW in the process and the cache lock may not
648+
/// be held across it. Until it has run, the RCW is in the cache but nothing can get from it back to
649+
/// this wrapper, so it may not be handed out yet.
650+
/// </remarks>
651+
internal bool IsRegistered => _registered;
652+
653+
/// <summary>
654+
/// Marks the RCW this wrapper tracks as resolvable back to it.
655+
/// </summary>
656+
internal void MarkRegistered() => _registered = true;
657+
641658
public virtual void Release()
642659
{
643660
if (!_uniqueInstance && _comWrappers is not null)
@@ -1140,7 +1157,15 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal(
11401157

11411158
// If we have a live cached wrapper currently,
11421159
// return that.
1143-
if (_rcwCache.FindProxyForComInstance(identity) is object liveCachedWrapper)
1160+
//
1161+
// An entry goes into the cache before its wrapper is registered in 's_nativeObjectWrapperTable',
1162+
// because that registration takes a lock covering every RCW in the process and the cache lock may
1163+
// not be held across it. Handing out an entry in that window would give the caller an RCW that
1164+
// 'TryGetComInstance' cannot yet resolve, so it is skipped and treated as a miss instead. The
1165+
// creation path below then finds that same entry under the cache write lock and registers it
1166+
// before returning it, so this only costs one extra 'CreateObject' call in a rare race.
1167+
if (_rcwCache.FindProxyForComInstance(identity, out NativeObjectWrapper? cachedWrapper) is object liveCachedWrapper
1168+
&& cachedWrapper is { IsRegistered: true })
11441169
{
11451170
retValue = liveCachedWrapper;
11461171
return true;
@@ -1286,7 +1311,7 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro
12861311
// for both threads. In that case, it doesn't matter which thread adds the entry to the NativeObjectWrapper table
12871312
// as the entry is always the same pair.
12881313
Debug.Assert(wrapper.ProxyHandle.TryGetTarget(out object? proxyTarget) && proxyTarget == comProxy);
1289-
Debug.Assert(wrapper.IsUniqueInstance || _rcwCache.FindProxyForComInstance(wrapper.ExternalComObject) == comProxy);
1314+
Debug.Assert(wrapper.IsUniqueInstance || _rcwCache.FindProxyForComInstance(wrapper.ExternalComObject, out _) == comProxy);
12901315

12911316
// Add the input wrapper bound to the COM proxy, if there isn't one already. If another thread raced
12921317
// against this one and this lost, we'd get the wrapper added from that thread instead.
@@ -1306,6 +1331,9 @@ private void RegisterWrapperForObject(NativeObjectWrapper wrapper, object comPro
13061331
// TrackerObjectManager and we could end up missing a section of the object graph.
13071332
// This cache deduplicates, so it is okay that the wrapper will be registered multiple times.
13081333
AddWrapperToReferenceTrackerHandleCache(registeredWrapper);
1334+
1335+
// The RCW can now be resolved back to its wrapper, so it is safe for the cache to hand it out.
1336+
wrapper.MarkRegistered();
13091337
}
13101338

13111339
private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper)
@@ -1396,10 +1424,11 @@ private unsafe ref readonly Bucket GetBucket(IntPtr comPointer)
13961424
/// Gets the current RCW proxy object for <paramref name="comPointer"/>, if it exists in the cache and is still alive.
13971425
/// </summary>
13981426
/// <param name="comPointer">The com instance we want to get the RCW for.</param>
1427+
/// <param name="wrapper">The <see cref="NativeObjectWrapper"/> owning the returned proxy object, if any.</param>
13991428
/// <returns>The proxy object currently in the cache for <paramref name="comPointer"/>, if any.</returns>
1400-
public object? FindProxyForComInstance(IntPtr comPointer)
1429+
public object? FindProxyForComInstance(IntPtr comPointer, out NativeObjectWrapper? wrapper)
14011430
{
1402-
return GetBucket(comPointer).FindProxyForComInstance(comPointer);
1431+
return GetBucket(comPointer).FindProxyForComInstance(comPointer, out wrapper);
14031432
}
14041433

14051434
/// <summary>
@@ -1494,20 +1523,22 @@ public Bucket()
14941523
}
14951524

14961525
/// <inheritdoc cref="RcwCache.FindProxyForComInstance"/>
1497-
public object? FindProxyForComInstance(IntPtr comPointer)
1526+
public object? FindProxyForComInstance(IntPtr comPointer, out NativeObjectWrapper? wrapper)
14981527
{
14991528
_lock.EnterReadLock();
15001529
try
15011530
{
15021531
if (!_cache.TryGetValue(comPointer, out WeakGCHandle<NativeObjectWrapper> existingHandle))
15031532
{
15041533
// No entry in the cache.
1534+
wrapper = null;
15051535
return null;
15061536
}
15071537
if (existingHandle.TryGetTarget(out NativeObjectWrapper? cachedWrapper)
15081538
&& cachedWrapper.ProxyHandle.TryGetTarget(out object? cachedProxy))
15091539
{
15101540
// The target exists and is still alive. Return it.
1541+
wrapper = cachedWrapper;
15111542
return cachedProxy;
15121543
}
15131544
// The target was collected, so we need to remove the entry from the cache.
@@ -1539,6 +1570,7 @@ public Bucket()
15391570
_lock.ExitWriteLock();
15401571
}
15411572

1573+
wrapper = null;
15421574
return null;
15431575
}
15441576

0 commit comments

Comments
 (0)