From b2ae2dc9b5ceb2fb29255e6e8759ff4cc15f0df0 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 16:24:29 +0200 Subject: [PATCH 1/8] Make peer creation atomic Canonicalize concurrently created Java peers against the registered winner and make the CoreCLR peer registry safe for concurrent access. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c1b612e-232a-4e9f-983b-45dd00dd9b4e --- .../JniRuntime.JniValueManager.cs | 29 +++- .../JniRuntime.JniValueManagerTests.cs | 122 ++++++++++++++++ .../JavaMarshalRegisteredPeers.cs | 130 +++++++++--------- .../TrimmableTypeMapValueManager.cs | 17 ++- 4 files changed, 227 insertions(+), 71 deletions(-) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index a5383701a1e..469f0d49b6c 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -200,13 +200,30 @@ protected virtual bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (tr return null; } - var peeked = PeekPeer (reference); - if (peeked != null && - (targetType == null || - targetType.IsAssignableFrom (peeked.GetType ()))) { - return peeked; + while (true) { + var peeked = PeekPeer (reference); + if (peeked != null && + (targetType == null || + targetType.IsAssignableFrom (peeked.GetType ()))) { + return peeked; + } + + var created = CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); + var registered = PeekPeer (reference); + if (registered != null && + (targetType == null || + targetType.IsAssignableFrom (registered.GetType ()))) { + if (created != null && !ReferenceEquals (created, registered)) { + DisposePeerUnlessReferenced (created); + } + return registered; + } + if (registered != null || created == null) { + return created; + } + + DisposePeerUnlessReferenced (created); } - return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); } public abstract IJavaPeerable? CreatePeer ( diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs index a712105d7be..30b6deaa127 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs @@ -2,6 +2,8 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; using Java.Interop; @@ -30,6 +32,126 @@ public void CreateValue () } } + [Test] + public void GetPeer_ReturnsRegisteredPeerConcurrently () + { + using (var source = new JavaObject ()) + using (var vm = new ConcurrentGetPeerValueManager (source.JniPeerMembers)) + using (var start = new Barrier (2)) { + vm.OnSetRuntime (JniRuntime.CurrentRuntime); + vm.InitialPeekBarrier = start; + + var first = Task.Run (() => vm.GetPeer (source.PeerReference)); + var second = Task.Run (() => vm.GetPeer (source.PeerReference)); + Task.WaitAll (first, second); + + Assert.AreEqual (2, vm.CreatePeerCount); + Assert.AreSame (first.Result, second.Result); + } + } + + class ConcurrentGetPeerValueManager : MyValueManager { + + readonly JniPeerMembers peerMembers; + IJavaPeerable registeredPeer; + int peekPeerCount; + int createPeerCount; + + public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) + { + this.peerMembers = peerMembers; + } + + public Barrier InitialPeekBarrier { get; set; } + + public int CreatePeerCount => createPeerCount; + + public override IJavaPeerable PeekPeer (JniObjectReference reference) + { + var peer = Volatile.Read (ref registeredPeer); + if (Interlocked.Increment (ref peekPeerCount) <= 2 && InitialPeekBarrier != null) { + var barrier = InitialPeekBarrier; + if (!barrier.SignalAndWait (TimeSpan.FromSeconds (10))) { + throw new TimeoutException ("Timed out waiting for concurrent GetPeer() calls."); + } + } + return peer; + } + + public override IJavaPeerable CreatePeer ( + ref JniObjectReference reference, + JniObjectReferenceOptions transfer, + [DynamicallyAccessedMembers (Constructors)] + Type targetType) + { + Interlocked.Increment (ref createPeerCount); + var peer = new TestPeer (reference, peerMembers); + Interlocked.CompareExchange (ref registeredPeer, peer, null); + return peer; + } + + public override void DisposePeerUnlessReferenced (IJavaPeerable value) + { + value.Dispose (); + } + } + + class TestPeer : IJavaPeerable { + + JniObjectReference reference; + int identityHashCode; + JniManagedPeerStates state; + + public TestPeer (JniObjectReference reference, JniPeerMembers peerMembers) + { + this.reference = reference; + JniPeerMembers = peerMembers; + } + + public int JniIdentityHashCode => identityHashCode; + + public JniObjectReference PeerReference => reference; + + public JniPeerMembers JniPeerMembers { get; } + + public JniManagedPeerStates JniManagedPeerState => state; + + public void SetJniIdentityHashCode (int value) + { + identityHashCode = value; + } + + public void SetPeerReference (JniObjectReference value) + { + reference = value; + } + + public void SetJniManagedPeerState (JniManagedPeerStates value) + { + state = value; + } + + public void UnregisterFromRuntime () + { + } + + public void DisposeUnlessReferenced () + { + } + + public void Disposed () + { + } + + public void Finalized () + { + } + + public void Dispose () + { + } + } + [UnconditionalSuppressMessage ("AOT", "IL3050", Justification = "MyValueManager intentionally uses reflection-backed value manager behavior for tests.")] [UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "MyValueManager intentionally uses reflection-backed value manager behavior for tests.")] class MyValueManager : JniRuntime.ReflectionJniValueManager { diff --git a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs index 18f4ef0aaa9..5769f12532f 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs @@ -31,7 +31,7 @@ namespace Microsoft.Android.Runtime; /// static class JavaMarshalRegisteredPeers { - static readonly Dictionary> RegisteredInstances = new (); + static readonly ConcurrentDictionary> RegisteredInstances = new (); static readonly ConcurrentQueue CollectedContexts = new (); static readonly object initializeLock = new (); @@ -65,9 +65,7 @@ public static void CollectPeers () Debug.Assert (contextPtr != IntPtr.Zero, "CollectedContexts should not contain null pointers."); HandleContext* context = (HandleContext*)contextPtr; - lock (RegisteredInstances) { - Remove (context); - } + Remove (context); HandleContext.Free (ref context); } @@ -78,15 +76,13 @@ void Remove (HandleContext* context) if (!RegisteredInstances.TryGetValue (key, out List? peers)) return; - for (int i = peers.Count - 1; i >= 0; i--) { - var peer = peers [i]; - if (peer.BelongsToContext (context)) { - peers.RemoveAt (i); + lock (peers) { + for (int i = peers.Count - 1; i >= 0; i--) { + if (peers [i].BelongsToContext (context)) { + peers.RemoveAt (i); + } } - } - - if (peers.Count == 0) { - RegisteredInstances.Remove (key); + RemoveListIfEmpty (key, peers); } } } @@ -106,44 +102,48 @@ public static void AddPeer (IJavaPeerable value) JniObjectReference.Dispose (ref r, JniObjectReferenceOptions.CopyAndDispose); } int key = value.JniIdentityHashCode; - lock (RegisteredInstances) { - List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) { - peers = [new ReferenceTrackingHandle (value)]; - RegisteredInstances.Add (key, peers); - return; - } - - for (int i = peers.Count - 1; i >= 0; i--) { - ReferenceTrackingHandle peer = peers [i]; - if (peer.Target is not IJavaPeerable target) - continue; - if (!JniEnvironment.Types.IsSameObject (target.PeerReference, value.PeerReference)) + while (true) { + var peers = RegisteredInstances.GetOrAdd (key, static _ => []); + lock (peers) { + if (!RegisteredInstances.TryGetValue (key, out List? current) || + !ReferenceEquals (peers, current)) { continue; - // JNIEnv.NewObject/JNIEnv.CreateInstance() compatibility. - // When two MCW's are created for one Java instance [0], - // we want the 2nd MCW to replace the 1st, as the 2nd is - // the one the dev created; the 1st is an implicit intermediary. - // - // Meanwhile, a new "replaceable" instance should *not* replace an - // existing "replaceable" instance; see dotnet/android#9862. - // - // [0]: If Java ctor invokes overridden virtual method, we'll - // transition into managed code w/o a registered instance, and - // thus will create an "intermediary" via - // (IntPtr, JniHandleOwnership) .ctor. - if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) && - !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { - peer.Dispose (); - peers [i] = new ReferenceTrackingHandle (value); - } else if (JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) { - WarnNotReplacing (key, value, target); } - GC.KeepAlive (target); + + for (int i = peers.Count - 1; i >= 0; i--) { + ReferenceTrackingHandle peer = peers [i]; + if (peer.Target is not IJavaPeerable target) + continue; + if (!JniEnvironment.Types.IsSameObject (target.PeerReference, value.PeerReference)) + continue; + // JNIEnv.NewObject/JNIEnv.CreateInstance() compatibility. + // When two MCW's are created for one Java instance [0], + // we want the 2nd MCW to replace the 1st, as the 2nd is + // the one the dev created; the 1st is an implicit intermediary. + // + // Meanwhile, a new "replaceable" instance should *not* replace an + // existing "replaceable" instance; see dotnet/android#9862. + // + // [0]: If Java ctor invokes overridden virtual method, we'll + // transition into managed code w/o a registered instance, and + // thus will create an "intermediary" via + // (IntPtr, JniHandleOwnership) .ctor. + if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) && + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { + peer.Dispose (); + peers [i] = new ReferenceTrackingHandle (value); + } else if ((!target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) || + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) && + JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) { + WarnNotReplacing (key, value, target); + } + GC.KeepAlive (target); + return; + } + + peers.Add (new ReferenceTrackingHandle (value)); return; } - - peers.Add (new ReferenceTrackingHandle (value)); } } @@ -170,10 +170,10 @@ static void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable int key = JniEnvironment.References.GetIdentityHashCode (reference); - lock (RegisteredInstances) { - if (!RegisteredInstances.TryGetValue (key, out List? peers)) - return null; + if (!RegisteredInstances.TryGetValue (key, out List? peers)) + return null; + lock (peers) { for (int i = peers.Count - 1; i >= 0; i--) { if (peers [i].Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) @@ -181,11 +181,8 @@ static void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable return peer; } } - - if (peers.Count == 0) - RegisteredInstances.Remove (key); + return null; } - return null; } public static void RemovePeer (IJavaPeerable value) @@ -196,11 +193,11 @@ public static void RemovePeer (IJavaPeerable value) if (value == null) throw new ArgumentNullException (nameof (value)); - lock (RegisteredInstances) { - int key = value.JniIdentityHashCode; - if (!RegisteredInstances.TryGetValue (key, out List? peers)) - return; + int key = value.JniIdentityHashCode; + if (!RegisteredInstances.TryGetValue (key, out List? peers)) + return; + lock (peers) { for (int i = peers.Count - 1; i >= 0; i--) { ReferenceTrackingHandle peer = peers [i]; IJavaPeerable? target = peer.Target; @@ -210,8 +207,7 @@ public static void RemovePeer (IJavaPeerable value) } GC.KeepAlive (target); } - if (peers.Count == 0) - RegisteredInstances.Remove (key); + RemoveListIfEmpty (key, peers); } } @@ -255,16 +251,24 @@ public static List GetSurfacedPeers () // Remove any collected contexts before iterating over all the registered instances CollectPeers (); - lock (RegisteredInstances) { - var peers = new List (RegisteredInstances.Count); - foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) { + var peers = new List (RegisteredInstances.Count); + foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) { + lock (referenceTrackingHandles) { foreach (var peer in referenceTrackingHandles) { if (peer.Target is IJavaPeerable target) { peers.Add (new JniSurfacedPeerInfo (identityHashCode, new WeakReference (target))); } } } - return peers; + } + return peers; + } + + static void RemoveListIfEmpty (int key, List peers) + { + if (peers.Count == 0) { + ((ICollection>>) RegisteredInstances) + .Remove (new KeyValuePair> (key, peers)); } } diff --git a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs index b2344f8fd2a..33ba8913e3a 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs @@ -15,6 +15,9 @@ sealed partial class TrimmableTypeMapValueManager : JniRuntime.JniValueManager const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; const JniObjectReferenceOptions DoNotRegisterTarget = JniObjectReferenceOptions.CopyAndDoNotRegister & ~JniObjectReferenceOptions.Copy; + [ThreadStatic] + static JniObjectReference createPeerReference; + public TrimmableTypeMapValueManager () { JavaMarshalRegisteredPeers.InitializeIfNeeded (); @@ -112,6 +115,10 @@ protected override void ConstructPeerCore ( } if ((options & DoNotRegisterTarget) != DoNotRegisterTarget) { + if (createPeerReference.IsValid && + JniEnvironment.Types.IsSameObject (peer.PeerReference, createPeerReference)) { + peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable); + } AddPeer (peer); } } @@ -130,8 +137,14 @@ protected override void ConstructPeerCore ( try { var resolvedTargetType = ResolvePeerType (targetType); - return TrimmableTypeMap.Instance.CreateInstance (reference.Handle, resolvedTargetType) - ?? NotFoundFallback (ref reference, targetType, resolvedTargetType); + var previousCreatePeerReference = createPeerReference; + createPeerReference = reference; + try { + return TrimmableTypeMap.Instance.CreateInstance (reference.Handle, resolvedTargetType) + ?? NotFoundFallback (ref reference, targetType, resolvedTargetType); + } finally { + createPeerReference = previousCreatePeerReference; + } } finally { JniObjectReference.Dispose (ref reference, transfer); } From cb37b02951de50014ddee214c6926c502bab71bd Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 16:57:14 +0200 Subject: [PATCH 2/8] Simplify concurrent peer reconciliation Keep the existing registry lock and reconcile concurrent peer creation in one explicit pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../JniRuntime.JniValueManager.cs | 39 +++--- .../JniRuntime.JniValueManagerTests.cs | 4 + .../JavaMarshalRegisteredPeers.cs | 132 +++++++++--------- 3 files changed, 87 insertions(+), 88 deletions(-) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 469f0d49b6c..86ac6968cdd 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -200,32 +200,29 @@ protected virtual bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (tr return null; } - while (true) { - var peeked = PeekPeer (reference); - if (peeked != null && - (targetType == null || - targetType.IsAssignableFrom (peeked.GetType ()))) { - return peeked; - } + var existing = PeekPeer (reference); + if (IsCompatiblePeer (existing, targetType)) { + return existing; + } - var created = CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); - var registered = PeekPeer (reference); - if (registered != null && - (targetType == null || - targetType.IsAssignableFrom (registered.GetType ()))) { - if (created != null && !ReferenceEquals (created, registered)) { - DisposePeerUnlessReferenced (created); - } - return registered; - } - if (registered != null || created == null) { - return created; - } + var created = CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); - DisposePeerUnlessReferenced (created); + // Peer construction registers the new instance. Another thread may have + // registered its own instance first, so always return the registered winner. + var registered = PeekPeer (reference); + if (IsCompatiblePeer (registered, targetType)) { + if (created != null && !ReferenceEquals (created, registered)) { + DisposePeerUnlessReferenced (created); + } + return registered; } + + return created; } + static bool IsCompatiblePeer (IJavaPeerable? peer, Type? targetType) + => peer != null && (targetType == null || targetType.IsAssignableFrom (peer.GetType ())); + public abstract IJavaPeerable? CreatePeer ( ref JniObjectReference reference, JniObjectReferenceOptions transfer, diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs index 30b6deaa127..45f60f9f885 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs @@ -46,6 +46,7 @@ public void GetPeer_ReturnsRegisteredPeerConcurrently () Task.WaitAll (first, second); Assert.AreEqual (2, vm.CreatePeerCount); + Assert.AreEqual (1, vm.DisposePeerCount); Assert.AreSame (first.Result, second.Result); } } @@ -56,6 +57,7 @@ class ConcurrentGetPeerValueManager : MyValueManager { IJavaPeerable registeredPeer; int peekPeerCount; int createPeerCount; + int disposePeerCount; public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) { @@ -65,6 +67,7 @@ public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) public Barrier InitialPeekBarrier { get; set; } public int CreatePeerCount => createPeerCount; + public int DisposePeerCount => disposePeerCount; public override IJavaPeerable PeekPeer (JniObjectReference reference) { @@ -92,6 +95,7 @@ public override IJavaPeerable CreatePeer ( public override void DisposePeerUnlessReferenced (IJavaPeerable value) { + Interlocked.Increment (ref disposePeerCount); value.Dispose (); } } diff --git a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs index 5769f12532f..ab25587571e 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs @@ -31,7 +31,7 @@ namespace Microsoft.Android.Runtime; /// static class JavaMarshalRegisteredPeers { - static readonly ConcurrentDictionary> RegisteredInstances = new (); + static readonly Dictionary> RegisteredInstances = new (); static readonly ConcurrentQueue CollectedContexts = new (); static readonly object initializeLock = new (); @@ -65,7 +65,9 @@ public static void CollectPeers () Debug.Assert (contextPtr != IntPtr.Zero, "CollectedContexts should not contain null pointers."); HandleContext* context = (HandleContext*)contextPtr; - Remove (context); + lock (RegisteredInstances) { + Remove (context); + } HandleContext.Free (ref context); } @@ -76,13 +78,15 @@ void Remove (HandleContext* context) if (!RegisteredInstances.TryGetValue (key, out List? peers)) return; - lock (peers) { - for (int i = peers.Count - 1; i >= 0; i--) { - if (peers [i].BelongsToContext (context)) { - peers.RemoveAt (i); - } + for (int i = peers.Count - 1; i >= 0; i--) { + var peer = peers [i]; + if (peer.BelongsToContext (context)) { + peers.RemoveAt (i); } - RemoveListIfEmpty (key, peers); + } + + if (peers.Count == 0) { + RegisteredInstances.Remove (key); } } } @@ -102,48 +106,46 @@ public static void AddPeer (IJavaPeerable value) JniObjectReference.Dispose (ref r, JniObjectReferenceOptions.CopyAndDispose); } int key = value.JniIdentityHashCode; - while (true) { - var peers = RegisteredInstances.GetOrAdd (key, static _ => []); - lock (peers) { - if (!RegisteredInstances.TryGetValue (key, out List? current) || - !ReferenceEquals (peers, current)) { - continue; - } + lock (RegisteredInstances) { + List? peers; + if (!RegisteredInstances.TryGetValue (key, out peers)) { + peers = [new ReferenceTrackingHandle (value)]; + RegisteredInstances.Add (key, peers); + return; + } - for (int i = peers.Count - 1; i >= 0; i--) { - ReferenceTrackingHandle peer = peers [i]; - if (peer.Target is not IJavaPeerable target) - continue; - if (!JniEnvironment.Types.IsSameObject (target.PeerReference, value.PeerReference)) - continue; - // JNIEnv.NewObject/JNIEnv.CreateInstance() compatibility. - // When two MCW's are created for one Java instance [0], - // we want the 2nd MCW to replace the 1st, as the 2nd is - // the one the dev created; the 1st is an implicit intermediary. - // - // Meanwhile, a new "replaceable" instance should *not* replace an - // existing "replaceable" instance; see dotnet/android#9862. - // - // [0]: If Java ctor invokes overridden virtual method, we'll - // transition into managed code w/o a registered instance, and - // thus will create an "intermediary" via - // (IntPtr, JniHandleOwnership) .ctor. - if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) && - !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { - peer.Dispose (); - peers [i] = new ReferenceTrackingHandle (value); - } else if ((!target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) || - !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) && - JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) { - WarnNotReplacing (key, value, target); - } - GC.KeepAlive (target); - return; + for (int i = peers.Count - 1; i >= 0; i--) { + ReferenceTrackingHandle peer = peers [i]; + if (peer.Target is not IJavaPeerable target) + continue; + if (!JniEnvironment.Types.IsSameObject (target.PeerReference, value.PeerReference)) + continue; + // JNIEnv.NewObject/JNIEnv.CreateInstance() compatibility. + // When two MCW's are created for one Java instance [0], + // we want the 2nd MCW to replace the 1st, as the 2nd is + // the one the dev created; the 1st is an implicit intermediary. + // + // Meanwhile, a new "replaceable" instance should *not* replace an + // existing "replaceable" instance; see dotnet/android#9862. + // + // [0]: If Java ctor invokes overridden virtual method, we'll + // transition into managed code w/o a registered instance, and + // thus will create an "intermediary" via + // (IntPtr, JniHandleOwnership) .ctor. + if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) && + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { + peer.Dispose (); + peers [i] = new ReferenceTrackingHandle (value); + } else if ((!target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) || + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) && + JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) { + WarnNotReplacing (key, value, target); } - - peers.Add (new ReferenceTrackingHandle (value)); + GC.KeepAlive (target); return; } + + peers.Add (new ReferenceTrackingHandle (value)); } } @@ -170,10 +172,10 @@ static void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable int key = JniEnvironment.References.GetIdentityHashCode (reference); - if (!RegisteredInstances.TryGetValue (key, out List? peers)) - return null; + lock (RegisteredInstances) { + if (!RegisteredInstances.TryGetValue (key, out List? peers)) + return null; - lock (peers) { for (int i = peers.Count - 1; i >= 0; i--) { if (peers [i].Target is IJavaPeerable peer && JniEnvironment.Types.IsSameObject (reference, peer.PeerReference)) @@ -181,8 +183,11 @@ static void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable return peer; } } - return null; + + if (peers.Count == 0) + RegisteredInstances.Remove (key); } + return null; } public static void RemovePeer (IJavaPeerable value) @@ -193,11 +198,11 @@ public static void RemovePeer (IJavaPeerable value) if (value == null) throw new ArgumentNullException (nameof (value)); - int key = value.JniIdentityHashCode; - if (!RegisteredInstances.TryGetValue (key, out List? peers)) - return; + lock (RegisteredInstances) { + int key = value.JniIdentityHashCode; + if (!RegisteredInstances.TryGetValue (key, out List? peers)) + return; - lock (peers) { for (int i = peers.Count - 1; i >= 0; i--) { ReferenceTrackingHandle peer = peers [i]; IJavaPeerable? target = peer.Target; @@ -207,7 +212,8 @@ public static void RemovePeer (IJavaPeerable value) } GC.KeepAlive (target); } - RemoveListIfEmpty (key, peers); + if (peers.Count == 0) + RegisteredInstances.Remove (key); } } @@ -251,24 +257,16 @@ public static List GetSurfacedPeers () // Remove any collected contexts before iterating over all the registered instances CollectPeers (); - var peers = new List (RegisteredInstances.Count); - foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) { - lock (referenceTrackingHandles) { + lock (RegisteredInstances) { + var peers = new List (RegisteredInstances.Count); + foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) { foreach (var peer in referenceTrackingHandles) { if (peer.Target is IJavaPeerable target) { peers.Add (new JniSurfacedPeerInfo (identityHashCode, new WeakReference (target))); } } } - } - return peers; - } - - static void RemoveListIfEmpty (int key, List peers) - { - if (peers.Count == 0) { - ((ICollection>>) RegisteredInstances) - .Remove (new KeyValuePair> (key, peers)); + return peers; } } From 0982cb059e80bc6d1df102fc0f251e3bf038c7df Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 17:01:48 +0200 Subject: [PATCH 3/8] Serialize peer creation Double-check the peer registry while holding a value-manager lock so only one concurrent caller creates and registers a peer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../JniRuntime.JniValueManager.cs | 20 +++++++++---------- .../JniRuntime.JniValueManagerTests.cs | 10 +--------- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 86ac6968cdd..b70b124c762 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -45,6 +45,7 @@ public abstract partial class JniValueManager : ISetRuntime, IDisposable { internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; JniRuntime? runtime; + readonly object peerCreationLock = new (); bool disposed; public JniRuntime Runtime { get => runtime ?? throw new NotSupportedException (); @@ -205,19 +206,16 @@ protected virtual bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (tr return existing; } - var created = CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); - - // Peer construction registers the new instance. Another thread may have - // registered its own instance first, so always return the registered winner. - var registered = PeekPeer (reference); - if (IsCompatiblePeer (registered, targetType)) { - if (created != null && !ReferenceEquals (created, registered)) { - DisposePeerUnlessReferenced (created); + lock (peerCreationLock) { + // CreatePeer registers the new peer before returning. Check again while + // holding the lock so only one caller creates a peer for this reference. + existing = PeekPeer (reference); + if (IsCompatiblePeer (existing, targetType)) { + return existing; } - return registered; - } - return created; + return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); + } } static bool IsCompatiblePeer (IJavaPeerable? peer, Type? targetType) diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs index 45f60f9f885..b5ff337c394 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs @@ -45,8 +45,7 @@ public void GetPeer_ReturnsRegisteredPeerConcurrently () var second = Task.Run (() => vm.GetPeer (source.PeerReference)); Task.WaitAll (first, second); - Assert.AreEqual (2, vm.CreatePeerCount); - Assert.AreEqual (1, vm.DisposePeerCount); + Assert.AreEqual (1, vm.CreatePeerCount); Assert.AreSame (first.Result, second.Result); } } @@ -57,7 +56,6 @@ class ConcurrentGetPeerValueManager : MyValueManager { IJavaPeerable registeredPeer; int peekPeerCount; int createPeerCount; - int disposePeerCount; public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) { @@ -67,7 +65,6 @@ public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) public Barrier InitialPeekBarrier { get; set; } public int CreatePeerCount => createPeerCount; - public int DisposePeerCount => disposePeerCount; public override IJavaPeerable PeekPeer (JniObjectReference reference) { @@ -93,11 +90,6 @@ public override IJavaPeerable CreatePeer ( return peer; } - public override void DisposePeerUnlessReferenced (IJavaPeerable value) - { - Interlocked.Increment (ref disposePeerCount); - value.Dispose (); - } } class TestPeer : IJavaPeerable { From 28f375e39e262143a13b62949b441fbdb64f5459 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 17:10:25 +0200 Subject: [PATCH 4/8] Use System.Threading.Lock for peer creation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index b70b124c762..124b3062720 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -5,6 +5,7 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; +using System.Threading; namespace Java.Interop { @@ -45,7 +46,7 @@ public abstract partial class JniValueManager : ISetRuntime, IDisposable { internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; JniRuntime? runtime; - readonly object peerCreationLock = new (); + readonly Lock peerCreationLock = new (); bool disposed; public JniRuntime Runtime { get => runtime ?? throw new NotSupportedException (); From 615dafb0ed13bf82adcfb94d5f2badc4dbf54999 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 17:10:44 +0200 Subject: [PATCH 5/8] Avoid peer state checks when logging is disabled Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs index ab25587571e..e0134c7b084 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs @@ -136,9 +136,9 @@ public static void AddPeer (IJavaPeerable value) !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) { peer.Dispose (); peers [i] = new ReferenceTrackingHandle (value); - } else if ((!target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) || - !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) && - JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) { + } else if (JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages && + (!target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) || + !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable))) { WarnNotReplacing (key, value, target); } GC.KeepAlive (target); From c8151283c7d2d74b9685b053021cf80356675609 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Thu, 20 Aug 2026 17:23:45 +0200 Subject: [PATCH 6/8] Register trimmable peers explicitly Construct implicitly created peers without registration, mark them replaceable, and then register them explicitly instead of carrying creation state in thread-local storage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../TrimmableTypeMap.cs | 15 +++++++++------ .../TrimmableTypeMapValueManager.cs | 17 ++--------------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs index f887629cab6..01f57dfe757 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs @@ -19,6 +19,8 @@ namespace Microsoft.Android.Runtime; /// public class TrimmableTypeMap { + const JniHandleOwnership CreatedPeerOwnership = JniHandleOwnership.DoNotTransfer | JniHandleOwnership.DoNotRegister; + static readonly Lock s_initLock = new (); static readonly JavaPeerProxy s_noPeerSentinel = new MissingJavaPeerProxy (); static TrimmableTypeMap? s_instance; @@ -348,21 +350,21 @@ static JniMethodInfo GetClassGetInterfacesMethod () IJavaPeerable? peer; if (ShouldActivateClosedGenericTarget (proxy, targetType)) { - peer = ActivateUsingReflection (targetType, handle, JniHandleOwnership.DoNotTransfer); + peer = ActivateUsingReflection (targetType, handle, CreatedPeerOwnership); } else { - peer = proxy?.CreateInstance (handle, JniHandleOwnership.DoNotTransfer); + peer = proxy?.CreateInstance (handle, CreatedPeerOwnership); } if (peer is not null) { - MarkCreatedPeer (peer); + RegisterCreatedPeer (peer); } return peer; } internal IJavaPeerable? CreateInstanceWithoutReflectionFallback (IntPtr handle, Type? targetType = null) { - var peer = GetProxyForJavaObject (handle, targetType)?.CreateInstance (handle, JniHandleOwnership.DoNotTransfer); + var peer = GetProxyForJavaObject (handle, targetType)?.CreateInstance (handle, CreatedPeerOwnership); if (peer is not null) { - MarkCreatedPeer (peer); + RegisterCreatedPeer (peer); } return peer; } @@ -398,13 +400,14 @@ targetType is not null && return (IJavaPeerable) ctor.Invoke ([handle, transfer]); } - static void MarkCreatedPeer (IJavaPeerable peer) + static void RegisterCreatedPeer (IJavaPeerable peer) { var peerState = peer.JniManagedPeerState | JniManagedPeerStates.Replaceable; if (global::Java.Interop.Runtime.IsGCUserPeer (peer.PeerReference.Handle)) { peerState |= JniManagedPeerStates.Activatable; } peer.SetJniManagedPeerState (peerState); + JniEnvironment.Runtime.ValueManager.AddPeer (peer); } /// diff --git a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs index 33ba8913e3a..b2344f8fd2a 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMapValueManager.cs @@ -15,9 +15,6 @@ sealed partial class TrimmableTypeMapValueManager : JniRuntime.JniValueManager const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; const JniObjectReferenceOptions DoNotRegisterTarget = JniObjectReferenceOptions.CopyAndDoNotRegister & ~JniObjectReferenceOptions.Copy; - [ThreadStatic] - static JniObjectReference createPeerReference; - public TrimmableTypeMapValueManager () { JavaMarshalRegisteredPeers.InitializeIfNeeded (); @@ -115,10 +112,6 @@ protected override void ConstructPeerCore ( } if ((options & DoNotRegisterTarget) != DoNotRegisterTarget) { - if (createPeerReference.IsValid && - JniEnvironment.Types.IsSameObject (peer.PeerReference, createPeerReference)) { - peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable); - } AddPeer (peer); } } @@ -137,14 +130,8 @@ protected override void ConstructPeerCore ( try { var resolvedTargetType = ResolvePeerType (targetType); - var previousCreatePeerReference = createPeerReference; - createPeerReference = reference; - try { - return TrimmableTypeMap.Instance.CreateInstance (reference.Handle, resolvedTargetType) - ?? NotFoundFallback (ref reference, targetType, resolvedTargetType); - } finally { - createPeerReference = previousCreatePeerReference; - } + return TrimmableTypeMap.Instance.CreateInstance (reference.Handle, resolvedTargetType) + ?? NotFoundFallback (ref reference, targetType, resolvedTargetType); } finally { JniObjectReference.Dispose (ref reference, transfer); } From ce0c264686077c8b65cc2868cd03f47c74eee1d5 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 21 Aug 2026 12:39:48 +0200 Subject: [PATCH 7/8] Canonicalize created peers instead of locking Replace the value-manager-wide peer creation lock with post-creation canonicalization: GetPeer() creates as before, then re-reads the registry and returns whichever peer won registration, disposing the loser. The lock was held across CreatePeer(), which runs activation constructors and Java class loading, so it could invert against a Java monitor held by another thread that was itself waiting to create a peer. It also serialized every peer creation process-wide, in shared code used by all runtime configurations, while still leaving GetValue() and JavaAs() free to create peers without it. Registration was already the arbiter of which peer is canonical; the only missing piece was that GetPeer() returned the peer it created rather than the registered one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../JniRuntime.JniValueManager.cs | 26 ++++++++++++------- .../JniRuntime.JniValueManagerTests.cs | 14 ++++++++-- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 124b3062720..8dcc2b06479 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -5,7 +5,6 @@ using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; -using System.Threading; namespace Java.Interop { @@ -46,7 +45,6 @@ public abstract partial class JniValueManager : ISetRuntime, IDisposable { internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; JniRuntime? runtime; - readonly Lock peerCreationLock = new (); bool disposed; public JniRuntime Runtime { get => runtime ?? throw new NotSupportedException (); @@ -207,16 +205,24 @@ protected virtual bool TryUnboxPeerObject (IJavaPeerable value, [NotNullWhen (tr return existing; } - lock (peerCreationLock) { - // CreatePeer registers the new peer before returning. Check again while - // holding the lock so only one caller creates a peer for this reference. - existing = PeekPeer (reference); - if (IsCompatiblePeer (existing, targetType)) { - return existing; + var created = CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); + + // Creating a peer registers it, and registration -- not creation -- decides + // which peer is canonical for a given Java instance. Concurrent callers can + // therefore each create a peer while only one of them wins registration, so + // always hand back the registered winner and discard the loser. Serializing + // creation instead is not an option: CreatePeer() runs activation + // constructors and Java class loading, so a lock held across it would invert + // against Java monitors. + var registered = PeekPeer (reference); + if (!ReferenceEquals (created, registered) && IsCompatiblePeer (registered, targetType)) { + if (created != null) { + DisposePeerUnlessReferenced (created); } - - return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType); + return registered; } + + return created; } static bool IsCompatiblePeer (IJavaPeerable? peer, Type? targetType) diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs index b5ff337c394..2a60bded9dd 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRuntime.JniValueManagerTests.cs @@ -45,7 +45,11 @@ public void GetPeer_ReturnsRegisteredPeerConcurrently () var second = Task.Run (() => vm.GetPeer (source.PeerReference)); Task.WaitAll (first, second); - Assert.AreEqual (1, vm.CreatePeerCount); + // Both callers miss the cache and create a peer -- creation is deliberately + // not serialized, because CreatePeer() runs activation constructors. Only one + // of them wins registration, and GetPeer() must hand that winner to both. + Assert.AreEqual (2, vm.CreatePeerCount); + Assert.AreSame (vm.RegisteredPeer, first.Result); Assert.AreSame (first.Result, second.Result); } } @@ -66,9 +70,13 @@ public ConcurrentGetPeerValueManager (JniPeerMembers peerMembers) public int CreatePeerCount => createPeerCount; + public IJavaPeerable RegisteredPeer => Volatile.Read (ref registeredPeer); + public override IJavaPeerable PeekPeer (JniObjectReference reference) { var peer = Volatile.Read (ref registeredPeer); + // Hold both callers at their *first* peek so both are guaranteed to miss + // before either one registers a peer. if (Interlocked.Increment (ref peekPeerCount) <= 2 && InitialPeekBarrier != null) { var barrier = InitialPeekBarrier; if (!barrier.SignalAndWait (TimeSpan.FromSeconds (10))) { @@ -85,7 +93,9 @@ public override IJavaPeerable CreatePeer ( Type targetType) { Interlocked.Increment (ref createPeerCount); - var peer = new TestPeer (reference, peerMembers); + // Each peer owns its own reference, so discarding the loser cannot + // invalidate the winner or the source object. + var peer = new TestPeer (reference.NewGlobalRef (), peerMembers); Interlocked.CompareExchange (ref registeredPeer, peer, null); return peer; } From afe150ffc0ac061bcf3063095da984607de96866 Mon Sep 17 00:00:00 2001 From: Simon Rozsival Date: Fri, 21 Aug 2026 12:40:01 +0200 Subject: [PATCH 8/8] Mark implicitly created peers replaceable before activation Generated JavaPeerProxy.CreateInstance() overrides now allocate the peer with RuntimeHelpers.GetUninitializedObject(), mark it Replaceable, and then invoke the activation constructor, instead of using newobj. TrimmableTypeMap's reflection fallback for closed generic targets does the same. The activation constructor is what registers the peer, so a peer that only becomes Replaceable afterwards can be evicted by the next implicitly created peer -- letting two threads that wrap the same Java instance each keep a different wrapper. This replaces the previous attempt, which passed JniHandleOwnership.DoNotRegister so registration could happen explicitly after construction. That flag never reached ConstructPeerCore() for two families of peers: Java.Lang.Throwable's SetHandle() hardcodes JniObjectReferenceOptions.Copy, and so do both generated Java.Interop-style activation paths. It also left peers unregistered while their constructors ran. Pre-marking mirrors TypeManager.CreateProxy() in the non-trimmable typemap implementation, which already establishes this invariant the same way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Generator/TypeMapAssemblyEmitter.cs | 84 +++++++++++------ .../Java.Interop/JavaPeerProxy.cs | 24 +++++ .../TrimmableTypeMap.cs | 24 ++--- .../PublicAPI/API-37/PublicAPI.Unshipped.txt | 1 + .../TypeMapAssemblyGeneratorTests.cs | 89 ++++++++++++++++++- 5 files changed, 184 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs b/src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs index 17865148a4e..4ef713e2172 100644 --- a/src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs +++ b/src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs @@ -108,6 +108,7 @@ sealed class TypeMapAssemblyEmitter MemberReferenceHandle _getActivationPeerRef; MemberReferenceHandle _setActivationPeerReferenceRef; MemberReferenceHandle _markActivationPeerReplaceableRef; + MemberReferenceHandle _markCreatedPeerReplaceableRef; MemberReferenceHandle _waitForBridgeProcessingRef; MemberReferenceHandle _androidEnvironmentUnhandledExceptionRef; MemberReferenceHandle _ucoAttrCtorRef; @@ -432,6 +433,11 @@ void EmitMemberReferences () rt => rt.Void (), p => p.AddParameter ().Type ().IntPtr ())); + _markCreatedPeerReplaceableRef = _pe.AddMemberRef (_javaPeerProxyRef, "MarkCreatedPeerReplaceable", + sig => sig.MethodSignature ().Parameters (1, + rt => rt.Void (), + p => p.AddParameter ().Type ().Type (_iJavaPeerableRef, false))); + _waitForBridgeProcessingRef = _pe.AddMemberRef (_androidRuntimeInternalRef, "WaitForBridgeProcessing", sig => sig.MethodSignature ().Parameters (0, rt => rt.Void (), p => { })); @@ -800,25 +806,55 @@ void EmitCreateInstanceGenericDefinition () }); } + /// + /// Emits CreateInstance for XA-style activation (leaf type): + /// var obj = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType)); + /// JavaPeerProxy.MarkCreatedPeerReplaceable(obj); + /// obj.Ctor(handle, ownership); + /// return obj; + /// + /// + /// The peer is allocated uninitialized and marked replaceable before the activation + /// constructor runs, because that constructor is what registers the peer. See + /// JavaPeerProxy.MarkCreatedPeerReplaceable(). This mirrors + /// TypeManager.CreateProxy() in the non-trimmable typemap implementation. + /// void EmitCreateInstanceViaNewobj (EntityHandle typeRef) { var ctorRef = AddActivationCtorRef (typeRef); EmitCreateInstanceBody (encoder => { + EmitUninitializedReplaceablePeer (encoder, typeRef); + + encoder.OpCode (ILOpCode.Dup); encoder.OpCode (ILOpCode.Ldarg_1); encoder.OpCode (ILOpCode.Ldarg_2); - encoder.NewObject (ctorRef, parameterCount: 2); + encoder.Call (ctorRef, parameterCount: 2, isInstance: true); + encoder.Return (returnsValue: true); }); } + /// + /// Emits, leaving the new instance on the stack: + /// var obj = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType)); + /// JavaPeerProxy.MarkCreatedPeerReplaceable(obj); + /// + void EmitUninitializedReplaceablePeer (PEAssemblyBuilder.TrackedInstructionEncoder encoder, EntityHandle typeRef) + { + encoder.LoadToken (typeRef); + encoder.Call (_getTypeFromHandleRef, parameterCount: 1, returnsValue: true); + encoder.Call (_getUninitializedObjectRef, parameterCount: 1, returnsValue: true); + encoder.CastClass (typeRef); + + encoder.OpCode (ILOpCode.Dup); + encoder.Call (_markCreatedPeerReplaceableRef, parameterCount: 1); + } + void EmitCreateInstanceInheritedCtor (EntityHandle targetTypeRef, ActivationCtorData activationCtor) { var baseActivationCtorRef = AddActivationCtorRef (_pe.ResolveTypeRef (activationCtor.DeclaringType)); EmitCreateInstanceBody (encoder => { - encoder.LoadToken (targetTypeRef); - encoder.Call (_getTypeFromHandleRef, parameterCount: 1, returnsValue: true); - encoder.Call (_getUninitializedObjectRef, parameterCount: 1, returnsValue: true); - encoder.CastClass (targetTypeRef); + EmitUninitializedReplaceablePeer (encoder, targetTypeRef); encoder.OpCode (ILOpCode.Dup); encoder.OpCode (ILOpCode.Ldarg_1); @@ -831,16 +867,22 @@ void EmitCreateInstanceInheritedCtor (EntityHandle targetTypeRef, ActivationCtor /// /// Emits CreateInstance for JavaInterop-style activation (leaf type): + /// var result = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType)); + /// JavaPeerProxy.MarkCreatedPeerReplaceable(result); /// var jniRef = new JniObjectReference(handle); - /// var result = new TargetType(ref jniRef, JniObjectReferenceOptions.Copy); + /// result.Ctor(ref jniRef, JniObjectReferenceOptions.Copy); /// JNIEnv.DeleteRef(handle, ownership); /// return result; /// + /// + /// See for why the instance is allocated + /// uninitialized and marked replaceable before its activation constructor runs. + /// void EmitCreateInstanceViaJavaInteropNewobj (EntityHandle typeRef) { var ctorRef = AddJavaInteropActivationCtorRef (typeRef); EmitCreateInstanceBodyWithLocals ( - EncodeJniObjectReferenceAndObjectLocals, + EncodeJniObjectReferenceLocal, encoder => { // var jniRef = new JniObjectReference(handle, JniObjectReferenceType.Invalid); encoder.LoadLocalAddress (0); @@ -848,18 +890,20 @@ void EmitCreateInstanceViaJavaInteropNewobj (EntityHandle typeRef) encoder.LoadConstantI4 (0); // JniObjectReferenceType.Invalid encoder.Call (_jniObjectReferenceCtorRef, parameterCount: 2, isInstance: true); - // var result = new TargetType(ref jniRef, JniObjectReferenceOptions.Copy); + EmitUninitializedReplaceablePeer (encoder, typeRef); + + // result.Ctor(ref jniRef, JniObjectReferenceOptions.Copy); + // The result stays on the stack across JNIEnv.DeleteRef() below. + encoder.OpCode (ILOpCode.Dup); encoder.LoadLocalAddress (0); encoder.LoadConstantI4 (1); // JniObjectReferenceOptions.Copy - encoder.NewObject (ctorRef, parameterCount: 2); - encoder.StoreLocal (1); // save result + encoder.Call (ctorRef, parameterCount: 2, isInstance: true); // JNIEnv.DeleteRef(handle, ownership); encoder.OpCode (ILOpCode.Ldarg_1); // handle encoder.OpCode (ILOpCode.Ldarg_2); // ownership encoder.Call (_jniEnvDeleteRefRef, parameterCount: 2); - encoder.LoadLocal (1); // load result encoder.Return (returnsValue: true); }); } @@ -879,10 +923,8 @@ void EmitCreateInstanceInheritedJavaInteropCtor (EntityHandle targetTypeRef, Act EncodeJniObjectReferenceLocal, encoder => { // var obj = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType)); - encoder.LoadToken (targetTypeRef); - encoder.Call (_getTypeFromHandleRef, parameterCount: 1, returnsValue: true); - encoder.Call (_getUninitializedObjectRef, parameterCount: 1, returnsValue: true); - encoder.CastClass (targetTypeRef); + // JavaPeerProxy.MarkCreatedPeerReplaceable(obj); + EmitUninitializedReplaceablePeer (encoder, targetTypeRef); // dup obj (one copy for the call, one for the return) encoder.OpCode (ILOpCode.Dup); @@ -916,18 +958,6 @@ void EncodeJniObjectReferenceLocal (BlobBuilder blob) blob.WriteCompressedInteger (CodedIndex.TypeDefOrRefOrSpec (_jniObjectReferenceRef)); } - void EncodeJniObjectReferenceAndObjectLocals (BlobBuilder blob) - { - // LOCAL_SIG header (0x07), count = 2: - // local 0: JniObjectReference (valuetype) - // local 1: object (for storing the newobj result across the DeleteRef call) - blob.WriteByte ((byte) SignatureKind.LocalVariables); - blob.WriteCompressedInteger (2); // 2 local variables - blob.WriteByte ((byte) SignatureTypeKind.ValueType); - blob.WriteCompressedInteger (CodedIndex.TypeDefOrRefOrSpec (_jniObjectReferenceRef)); - blob.WriteByte ((byte) SignatureTypeCode.Object); - } - MemberReferenceHandle AddJavaInteropActivationCtorRef (EntityHandle declaringTypeRef) { return _pe.AddMemberRef (declaringTypeRef, ".ctor", diff --git a/src/Mono.Android/Java.Interop/JavaPeerProxy.cs b/src/Mono.Android/Java.Interop/JavaPeerProxy.cs index f6d966df235..3465a10dde5 100644 --- a/src/Mono.Android/Java.Interop/JavaPeerProxy.cs +++ b/src/Mono.Android/Java.Interop/JavaPeerProxy.cs @@ -110,6 +110,30 @@ public static void MarkActivationPeerReplaceable (IntPtr jniSelf) peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable); } + /// + /// Marks an implicitly created peer as replaceable, before its activation + /// constructor runs. + /// + /// + /// Generated overrides call this on the + /// uninitialized instance before invoking the activation constructor, because the + /// constructor is what registers the peer and + /// JniRuntime.JniValueManager.AddPeer() arbitrates between an incoming and + /// an already registered peer using . + /// A peer that only becomes replaceable *after* it is registered would let a second + /// implicitly created peer evict it, so two threads wrapping the same Java instance + /// could each end up holding a different wrapper. + /// + /// This mirrors TypeManager.CreateProxy(), which pre-marks the uninitialized + /// instance for the same reason. + /// + public static void MarkCreatedPeerReplaceable (IJavaPeerable peer) + { + ArgumentNullException.ThrowIfNull (peer); + + peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable); + } + static bool IsActivationPeer (IJavaPeerable peer) { var state = peer.JniManagedPeerState; diff --git a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs index 01f57dfe757..9c8761c46b7 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Reflection; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; using Android.Runtime; @@ -19,8 +20,6 @@ namespace Microsoft.Android.Runtime; /// public class TrimmableTypeMap { - const JniHandleOwnership CreatedPeerOwnership = JniHandleOwnership.DoNotTransfer | JniHandleOwnership.DoNotRegister; - static readonly Lock s_initLock = new (); static readonly JavaPeerProxy s_noPeerSentinel = new MissingJavaPeerProxy (); static TrimmableTypeMap? s_instance; @@ -350,21 +349,21 @@ static JniMethodInfo GetClassGetInterfacesMethod () IJavaPeerable? peer; if (ShouldActivateClosedGenericTarget (proxy, targetType)) { - peer = ActivateUsingReflection (targetType, handle, CreatedPeerOwnership); + peer = ActivateUsingReflection (targetType, handle, JniHandleOwnership.DoNotTransfer); } else { - peer = proxy?.CreateInstance (handle, CreatedPeerOwnership); + peer = proxy?.CreateInstance (handle, JniHandleOwnership.DoNotTransfer); } if (peer is not null) { - RegisterCreatedPeer (peer); + MarkCreatedPeer (peer); } return peer; } internal IJavaPeerable? CreateInstanceWithoutReflectionFallback (IntPtr handle, Type? targetType = null) { - var peer = GetProxyForJavaObject (handle, targetType)?.CreateInstance (handle, CreatedPeerOwnership); + var peer = GetProxyForJavaObject (handle, targetType)?.CreateInstance (handle, JniHandleOwnership.DoNotTransfer); if (peer is not null) { - RegisterCreatedPeer (peer); + MarkCreatedPeer (peer); } return peer; } @@ -397,17 +396,22 @@ targetType is not null && return null; } - return (IJavaPeerable) ctor.Invoke ([handle, transfer]); + // Allocate and mark the peer before running the activation constructor, which is + // what registers it; see JavaPeerProxy.MarkCreatedPeerReplaceable(). Generated + // JavaPeerProxy.CreateInstance() overrides do the same thing. + var peer = (IJavaPeerable) RuntimeHelpers.GetUninitializedObject (closedType); + JavaPeerProxy.MarkCreatedPeerReplaceable (peer); + ctor.Invoke (peer, [handle, transfer]); + return peer; } - static void RegisterCreatedPeer (IJavaPeerable peer) + static void MarkCreatedPeer (IJavaPeerable peer) { var peerState = peer.JniManagedPeerState | JniManagedPeerStates.Replaceable; if (global::Java.Interop.Runtime.IsGCUserPeer (peer.PeerReference.Handle)) { peerState |= JniManagedPeerStates.Activatable; } peer.SetJniManagedPeerState (peerState); - JniEnvironment.Runtime.ValueManager.AddPeer (peer); } /// diff --git a/src/Mono.Android/PublicAPI/API-37/PublicAPI.Unshipped.txt b/src/Mono.Android/PublicAPI/API-37/PublicAPI.Unshipped.txt index 7fdf466ee6b..2015895ea3b 100644 --- a/src/Mono.Android/PublicAPI/API-37/PublicAPI.Unshipped.txt +++ b/src/Mono.Android/PublicAPI/API-37/PublicAPI.Unshipped.txt @@ -4375,6 +4375,7 @@ static Android.Widget.PhotoPicker.PhotoPickerSelectionParams.Creator.get -> Andr static Android.Widget.PhotoPicker.PhotoPickerUiCustomizationParams.Creator.get -> Android.OS.IParcelableCreator! static Java.Interop.JavaPeerProxy.GetActivationPeer(nint jniSelf) -> Java.Interop.IJavaPeerable? static Java.Interop.JavaPeerProxy.MarkActivationPeerReplaceable(nint jniSelf) -> void +static Java.Interop.JavaPeerProxy.MarkCreatedPeerReplaceable(Java.Interop.IJavaPeerable! peer) -> void static Java.Interop.JavaPeerProxy.SetActivationPeerReference(Java.Interop.IJavaPeerable! peer, nint jniSelf) -> void static Java.Interop.JavaPeerProxy.ShouldSkipActivation(nint jniSelf) -> bool static Java.Lang.Character.UnicodeBlock.CjkUnifiedIdeographsExtensionI.get -> Java.Lang.Character.UnicodeBlock? diff --git a/tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs b/tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs index d448de126f5..506918dfb66 100644 --- a/tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs +++ b/tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs @@ -425,6 +425,38 @@ public void Generate_LeafCtor_DoesNotUseCreateManagedPeer () Assert.True (ctorRefs.Count >= 2, "Should have ctor refs for proxy base + target type"); } + [Fact] + public void Generate_LeafCtor_CreateInstanceMarksPeerReplaceableBeforeActivation () + { + var peers = ScanFixtures (); + // ClickableView has its own (IntPtr, JniHandleOwnership) ctor + var clickableView = peers.First (p => p.JavaName == "my/app/ClickableView"); + + using var stream = GenerateAssembly (new [] { clickableView }, "LeafCtorPreMarkTest"); + using var pe = new PEReader (stream); + var reader = pe.GetMetadataReader (); + + AssertCreateInstancePreMarksPeer (pe, reader, "MyApp_ClickableView_Proxy", "ClickableView"); + } + + [Fact] + public void Generate_LeafJavaInteropCtor_CreateInstanceMarksPeerReplaceableBeforeActivation () + { + var peer = MakeAcwPeer ("test/JiLeafTarget", "Test.JiLeafTarget", "TestAsm") with { + ActivationCtor = new ActivationCtorInfo { + DeclaringTypeName = "Test.JiLeafTarget", + DeclaringAssemblyName = "TestAsm", + Style = ActivationCtorStyle.JavaInterop, + }, + }; + + using var stream = GenerateAssembly (new [] { peer }, "LeafJiCtorPreMarkTest"); + using var pe = new PEReader (stream); + var reader = pe.GetMetadataReader (); + + AssertCreateInstancePreMarksPeer (pe, reader, "Test_JiLeafTarget_Proxy", "JiLeafTarget"); + } + [Fact] public void Generate_InheritedCtor_CreateInstanceDoesNotActivate () { @@ -2698,6 +2730,61 @@ static byte[] GetNctorUcoIL (PEReader pe, MetadataReader reader) } static void AssertCreateInstanceReturnsNull (PEReader pe, MetadataReader reader, string proxyTypeName) + { + var ilBytes = GetCreateInstanceIL (pe, reader, proxyTypeName); + Assert.Equal (new [] { (byte) ILOpCode.Ldnull, (byte) ILOpCode.Ret }, ilBytes); + } + + /// + /// Asserts that CreateInstance allocates the peer uninitialized and marks it replaceable + /// *before* invoking the activation constructor, rather than using newobj. + /// The activation constructor is what registers the peer, and + /// JniRuntime.JniValueManager.AddPeer() arbitrates using + /// JniManagedPeerStates.Replaceable: a peer that only becomes replaceable after it + /// is registered can be evicted by a second implicitly created peer, so two threads + /// wrapping the same Java instance could each keep a different wrapper. + /// + static void AssertCreateInstancePreMarksPeer ( + PEReader pe, + MetadataReader reader, + string proxyTypeName, + string targetTypeShortName) + { + var ilBytes = GetCreateInstanceIL (pe, reader, proxyTypeName); + + Assert.True ( + AllMemberRefHandles (reader) + .Where (h => reader.GetString (reader.GetMemberReference (h).Name) == "GetUninitializedObject") + .Any (h => ILContainsCallToken (ilBytes, MetadataTokens.GetToken (h))), + "CreateInstance should allocate the peer via RuntimeHelpers.GetUninitializedObject()"); + + Assert.True ( + AllMemberRefHandles (reader) + .Where (h => reader.GetString (reader.GetMemberReference (h).Name) == "MarkCreatedPeerReplaceable") + .Any (h => ILContainsCallToken (ilBytes, MetadataTokens.GetToken (h))), + "CreateInstance should call JavaPeerProxy.MarkCreatedPeerReplaceable()"); + + var activationCtors = AllMemberRefHandles (reader) + .Where (h => { + var mref = reader.GetMemberReference (h); + if (reader.GetString (mref.Name) != ".ctor" || mref.Parent.Kind != HandleKind.TypeReference) { + return false; + } + var typeRef = reader.GetTypeReference ((TypeReferenceHandle) mref.Parent); + return reader.GetString (typeRef.Name) == targetTypeShortName; + }) + .ToList (); + Assert.NotEmpty (activationCtors); + + Assert.True ( + activationCtors.Any (h => ILContainsCallToken (ilBytes, MetadataTokens.GetToken (h))), + $"CreateInstance should `call` the {targetTypeShortName} activation ctor on the pre-marked instance"); + Assert.All (activationCtors, h => Assert.False ( + ILContainsNewobjToken (ilBytes, MetadataTokens.GetToken (h)), + $"CreateInstance must not `newobj` {targetTypeShortName}: the peer would register before it is marked replaceable")); + } + + static byte[] GetCreateInstanceIL (PEReader pe, MetadataReader reader, string proxyTypeName) { var proxyTypeHandle = reader.TypeDefinitions.First (h => { var type = reader.GetTypeDefinition (h); @@ -2712,7 +2799,7 @@ static void AssertCreateInstanceReturnsNull (PEReader pe, MetadataReader reader, Assert.NotNull (body); var ilBytes = body.GetILBytes (); Assert.NotNull (ilBytes); - Assert.Equal (new [] { (byte) ILOpCode.Ldnull, (byte) ILOpCode.Ret }, ilBytes!); + return ilBytes!; } static List AllMemberRefHandles (MetadataReader reader) =>