diff --git a/src/Mono.Android/Java.Lang/Object.cs b/src/Mono.Android/Java.Lang/Object.cs index e065c63d27a..88fe53f5a3c 100644 --- a/src/Mono.Android/Java.Lang/Object.cs +++ b/src/Mono.Android/Java.Lang/Object.cs @@ -166,9 +166,17 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer) if (handle == IntPtr.Zero) return null; - var r = JniEnvironment.Runtime.ValueManager.GetPeer (new JniObjectReference (handle), type); + var peer = PeekObject (handle, type); + if (peer == null) { + var reference = new JniObjectReference (handle); + peer = JniEnvironment.Runtime.ValueManager.CreatePeer (ref reference, JniObjectReferenceOptions.Copy, type); + if (peer != null) { + // Activation can register a competing peer before this one is constructed. + peer = PeekObject (handle, type) ?? peer; + } + } JNIEnv.DeleteRef (handle, transfer); - return r; + return peer; } [EditorBrowsable (EditorBrowsableState.Never)] diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs index ce3707015a5..ebdfbfcb4b3 100644 --- a/tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -589,8 +590,11 @@ public async Task GetObjectArray_DuringConcurrentPeerLookup () Java.InteropTests.TrimmableRuntimeJavaInteropPeer first = null; Java.InteropTests.TrimmableRuntimeJavaInteropPeer second = null; + var createdPeers = new ConcurrentQueue (); using var activationBarrier = new Barrier (2); Java.InteropTests.TrimmableRuntimeJavaInteropPeer.ActivationBarrier = activationBarrier; + // Retain the losing alias too, so its finalizer cannot race the disposal assertion. + Java.InteropTests.TrimmableRuntimeJavaInteropPeer.PeerCreated = createdPeers.Enqueue; try { var firstTask = Task.Factory.StartNew ( () => JNIEnv.GetObjectArray (arrayReference.Handle, new [] { typeof (Java.InteropTests.TrimmableRuntimeJavaInteropPeer) }) [0], @@ -605,10 +609,9 @@ public async Task GetObjectArray_DuringConcurrentPeerLookup () first = firstPeer; second = secondPeer; - // Each GetObjectArray() caller converts through CreatePeer(), which is - // contractually required to return a new peer even when a compatible one is - // already registered, so the caller which loses the race gets an alias. - Assert.AreNotSame (first, second, "Each converting caller should receive its own peer."); + // Both callers create peers, but high-level lookup must return the registered + // peer rather than handing the losing caller an unregistered alias. + Assert.AreSame (first, second, "Both converting callers should receive the registered peer."); Assert.AreEqual (2, Java.InteropTests.TrimmableRuntimeJavaInteropPeer.ConstructorInvocations, "Both callers should have raced through peer activation."); Assert.AreEqual (0, Java.InteropTests.TrimmableRuntimeJavaInteropPeer.DisposeInvocations, @@ -619,13 +622,12 @@ public async Task GetObjectArray_DuringConcurrentPeerLookup () // itself before it was marked Replaceable, so it evicted the first. var registered = Java.Interop.JniRuntime.CurrentRuntime.ValueManager.PeekPeer (reference); Assert.IsNotNull (registered, "One of the racing peers should have won registration."); - Assert.IsTrue (ReferenceEquals (registered, first) || ReferenceEquals (registered, second), - $"The registered peer should be one of the racing peers, but was {registered.GetType ()}."); + Assert.AreSame (registered, first, "Array marshaling should return the peer which won registration."); } finally { Java.InteropTests.TrimmableRuntimeJavaInteropPeer.ActivationBarrier = null; - if (!ReferenceEquals (first, second)) - second?.Dispose (); - first?.Dispose (); + Java.InteropTests.TrimmableRuntimeJavaInteropPeer.PeerCreated = null; + foreach (var peer in createdPeers) + peer.Dispose (); Java.Interop.JniObjectReference.Dispose (ref arrayReference); Java.Interop.JniObjectReference.Dispose (ref reference); } diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/PeerIdentityTests.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/PeerIdentityTests.cs new file mode 100644 index 00000000000..5277804c05a --- /dev/null +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/PeerIdentityTests.cs @@ -0,0 +1,97 @@ +#nullable enable + +using System; +using System.Runtime.CompilerServices; + +using Android.Runtime; + +using Java.Interop; + +using NUnit.Framework; + +namespace Java.InteropTests +{ + [TestFixture] + [NonParallelizable] + [Category ("JNIObjectArray")] + public class PeerIdentityTests + { + [Test] + [Category ("PeerIdentityRepro")] + public void GetObject_ReentrantActivation_PreservesRoundtripIdentity () + { + var manager = JniRuntime.CurrentRuntime.ValueManager; + var handle = JNIEnv.CreateInstance (ReentrantLookupPeer.JniName, "()V"); + ReentrantLookupPeer? cached = null; + ReentrantLookupPeer? reentrant = null; + try { + Assert.IsNull (manager.PeekPeer (new JniObjectReference (handle))); + ReentrantLookupPeer.BeforeRegistration = value => { + reentrant = Java.Lang.Object.GetObject (value, JniHandleOwnership.DoNotTransfer); + }; + + cached = Java.Lang.Object.GetObject (handle, JniHandleOwnership.DoNotTransfer) + ?? throw new InvalidOperationException ("Could not create the cached peer."); + var registered = manager.PeekPeer (new JniObjectReference (handle)) + ?? throw new InvalidOperationException ("No peer was registered."); + Assert.AreSame (reentrant, registered, "The reentrant lookup should have registered first."); + Assert.IsTrue (JniEnvironment.Types.IsSameObject (cached.PeerReference, registered.PeerReference), + "The wrappers should refer to the same Java object."); + + using var array = new Java.Lang.Object ( + JNIEnv.NewArray (new Java.Lang.Object [] { cached }, typeof (Java.Lang.Object)), + JniHandleOwnership.TransferLocalRef); + var values = JNIEnv.GetObjectArray (array.Handle, new [] { typeof (ReentrantLookupPeer) }) + ?? throw new InvalidOperationException ("Could not read the Java object array."); + Assert.AreSame (registered, values [0], "Array marshaling should find the registered peer."); + Assert.AreSame (cached, values [0], + $"Lookup returned managed-{RuntimeHelpers.GetHashCode (cached):x} ({cached.JniManagedPeerState}), " + + $"but the registry retained managed-{RuntimeHelpers.GetHashCode (registered):x} ({registered.JniManagedPeerState}); " + + $"value manager: {manager.GetType ().FullName}."); + } finally { + ReentrantLookupPeer.BeforeRegistration = null; + cached?.Dispose (); + if (!ReferenceEquals (cached, reentrant)) + reentrant?.Dispose (); + JNIEnv.DeleteLocalRef (handle); + } + } + + [Test] + public void GetObject_IncompatibleRegisteredPeer_PreservesRequestedType () + { + var handle = JNIEnv.CreateInstance (ReentrantLookupPeer.JniName, "()V"); + try { + using var registered = new Java.Lang.Object (handle, JniHandleOwnership.DoNotTransfer); + using var typed = Java.Lang.Object.GetObject (handle, JniHandleOwnership.DoNotTransfer); + Assert.IsNotNull (typed); + Assert.AreNotSame (registered, typed); + Assert.AreSame (registered, JniRuntime.CurrentRuntime.ValueManager.PeekPeer (new JniObjectReference (handle)), + "Requesting another managed type should not replace the explicitly registered peer."); + } finally { + JNIEnv.DeleteLocalRef (handle); + } + } + } + + [Register (JniName, DoNotGenerateAcw = true)] + public sealed class ReentrantLookupPeer : Java.Lang.Object + { + public const string JniName = "net/dot/android/test/ReentrantLookupPeer"; + public static Action? BeforeRegistration; + + public ReentrantLookupPeer (IntPtr handle, JniHandleOwnership transfer) + : base (BeforeConstruct (handle), transfer) + { + } + + static IntPtr BeforeConstruct (IntPtr handle) + { + // Complete another lookup after the outer registry miss but before its registration. + var callback = BeforeRegistration; + BeforeRegistration = null; + callback?.Invoke (handle); + return handle; + } + } +} diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs index f93da0ad13d..89888da243e 100644 --- a/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs @@ -404,6 +404,7 @@ sealed class TrimmableRuntimeJavaInteropPeer : Java.Lang.Object public static int DisposeInvocations; public static JniObjectReferenceOptions Options; public static volatile Barrier ActivationBarrier; + public static Action PeerCreated; public TrimmableRuntimeJavaInteropPeer (ref JniObjectReference reference, JniObjectReferenceOptions options) : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) @@ -413,6 +414,7 @@ public TrimmableRuntimeJavaInteropPeer (ref JniObjectReference reference, JniObj Interlocked.Increment (ref ConstructorInvocations); Options = options; Construct (ref reference, options); + PeerCreated?.Invoke (this); var barrier = ActivationBarrier; if (barrier != null && !barrier.SignalAndWait (TimeSpan.FromSeconds (10))) { throw new TimeoutException ("Timed out waiting for concurrent peer activation."); @@ -433,6 +435,7 @@ public static void Reset () DisposeInvocations = 0; Options = JniObjectReferenceOptions.None; ActivationBarrier = null; + PeerCreated = null; } } } diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj b/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj index 8c817a19434..fea590be0d7 100644 --- a/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj @@ -145,6 +145,7 @@ + diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/ReentrantLookupPeer.java b/tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/ReentrantLookupPeer.java new file mode 100644 index 00000000000..e2f0beebf01 --- /dev/null +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/ReentrantLookupPeer.java @@ -0,0 +1,5 @@ +package net.dot.android.test; + +public class ReentrantLookupPeer { + public ReentrantLookupPeer() {} +}