Skip to content

Commit d7948dc

Browse files
manodasanWmanodasanW
authored
Fixes hang in WinUI apps published to AOT (#104583)
* 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. * Move common logic * Change collection type to HashSet to match with JIT implementation and to improve performance of remove given it now takes a lock * Move to using a custom collection to protect against GC freezing threads during add / remove. * Address PR feedback by using alternative approach to handle race * Address PR feedback * Address PR feedback around handle being freed.
1 parent 7e3543d commit d7948dc

File tree

1 file changed

+242
-22
lines changed

1 file changed

+242
-22
lines changed

‎src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs‎

Lines changed: 242 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System;
4+
using System.Collections;
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.Diagnostics.CodeAnalysis;
@@ -41,7 +41,7 @@ public abstract partial class ComWrappers
4141
private static readonly Guid IID_IWeakReferenceSource = new Guid(0x00000038, 0, 0, 0xC0, 0, 0, 0, 0, 0, 0, 0x46);
4242

4343
private static readonly ConditionalWeakTable<object, NativeObjectWrapper> s_rcwTable = new ConditionalWeakTable<object, NativeObjectWrapper>();
44-
private static readonly List<GCHandle> s_referenceTrackerNativeObjectWrapperCache = new List<GCHandle>();
44+
private static readonly GCHandleSet s_referenceTrackerNativeObjectWrapperCache = new GCHandleSet();
4545

4646
private readonly ConditionalWeakTable<object, ManagedObjectWrapperHolder> _ccwTable = new ConditionalWeakTable<object, ManagedObjectWrapperHolder>();
4747
private readonly Lock _lock = new Lock(useTrivialWaits: true);
@@ -984,10 +984,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal(
984984
throw new NotSupportedException();
985985
}
986986
_rcwCache.Add(identity, wrapper._proxyHandle);
987-
if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper)
988-
{
989-
s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle);
990-
}
987+
AddWrapperToReferenceTrackerHandleCache(wrapper);
991988
return true;
992989
}
993990
}
@@ -1040,10 +1037,7 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal(
10401037
wrapper.Release();
10411038
throw new NotSupportedException();
10421039
}
1043-
if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper)
1044-
{
1045-
s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle);
1046-
}
1040+
AddWrapperToReferenceTrackerHandleCache(wrapper);
10471041
return true;
10481042
}
10491043

@@ -1079,17 +1073,22 @@ private unsafe bool TryGetOrCreateObjectForComInstanceInternal(
10791073
throw new NotSupportedException();
10801074
}
10811075
_rcwCache.Add(identity, wrapper._proxyHandle);
1082-
if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper)
1083-
{
1084-
s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle);
1085-
}
1076+
AddWrapperToReferenceTrackerHandleCache(wrapper);
10861077
}
10871078
}
10881079

10891080
return true;
10901081
}
10911082
#pragma warning restore IDE0060
10921083

1084+
private static void AddWrapperToReferenceTrackerHandleCache(NativeObjectWrapper wrapper)
1085+
{
1086+
if (wrapper is ReferenceTrackerNativeObjectWrapper referenceTrackerNativeObjectWrapper)
1087+
{
1088+
s_referenceTrackerNativeObjectWrapperCache.Add(referenceTrackerNativeObjectWrapper._nativeObjectWrapperWeakHandle);
1089+
}
1090+
}
1091+
10931092
private void RemoveRCWFromCache(IntPtr comPointer, GCHandle expectedValue)
10941093
{
10951094
using (_lock.EnterScope())
@@ -1220,17 +1219,30 @@ internal static void ReleaseExternalObjectsFromCurrentThread()
12201219
IntPtr contextToken = GetContextToken();
12211220

12221221
List<object> objects = new List<object>();
1223-
foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache)
1222+
1223+
// Here we aren't part of a GC callback, so other threads can still be running
1224+
// who are adding and removing from the collection. This means we can possibly race
1225+
// with a handle being removed and freed and we can end up accessing a freed handle.
1226+
// To avoid this, we take a lock on modifications to the collection while we gather
1227+
// the objects.
1228+
using (s_referenceTrackerNativeObjectWrapperCache.ModificationLock.EnterScope())
12241229
{
1225-
ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As<ReferenceTrackerNativeObjectWrapper?>(weakNativeObjectWrapperHandle.Target);
1226-
if (nativeObjectWrapper != null &&
1227-
nativeObjectWrapper._contextToken == contextToken)
1230+
foreach (GCHandle weakNativeObjectWrapperHandle in s_referenceTrackerNativeObjectWrapperCache)
12281231
{
1229-
objects.Add(nativeObjectWrapper._proxyHandle.Target);
1232+
ReferenceTrackerNativeObjectWrapper? nativeObjectWrapper = Unsafe.As<ReferenceTrackerNativeObjectWrapper?>(weakNativeObjectWrapperHandle.Target);
1233+
if (nativeObjectWrapper != null &&
1234+
nativeObjectWrapper._contextToken == contextToken)
1235+
{
1236+
object? target = nativeObjectWrapper._proxyHandle.Target;
1237+
if (target != null)
1238+
{
1239+
objects.Add(target);
1240+
}
12301241

1231-
// Separate the wrapper from the tracker runtime prior to
1232-
// passing them.
1233-
nativeObjectWrapper.DisconnectTracker();
1242+
// Separate the wrapper from the tracker runtime prior to
1243+
// passing them.
1244+
nativeObjectWrapper.DisconnectTracker();
1245+
}
12341246
}
12351247
}
12361248

@@ -1630,4 +1642,212 @@ private static unsafe IntPtr ObjectToComWeakRef(object target, out long wrapperI
16301642
return IntPtr.Zero;
16311643
}
16321644
}
1645+
1646+
// This is a GCHandle HashSet implementation based on LowLevelDictionary.
1647+
// It uses no locking for readers. While for writers (add / remove),
1648+
// it handles the locking itself.
1649+
// This implementation specifically makes sure that any readers of this
1650+
// collection during GC aren't impacted by other threads being
1651+
// frozen while in the middle of an write. It makes no guarantees on
1652+
// whether you will observe the element being added / removed, but does
1653+
// make sure the collection is in a good state and doesn't run into issues
1654+
// while iterating.
1655+
internal sealed class GCHandleSet : IEnumerable<GCHandle>
1656+
{
1657+
private const int DefaultSize = 7;
1658+
1659+
private Entry?[] _buckets = new Entry[DefaultSize];
1660+
private int _numEntries;
1661+
private readonly Lock _lock = new Lock(useTrivialWaits: true);
1662+
1663+
public Lock ModificationLock => _lock;
1664+
1665+
public void Add(GCHandle handle)
1666+
{
1667+
using (_lock.EnterScope())
1668+
{
1669+
int bucket = GetBucket(handle, _buckets.Length);
1670+
Entry? prev = null;
1671+
Entry? entry = _buckets[bucket];
1672+
while (entry != null)
1673+
{
1674+
// Handle already exists, nothing to add.
1675+
if (handle.Equals(entry.m_value))
1676+
{
1677+
return;
1678+
}
1679+
1680+
prev = entry;
1681+
entry = entry.m_next;
1682+
}
1683+
1684+
Entry newEntry = new Entry()
1685+
{
1686+
m_value = handle
1687+
};
1688+
1689+
if (prev == null)
1690+
{
1691+
_buckets[bucket] = newEntry;
1692+
}
1693+
else
1694+
{
1695+
prev.m_next = newEntry;
1696+
}
1697+
1698+
// _numEntries is only maintained for the purposes of deciding whether to
1699+
// expand the bucket and is not used during iteration to handle the
1700+
// scenario where element is in bucket but _numEntries hasn't been incremented
1701+
// yet.
1702+
_numEntries++;
1703+
if (_numEntries > (_buckets.Length * 2))
1704+
{
1705+
ExpandBuckets();
1706+
}
1707+
}
1708+
}
1709+
1710+
private void ExpandBuckets()
1711+
{
1712+
int newNumBuckets = _buckets.Length * 2 + 1;
1713+
Entry?[] newBuckets = new Entry[newNumBuckets];
1714+
for (int i = 0; i < _buckets.Length; i++)
1715+
{
1716+
Entry? entry = _buckets[i];
1717+
while (entry != null)
1718+
{
1719+
Entry? nextEntry = entry.m_next;
1720+
1721+
int bucket = GetBucket(entry.m_value, newNumBuckets);
1722+
1723+
// We are allocating new entries for the bucket to ensure that
1724+
// if there is an enumeration already in progress, we don't
1725+
// modify what it observes by changing next in existing instances.
1726+
Entry newEntry = new Entry()
1727+
{
1728+
m_value = entry.m_value,
1729+
m_next = newBuckets[bucket],
1730+
};
1731+
newBuckets[bucket] = newEntry;
1732+
1733+
entry = nextEntry;
1734+
}
1735+
}
1736+
_buckets = newBuckets;
1737+
}
1738+
1739+
public void Remove(GCHandle handle)
1740+
{
1741+
using (_lock.EnterScope())
1742+
{
1743+
int bucket = GetBucket(handle, _buckets.Length);
1744+
Entry? prev = null;
1745+
Entry? entry = _buckets[bucket];
1746+
while (entry != null)
1747+
{
1748+
if (handle.Equals(entry.m_value))
1749+
{
1750+
if (prev == null)
1751+
{
1752+
_buckets[bucket] = entry.m_next;
1753+
}
1754+
else
1755+
{
1756+
prev.m_next = entry.m_next;
1757+
}
1758+
_numEntries--;
1759+
return;
1760+
}
1761+
1762+
prev = entry;
1763+
entry = entry.m_next;
1764+
}
1765+
}
1766+
}
1767+
1768+
private static int GetBucket(GCHandle handle, int numBuckets)
1769+
{
1770+
int h = handle.GetHashCode();
1771+
return (int)((uint)h % (uint)numBuckets);
1772+
}
1773+
1774+
public Enumerator GetEnumerator() => new Enumerator(this);
1775+
1776+
IEnumerator<GCHandle> IEnumerable<GCHandle>.GetEnumerator() => GetEnumerator();
1777+
1778+
IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable<GCHandle>)this).GetEnumerator();
1779+
1780+
private sealed class Entry
1781+
{
1782+
public GCHandle m_value;
1783+
public Entry? m_next;
1784+
}
1785+
1786+
public struct Enumerator : IEnumerator<GCHandle>
1787+
{
1788+
private readonly Entry?[] _buckets;
1789+
private int _currentIdx;
1790+
private Entry? _currentEntry;
1791+
1792+
public Enumerator(GCHandleSet set)
1793+
{
1794+
// We hold onto the buckets of the set rather than the set itself
1795+
// so that if it is ever expanded, we are not impacted by that during
1796+
// enumeration.
1797+
_buckets = set._buckets;
1798+
Reset();
1799+
}
1800+
1801+
public GCHandle Current
1802+
{
1803+
get
1804+
{
1805+
if (_currentEntry == null)
1806+
{
1807+
throw new InvalidOperationException("InvalidOperation_EnumOpCantHappen");
1808+
}
1809+
1810+
return _currentEntry.m_value;
1811+
}
1812+
}
1813+
1814+
object IEnumerator.Current => Current;
1815+
1816+
public void Dispose()
1817+
{
1818+
}
1819+
1820+
public bool MoveNext()
1821+
{
1822+
if (_currentEntry != null)
1823+
{
1824+
_currentEntry = _currentEntry.m_next;
1825+
}
1826+
1827+
if (_currentEntry == null)
1828+
{
1829+
// Certain buckets might be empty, so loop until we find
1830+
// one with an entry.
1831+
while (++_currentIdx != _buckets.Length)
1832+
{
1833+
_currentEntry = _buckets[_currentIdx];
1834+
if (_currentEntry != null)
1835+
{
1836+
return true;
1837+
}
1838+
}
1839+
1840+
return false;
1841+
}
1842+
1843+
return true;
1844+
}
1845+
1846+
public void Reset()
1847+
{
1848+
_currentIdx = -1;
1849+
_currentEntry = null;
1850+
}
1851+
}
1852+
}
16331853
}

0 commit comments

Comments
 (0)