[CoreCLR/NativeAOT] Simplify synchronization in ManagedValueManager

[simonrozsival]

Summary

Simplify synchronization in ManagedValueManager by removing shared mutable state between the bridge thread and managed threads.

Key insight

The bridge thread must not hold ANY managed lock that managed threads also hold. A managed thread holding a shared lock can be suspended at a GC safepoint (e.g., WeakReference.TryGetTarget()). If the GC then triggers bridge processing, the bridge thread will block trying to acquire the same lock, creating a 3-way deadlock: managed thread suspended → bridge blocked on lock → GC waiting for bridge.

This was validated through extensive stress testing (~30,000 iterations, ~1,500 GC cycles per run) on a physical Android device.

Changes

1. Remove bridgeProcessingSemaphore — WaitForGCBridgeProcessing() becomes a no-op. The semaphore Wait()/Release() pattern was not actually preventing anything useful.

2. Embed GCHandle directly in HandleContext struct — eliminates the Dictionary<IntPtr, GCHandle> referenceTrackingHandles which was the last shared mutable state between bridge and managed threads. The GCHandle is stored as an IntPtr field in the struct (native code only accesses the first two fields, so this is safe).

3. Move EnsureAllContextsAreOurs behind #if DEBUG — this validation used the now-removed dictionary. In DEBUG builds, a simple HashSet<IntPtr> provides the same validation.

4. Schedule CollectPeers via Task.Run after bridge processing — instead of draining the collected contexts queue on every AddPeer call, cleanup is scheduled on a thread pool thread after FinishCrossReferenceProcessing returns.

5. Use dedicated System.Threading.Lock — replace lock(RegisteredInstances) with a dedicated Lock instance.

Data ownership after this change

Data	Owner	Protected by
RegisteredInstances	Managed threads	_registeredInstancesLock
HandleContext native memory	Bridge thread (alloc on managed, free on bridge)	N/A (single owner)
CollectedContexts queue	Bridge enqueues, managed dequeues	ConcurrentQueue (lock-free)

The bridge thread now touches zero shared locks in Release builds.

Fixes #10783

[Copilot]

Pull request overview

This PR updates CoreCLR/NativeAOT GC-bridge synchronization in ManagedValueManager by replacing the prior SemaphoreSlim/lock approach with a ReaderWriterLockSlim intended to better coordinate access to peer registration state during bridge processing.

Changes:

• Replaced SemaphoreSlim + lock (RegisteredInstances) with a single ReaderWriterLockSlim (read locks for lookups, write locks for mutations).
• Refactored AddPeer, RemovePeer, PeekPeer, and GetSurfacedPeers to use the new lock strategy.
• Removed locking around HandleContext.referenceTrackingHandles and altered bridge-processing start/finish synchronization to use the new lock.

Comments suppressed due to low confidence (1)

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs:74

• CollectPeers() releases rwLock before calling HandleContext.Free(), but Free() mutates the static referenceTrackingHandles dictionary. This can race with bridge processing (or other writer operations) and can corrupt the dictionary. Keep HandleContext.Free() within the same write-lock scope (or otherwise synchronize access to referenceTrackingHandles).

			rwLock.EnterWriteLock ();
			try {
				Remove (context);
			} finally {
				rwLock.ExitWriteLock ();
			}

			HandleContext.Free (ref context);
		}

[simonrozsival]

I had to make more changes because the ReadWriteLockSlim was causing deadlocks.

In its final state, this PR just simplifies the existing code and cleans it up. The most significant change is removing the SemaphoreSlim bridgeProcessingSemaphore which adds unnecessary overhead and does not prevent any race conditions.

[jonathanpeppers]

Should this call GetOrCreateInstance()? Is there a way to do the equivalent of Get() and return early if none found?

[jonathanpeppers]

public static ManagedValueManager GetOrCreateInstance () => s_instance.Value; actually we might rename this to be Instance to help confusion in the future.

[jonathanpeppers]

I think there is a test failure worth looking into:

Mono.Android.NET_Tests, Java.InteropTests.JavaObjectTest.Dispose_Finalized / Release
System.AggregateException : AggregateException_ctor_DefaultMessage (  Expected: True
But was:  False
)
----> NUnit.Framework.AssertionException :   Expected: True
But was:  False

[simonrozsival]

@copilot address Jonathan's reveiw comments. I have struggled with the Dispose_Finalized test originally when I was implementing the GC bridge, so this is likely due to the changes to CollectPeers or some deferred disposal of handles/contexts. Analyze this test and give me your best reasoning of the usefulness of this test. Should we make sure this test passes as is? Should we modify the test to reflect the changes to ManagedValueManager?

[Copilot]

@simonrozsival I've opened a new pull request, #10806, to work on those changes. Once the pull request is ready, I'll request review from you.

[jonathanpeppers]

There must be one spot using the old name:

/Users/runner/work/1/s/android/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs(64,60): error CS0117: 'ManagedValueManager' does not contain a definition for 'GetOrCreateInstance'