From 3a04e5c6d9aee1963f40053ab5dd6c4cf3c7da7b Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Sat, 6 Jul 2024 19:14:00 -0700 Subject: [PATCH 1/7] Fix exception during GC callback in WinUI scenarios due to removing from and adding to the nativeObjectReference list around the same time and causing removal to fail. --- .../InteropServices/ComWrappers.NativeAot.cs | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 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 f8547cb408f8d2..3a6db27a4be036 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 @@ -42,6 +42,7 @@ public abstract partial class ComWrappers private static readonly ConditionalWeakTable s_rcwTable = new ConditionalWeakTable(); private static readonly List s_referenceTrackerNativeObjectWrapperCache = new List(); + private static readonly Lock s_nativeObjectWrapperLockCache = new Lock(useTrivialWaits: true); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); private readonly Lock _lock = new Lock(useTrivialWaits: true); @@ -637,7 +638,10 @@ public override void Release() // Remove the entry from the cache that keeps track of the active NativeObjectWrappers. if (_nativeObjectWrapperWeakHandle.IsAllocated) { - s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle); + using (s_nativeObjectWrapperLockCache.EnterScope()) + { + s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle); + } _nativeObjectWrapperWeakHandle.Free(); } @@ -986,7 +990,10 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( _rcwCache.Add(identity, wrapper._proxyHandle); if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + using (s_nativeObjectWrapperLockCache.EnterScope()) + { + s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + } } return true; } @@ -1042,7 +1049,10 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( } if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + using (s_nativeObjectWrapperLockCache.EnterScope()) + { + s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + } } return true; } @@ -1081,7 +1091,10 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( _rcwCache.Add(identity, wrapper._proxyHandle); if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + using (s_nativeObjectWrapperLockCache.EnterScope()) + { + s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + } } } } @@ -1220,17 +1233,21 @@ internal static void ReleaseExternalObjectsFromCurrentThread() IntPtr contextToken = GetContextToken(); List objects = new List(); - foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) + + using (s_nativeObjectWrapperLockCache.EnterScope()) { - ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); - if (nativeObjectWrapper != null && - nativeObjectWrapper._contextToken == contextToken) + foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { - objects.Add(nativeObjectWrapper._proxyHandle.Target); + ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); + if (nativeObjectWrapper != null && + nativeObjectWrapper._contextToken == contextToken) + { + objects.Add(nativeObjectWrapper._proxyHandle.Target); - // Separate the wrapper from the tracker runtime prior to - // passing them. - nativeObjectWrapper.DisconnectTracker(); + // Separate the wrapper from the tracker runtime prior to + // passing them. + nativeObjectWrapper.DisconnectTracker(); + } } } @@ -1242,6 +1259,7 @@ internal static unsafe void WalkExternalTrackerObjects() { bool walkFailed = false; + // No lock needed to access s_referenceTrackerNativeObjectWrapperCache as this is during GC callback. foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); @@ -1270,6 +1288,7 @@ internal static unsafe void WalkExternalTrackerObjects() // Used during GC callback internal static void DetachNonPromotedObjects() { + // No lock needed to access s_referenceTrackerNativeObjectWrapperCache as this is during GC callback. foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); From b27dabbf5807e3f81935fb8eb805d3aa2bbade15 Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Sat, 6 Jul 2024 19:51:41 -0700 Subject: [PATCH 2/7] Move common logic --- .../InteropServices/ComWrappers.NativeAot.cs | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 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 3a6db27a4be036..51cd90b5c657c0 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 @@ -42,7 +42,7 @@ public abstract partial class ComWrappers private static readonly ConditionalWeakTable s_rcwTable = new ConditionalWeakTable(); private static readonly List s_referenceTrackerNativeObjectWrapperCache = new List(); - private static readonly Lock s_nativeObjectWrapperLockCache = new Lock(useTrivialWaits: true); + private static readonly Lock s_nativeObjectWrapperCacheLock = new Lock(useTrivialWaits: true); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); private readonly Lock _lock = new Lock(useTrivialWaits: true); @@ -638,7 +638,7 @@ public override void Release() // Remove the entry from the cache that keeps track of the active NativeObjectWrappers. if (_nativeObjectWrapperWeakHandle.IsAllocated) { - using (s_nativeObjectWrapperLockCache.EnterScope()) + using (s_nativeObjectWrapperCacheLock.EnterScope()) { s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle); } @@ -988,13 +988,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( throw new NotSupportedException(); } _rcwCache.Add(identity, wrapper._proxyHandle); - if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) - { - using (s_nativeObjectWrapperLockCache.EnterScope()) - { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); - } - } + AddWrapperToReferenceTrackerHandleCache(wrapper); return true; } } @@ -1047,13 +1041,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( wrapper.Release(); throw new NotSupportedException(); } - if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) - { - using (s_nativeObjectWrapperLockCache.EnterScope()) - { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); - } - } + AddWrapperToReferenceTrackerHandleCache(wrapper); return true; } @@ -1089,13 +1077,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( throw new NotSupportedException(); } _rcwCache.Add(identity, wrapper._proxyHandle); - if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) - { - using (s_nativeObjectWrapperLockCache.EnterScope()) - { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); - } - } + AddWrapperToReferenceTrackerHandleCache(wrapper); } } @@ -1103,6 +1085,17 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal( } #pragma warning restore IDE0060 + private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper) + { + if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) + { + using (s_nativeObjectWrapperCacheLock.EnterScope()) + { + s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); + } + } + } + private void RemoveRCWFromCache(IntPtr comPointer, GCHandle expectedValue) { using (_lock.EnterScope()) @@ -1234,7 +1227,7 @@ internal static void ReleaseExternalObjectsFromCurrentThread() List objects = new List(); - using (s_nativeObjectWrapperLockCache.EnterScope()) + using (s_nativeObjectWrapperCacheLock.EnterScope()) { foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { From f9e4b2d5b5b74c0f9ca26db7c2d6f33de98bf4df Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Mon, 8 Jul 2024 17:34:00 -0700 Subject: [PATCH 3/7] Change collection type to HashSet to match with JIT implementation and to improve performance of remove given it now takes a lock --- .../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 51cd90b5c657c0..24500c5090c64a 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 @@ -41,7 +41,7 @@ public abstract partial class ComWrappers 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 List s_referenceTrackerNativeObjectWrapperCache = new List(); + private static readonly HashSet s_referenceTrackerNativeObjectWrapperCache = new HashSet(); private static readonly Lock s_nativeObjectWrapperCacheLock = new Lock(useTrivialWaits: true); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); From 69dc85528d2ee330b3643e3c003ddf095c7c332b Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Thu, 18 Jul 2024 23:13:13 -0700 Subject: [PATCH 4/7] Move to using a custom collection to protect against GC freezing threads during add / remove. --- .../InteropServices/ComWrappers.NativeAot.cs | 250 ++++++++++++++++-- 1 file changed, 235 insertions(+), 15 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 24500c5090c64a..804ac444820a01 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 @@ -1,7 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -41,8 +41,7 @@ public abstract partial class ComWrappers 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 HashSet s_referenceTrackerNativeObjectWrapperCache = new HashSet(); - private static readonly Lock s_nativeObjectWrapperCacheLock = new Lock(useTrivialWaits: true); + private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet(); private readonly ConditionalWeakTable _ccwTable = new ConditionalWeakTable(); private readonly Lock _lock = new Lock(useTrivialWaits: true); @@ -638,10 +637,7 @@ public override void Release() // Remove the entry from the cache that keeps track of the active NativeObjectWrappers. if (_nativeObjectWrapperWeakHandle.IsAllocated) { - using (s_nativeObjectWrapperCacheLock.EnterScope()) - { - s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle); - } + s_referenceTrackerNativeObjectWrapperCache.Remove(_nativeObjectWrapperWeakHandle); _nativeObjectWrapperWeakHandle.Free(); } @@ -1089,10 +1085,7 @@ private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper { if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper) { - using (s_nativeObjectWrapperCacheLock.EnterScope()) - { - s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); - } + s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle); } } @@ -1227,9 +1220,20 @@ internal static void ReleaseExternalObjectsFromCurrentThread() List objects = new List(); - using (s_nativeObjectWrapperCacheLock.EnterScope()) + foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { - foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) + // Here we aren't part of a GC callback, so other threads can still be running + // who are adding and removing from the collection. The collection we are enumerating over + // supports lock free enumeration, but it is possible we can run into handles + // that just got removed and freed. So we guard against that here. If they have + // been freed, we don't need to worry about them as they don't need to be released. + // In addition, we are only handling objects from our current thread here. + if (!weakNativeObjectWrapperHandle.IsAllocated) + { + continue; + } + + try { ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); if (nativeObjectWrapper != null && @@ -1242,6 +1246,11 @@ internal static void ReleaseExternalObjectsFromCurrentThread() nativeObjectWrapper.DisconnectTracker(); } } + catch (InvalidOperationException) + { + // Even though we check for if a handle is allocated before trying to get it, + // it is possible we can race. So catch such scenarios here. + } } s_globalInstanceForTrackerSupport.ReleaseObjects(objects); @@ -1252,7 +1261,6 @@ internal static unsafe void WalkExternalTrackerObjects() { bool walkFailed = false; - // No lock needed to access s_referenceTrackerNativeObjectWrapperCache as this is during GC callback. foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); @@ -1281,7 +1289,6 @@ internal static unsafe void WalkExternalTrackerObjects() // Used during GC callback internal static void DetachNonPromotedObjects() { - // No lock needed to access s_referenceTrackerNativeObjectWrapperCache as this is during GC callback. foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); @@ -1642,4 +1649,217 @@ private static unsafe IntPtr ObjectToComWeakRef(object target, out long wrapperI return IntPtr.Zero; } } + + // This is a GCHandle HashSet implementation based on LowLevelDictionary. + // It uses no locking for readers. While for writers (add / remove), + // it handles the locking itself. + // This implementation specifically makes sure that any readers of this + // collection during GC aren't impacted by other threads being + // frozen while in the middle of an write. It makes no guarantees on + // whether you will observe the element being added / removed, but does + // make sure the collection is in a good state and doesn't run into issues + // while iterating. + internal sealed class GCHandleSet : IEnumerable + { + private const int DefaultSize = 7; + + private Entry?[] _buckets = new Entry[DefaultSize]; + private int _numEntries; + private readonly Lock _lock = new Lock(useTrivialWaits: true); + + public void Add(GCHandle handle) + { + using (_lock.EnterScope()) + { + int bucket = GetBucket(handle); + Entry? prev = null; + Entry? entry = _buckets[bucket]; + while (entry != null) + { + // Handle already exists, nothing to add. + if (handle.Equals(entry.m_value)) + { + return; + } + + prev = entry; + entry = entry.m_next; + } + + Entry newEntry = new Entry() + { + m_value = handle + }; + + if (prev == null) + { + _buckets[bucket] = newEntry; + } + else + { + prev.m_next = newEntry; + } + + // _numEntries is only maintained for the purposes of deciding whether to + // expand the bucket and is not used during iteration to handle the + // scenario where element is in bucket but _numEntries hasn't been incremented + // yet. + _numEntries++; + if (_numEntries > (_buckets.Length * 2)) + { + ExpandBuckets(); + } + } + } + + private void ExpandBuckets() + { + try + { + int newNumBuckets = _buckets.Length * 2 + 1; + Entry?[] newBuckets = new Entry[newNumBuckets]; + for (int i = 0; i < _buckets.Length; i++) + { + Entry? entry = _buckets[i]; + while (entry != null) + { + Entry? nextEntry = entry.m_next; + + int bucket = GetBucket(entry.m_value, newNumBuckets); + + // We are allocating new entries for the bucket to ensure that + // if there is an enumeration already in progress, we don't + // modify what it observes by changing next in existing instances. + Entry newEntry = new Entry() + { + m_value = entry.m_value, + m_next = newBuckets[bucket], + }; + newBuckets[bucket] = newEntry; + + entry = nextEntry; + } + } + _buckets = newBuckets; + } + catch (OutOfMemoryException) + { + } + } + + public void Remove(GCHandle handle) + { + using (_lock.EnterScope()) + { + int bucket = GetBucket(handle); + Entry? prev = null; + Entry? entry = _buckets[bucket]; + while (entry != null) + { + if (handle.Equals(entry.m_value)) + { + if (prev == null) + { + _buckets[bucket] = entry.m_next; + } + else + { + prev.m_next = entry.m_next; + } + _numEntries--; + return; + } + + prev = entry; + entry = entry.m_next; + } + } + } + + private int GetBucket(GCHandle handle, int numBuckets = 0) + { + int h = handle.GetHashCode(); + h &= 0x7fffffff; + return (h % (numBuckets == 0 ? _buckets.Length : numBuckets)); + } + + public Enumerator GetEnumerator() => new Enumerator(this); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)this).GetEnumerator(); + + private sealed class Entry + { + public GCHandle m_value; + public Entry? m_next; + } + + public struct Enumerator : IEnumerator + { + private readonly Entry?[] _buckets; + private int _currentIdx; + private Entry? _currentEntry; + + public Enumerator(GCHandleSet set) + { + // We hold onto the buckets of the set rather than the set itself + // so that if it is ever expanded, we are not impacted by that during + // enumeration. + _buckets = set._buckets; + Reset(); + } + + public GCHandle Current + { + get + { + if (_currentEntry == null) + { + throw new InvalidOperationException("InvalidOperation_EnumOpCantHappen"); + } + + return _currentEntry.m_value; + } + } + + object IEnumerator.Current => Current; + + public void Dispose() + { + } + + public bool MoveNext() + { + if (_currentEntry != null) + { + _currentEntry = _currentEntry.m_next; + } + + if (_currentEntry == null) + { + // Certain buckets might be empty, so loop until we find + // one with an entry. + while (++_currentIdx != _buckets.Length) + { + _currentEntry = _buckets[_currentIdx]; + if (_currentEntry != null) + { + return true; + } + } + + return false; + } + + return true; + } + + public void Reset() + { + _currentIdx = -1; + _currentEntry = null; + } + } + } } From 63a371a9715b8219535be94e9b6708ba702e4ce2 Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Fri, 19 Jul 2024 17:20:42 -0700 Subject: [PATCH 5/7] Address PR feedback by using alternative approach to handle race --- .../InteropServices/ComWrappers.NativeAot.cs | 32 ++++++++----------- 1 file changed, 13 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 804ac444820a01..30272db5104e56 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 @@ -1224,32 +1224,26 @@ internal static void ReleaseExternalObjectsFromCurrentThread() { // Here we aren't part of a GC callback, so other threads can still be running // who are adding and removing from the collection. The collection we are enumerating over - // supports lock free enumeration, but it is possible we can run into handles - // that just got removed and freed. So we guard against that here. If they have - // been freed, we don't need to worry about them as they don't need to be released. + // supports lock free enumeration, but it is possible we can run into handles that just + // got removed and freed. So we guard against that here by using GCHandle.ToIntPtr + // rather than Target given the latter would throw an exception. If they have been freed, + // we don't need to worry about them as they don't need to be released. // In addition, we are only handling objects from our current thread here. - if (!weakNativeObjectWrapperHandle.IsAllocated) + IntPtr ptr = GCHandle.ToIntPtr(weakNativeObjectWrapperHandle); + if (ptr == default) { continue; } - try + ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(GCHandle.InternalGet(ptr)); + if (nativeObjectWrapper != null && + nativeObjectWrapper._contextToken == contextToken) { - ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); - if (nativeObjectWrapper != null && - nativeObjectWrapper._contextToken == contextToken) - { - objects.Add(nativeObjectWrapper._proxyHandle.Target); + objects.Add(nativeObjectWrapper._proxyHandle.Target); - // Separate the wrapper from the tracker runtime prior to - // passing them. - nativeObjectWrapper.DisconnectTracker(); - } - } - catch (InvalidOperationException) - { - // Even though we check for if a handle is allocated before trying to get it, - // it is possible we can race. So catch such scenarios here. + // Separate the wrapper from the tracker runtime prior to + // passing them. + nativeObjectWrapper.DisconnectTracker(); } } From 189ac5f434445edb3a8160362f907cdcfaade11f Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Fri, 26 Jul 2024 11:50:21 -0700 Subject: [PATCH 6/7] Address PR feedback --- .../InteropServices/ComWrappers.NativeAot.cs | 51 ++++++++----------- 1 file changed, 22 insertions(+), 29 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 30272db5104e56..8c10abf6b24f85 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 @@ -1665,7 +1665,7 @@ public void Add(GCHandle handle) { using (_lock.EnterScope()) { - int bucket = GetBucket(handle); + int bucket = GetBucket(handle, _buckets.Length); Entry? prev = null; Entry? entry = _buckets[bucket]; while (entry != null) @@ -1708,44 +1708,38 @@ public void Add(GCHandle handle) private void ExpandBuckets() { - try + int newNumBuckets = _buckets.Length * 2 + 1; + Entry?[] newBuckets = new Entry[newNumBuckets]; + for (int i = 0; i < _buckets.Length; i++) { - int newNumBuckets = _buckets.Length * 2 + 1; - Entry?[] newBuckets = new Entry[newNumBuckets]; - for (int i = 0; i < _buckets.Length; i++) + Entry? entry = _buckets[i]; + while (entry != null) { - Entry? entry = _buckets[i]; - while (entry != null) - { - Entry? nextEntry = entry.m_next; + Entry? nextEntry = entry.m_next; - int bucket = GetBucket(entry.m_value, newNumBuckets); + int bucket = GetBucket(entry.m_value, newNumBuckets); - // We are allocating new entries for the bucket to ensure that - // if there is an enumeration already in progress, we don't - // modify what it observes by changing next in existing instances. - Entry newEntry = new Entry() - { - m_value = entry.m_value, - m_next = newBuckets[bucket], - }; - newBuckets[bucket] = newEntry; + // We are allocating new entries for the bucket to ensure that + // if there is an enumeration already in progress, we don't + // modify what it observes by changing next in existing instances. + Entry newEntry = new Entry() + { + m_value = entry.m_value, + m_next = newBuckets[bucket], + }; + newBuckets[bucket] = newEntry; - entry = nextEntry; - } + entry = nextEntry; } - _buckets = newBuckets; - } - catch (OutOfMemoryException) - { } + _buckets = newBuckets; } public void Remove(GCHandle handle) { using (_lock.EnterScope()) { - int bucket = GetBucket(handle); + int bucket = GetBucket(handle, _buckets.Length); Entry? prev = null; Entry? entry = _buckets[bucket]; while (entry != null) @@ -1770,11 +1764,10 @@ public void Remove(GCHandle handle) } } - private int GetBucket(GCHandle handle, int numBuckets = 0) + private static int GetBucket(GCHandle handle, int numBuckets) { int h = handle.GetHashCode(); - h &= 0x7fffffff; - return (h % (numBuckets == 0 ? _buckets.Length : numBuckets)); + return (int)((uint)h % (uint)numBuckets); } public Enumerator GetEnumerator() => new Enumerator(this); From 19b2790ca9bdb415c5eb5479623898adf6052deb Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Wed, 11 Sep 2024 14:19:03 -0700 Subject: [PATCH 7/7] Address PR feedback around handle being freed. --- .../InteropServices/ComWrappers.NativeAot.cs | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 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 8c10abf6b24f85..cc33e85d73917c 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 @@ -1220,30 +1220,29 @@ internal static void ReleaseExternalObjectsFromCurrentThread() List objects = new List(); - foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) - { - // Here we aren't part of a GC callback, so other threads can still be running - // who are adding and removing from the collection. The collection we are enumerating over - // supports lock free enumeration, but it is possible we can run into handles that just - // got removed and freed. So we guard against that here by using GCHandle.ToIntPtr - // rather than Target given the latter would throw an exception. If they have been freed, - // we don't need to worry about them as they don't need to be released. - // In addition, we are only handling objects from our current thread here. - IntPtr ptr = GCHandle.ToIntPtr(weakNativeObjectWrapperHandle); - if (ptr == default) - { - continue; - } - - ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(GCHandle.InternalGet(ptr)); - if (nativeObjectWrapper != null && - nativeObjectWrapper._contextToken == contextToken) + // Here we aren't part of a GC callback, so other threads can still be running + // who are adding and removing from the collection. This means we can possibly race + // with a handle being removed and freed and we can end up accessing a freed handle. + // To avoid this, we take a lock on modifications to the collection while we gather + // the objects. + using (s_referenceTrackerNativeObjectWrapperCache.ModificationLock.EnterScope()) + { + foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache) { - objects.Add(nativeObjectWrapper._proxyHandle.Target); + ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As(weakNativeObjectWrapperHandle.Target); + if (nativeObjectWrapper != null && + nativeObjectWrapper._contextToken == contextToken) + { + object? target = nativeObjectWrapper._proxyHandle.Target; + if (target != null) + { + objects.Add(target); + } - // Separate the wrapper from the tracker runtime prior to - // passing them. - nativeObjectWrapper.DisconnectTracker(); + // Separate the wrapper from the tracker runtime prior to + // passing them. + nativeObjectWrapper.DisconnectTracker(); + } } } @@ -1661,6 +1660,8 @@ internal sealed class GCHandleSet : IEnumerable private int _numEntries; private readonly Lock _lock = new Lock(useTrivialWaits: true); + public Lock ModificationLock => _lock; + public void Add(GCHandle handle) { using (_lock.EnterScope())