Fix concurrent creation of duplicate Java peers

external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs

@@ -200,15 +200,34 @@ 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;
+ var existing = PeekPeer (reference);
+ if (IsCompatiblePeer (existing, targetType)) {
+ return existing;
  }
- return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType);
+
+ 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 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,

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,132 @@ 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);
+
+ // 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);
+ }
+ }
+
+ 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 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))) {
+ 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);
+ // 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;
+ }
+
+ }
+
+ 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 {

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 ()
  });
 	}
 
+	/// <summary>
+	/// Emits CreateInstance for XA-style activation (leaf type):
+	/// var obj = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType));
+	/// JavaPeerProxy.MarkCreatedPeerReplaceable(obj);
+	/// obj.Ctor(handle, ownership);
+	/// return obj;
+	/// </summary>
+	/// <remarks>
+	/// The peer is allocated uninitialized and marked replaceable before the activation
+	/// constructor runs, because that constructor is what registers the peer. See
+	/// <c>JavaPeerProxy.MarkCreatedPeerReplaceable()</c>. This mirrors
+	/// <c>TypeManager.CreateProxy()</c> in the non-trimmable typemap implementation.
+	/// </remarks>
 	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);
  });
 	}
 
+	/// <summary>
+	/// Emits, leaving the new instance on the stack:
+	/// var obj = (TargetType)RuntimeHelpers.GetUninitializedObject(typeof(TargetType));
+	/// JavaPeerProxy.MarkCreatedPeerReplaceable(obj);
+	/// </summary>
+	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,35 +867,43 @@ void EmitCreateInstanceInheritedCtor (EntityHandle targetTypeRef, ActivationCtor
 
 	/// <summary>
 	/// 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;
 	/// </summary>
+	/// <remarks>
+	/// See <see cref="EmitCreateInstanceViaNewobj"/> for why the instance is allocated
+	/// uninitialized and marked replaceable before its activation constructor runs.
+	/// </remarks>
 	void EmitCreateInstanceViaJavaInteropNewobj (EntityHandle typeRef)
 	{
  var ctorRef = AddJavaInteropActivationCtorRef (typeRef);
  EmitCreateInstanceBodyWithLocals (
-EncodeJniObjectReferenceAndObjectLocals,
+EncodeJniObjectReferenceLocal,
  encoder => {
  // var jniRef = new JniObjectReference(handle, JniObjectReferenceType.Invalid);
  encoder.LoadLocalAddress (0);
  encoder.OpCode (ILOpCode.Ldarg_1); // handle
  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",

src/Mono.Android/Java.Interop/JavaPeerProxy.cs

@@ -110,6 +110,30 @@ public static void MarkActivationPeerReplaceable (IntPtr jniSelf)
  peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable);
  }
 
+ /// <summary>
+ /// Marks an implicitly created peer as replaceable, before its activation
+ /// constructor runs.
+ /// </summary>
+ /// <remarks>
+ /// Generated <see cref="CreateInstance"/> overrides call this on the
+ /// uninitialized instance before invoking the activation constructor, because the
+ /// constructor is what registers the peer and
+ /// <c>JniRuntime.JniValueManager.AddPeer()</c> arbitrates between an incoming and
+ /// an already registered peer using <see cref="JniManagedPeerStates.Replaceable"/>.
+ /// 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 <c>TypeManager.CreateProxy()</c>, which pre-marks the uninitialized
+ /// instance for the same reason.
+ /// </remarks>
+ public static void MarkCreatedPeerReplaceable (IJavaPeerable peer)
+ {
+ ArgumentNullException.ThrowIfNull (peer);
+
+ peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Replaceable);
+ }
+
  static bool IsActivationPeer (IJavaPeerable peer)
  {
  var state = peer.JniManagedPeerState;

src/Mono.Android/Microsoft.Android.Runtime/JavaMarshalRegisteredPeers.cs

@@ -136,7 +136,9 @@ public static void AddPeer (IJavaPeerable value)
  !value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) {
  peer.Dispose ();
  peers [i] = new ReferenceTrackingHandle (value);
- } else if (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);