[tests] make GetObjectArray peer mismatch self-diagnosing

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

@@ -6,6 +6,7 @@
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Runtime.InteropServices.Java;
+using System.Threading;
 using Android.Runtime;
 using Java.Interop;
 
@@ -58,6 +59,85 @@ internal static void InitializeIfNeeded ()
 		}
 	}
 
+	/// <summary>
+	/// Diagnostics for dotnet/android#10973. The registry only ever disagrees with a cached
+	/// reference such as <c>Application.Context</c> if a *second* managed peer is created for
+	/// the same Java instance. Record the two places <see cref="AddPeer"/> observes that -- an
+	/// existing entry being replaced, and a second entry being appended because the existing
+	/// one's weak reference was cleared -- so a failing test can report which happened, to
+	/// whom, and how often.
+	///
+	/// Published via <see cref="AppContext"/> so tests in other assemblies can read them
+	/// without needing <c>InternalsVisibleTo</c>. Recording only: behaviour is unchanged.
+	/// </summary>
+	internal const string LiveEvictionsKey = "Microsoft.Android.Runtime.LivePeerEvictions";
+	internal const string ReplacementsKey = "Microsoft.Android.Runtime.PeerReplacements";
+	internal const string DuplicateAppendsKey = "Microsoft.Android.Runtime.PeerDuplicateAppends";
+	internal const string NotReplacedKey = "Microsoft.Android.Runtime.PeerNotReplaced";
+	internal const string RemovedKey = "Microsoft.Android.Runtime.PeerRemoved";
+	internal const string DeadEvictionsKey = "Microsoft.Android.Runtime.DeadPeerEvictions";
+
+	static int liveEvictions;
+	static int deadEvictions;
+	static int replacements;
+	static int duplicateAppends;
+	static int notReplaced;
+	static int removed;
+
+	static string Id (IJavaPeerable peer)
+		=> $"{peer.GetType ().FullName}@managed-0x{RuntimeHelpers.GetHashCode (peer).ToString ("x", CultureInfo.InvariantCulture)}/{peer.PeerReference}";
+
+	static void RecordIfLive (int key, ReferenceTrackingHandle peer)
+	{
+		if (peer.Target is not IJavaPeerable live)
+			return;
+
+		int count = Interlocked.Increment (ref liveEvictions);
+		AppContext.SetData (LiveEvictionsKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)} {Id (live)})");
+		GC.KeepAlive (live);
+	}
+
+	static void RecordReplacement (int key, IJavaPeerable replaced, IJavaPeerable replacement)
+	{
+		int count = Interlocked.Increment (ref replacements);
+		AppContext.SetData (ReplacementsKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)} " +
+				$"replaced {Id (replaced)} with {Id (replacement)})");
+	}
+
+	static void RecordDuplicateAppend (int key, IJavaPeerable value, int existing)
+	{
+		int count = Interlocked.Increment (ref duplicateAppends);
+		AppContext.SetData (DuplicateAppendsKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)} " +
+				$"appended {Id (value)} alongside {existing} existing entr{(existing == 1 ? "y" : "ies")})");
+	}
+
+	// The peer that registers *first* wins, so this also records the registration order:
+	// `kept` was registered before `rejected` ever reached AddPeer().
+	static void RecordNotReplaced (int key, IJavaPeerable kept, IJavaPeerable rejected)
+	{
+		int count = Interlocked.Increment (ref notReplaced);
+		AppContext.SetData (NotReplacedKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)} " +
+				$"kept {Id (kept)}, did not register {Id (rejected)})");
+	}
+
+	static void RecordRemoved (int key, IJavaPeerable value)
+	{
+		int count = Interlocked.Increment (ref removed);
+		AppContext.SetData (RemovedKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)} {Id (value)})");
+	}
+
+	static void RecordDeadEviction (int key)
+	{
+		int count = Interlocked.Increment (ref deadEvictions);
+		AppContext.SetData (DeadEvictionsKey,
+				$"{count} (most recent: key=0x{key.ToString ("x", CultureInfo.InvariantCulture)})");
+	}
+
 	public static void CollectPeers ()
 	{
 		unsafe {
@@ -81,6 +161,8 @@ void Remove (HandleContext* context)
 				for (int i = peers.Count - 1; i >= 0; i--) {
 					var peer = peers [i];
 					if (peer.BelongsToContext (context)) {
+						RecordIfLive (key, peer);
+						RecordDeadEviction (key);
 						peers.RemoveAt (i);
 					}
 				}
@@ -120,16 +202,44 @@ public static void AddPeer (IJavaPeerable value)
 					continue;
 				if (!JniEnvironment.Types.IsSameObject (target.PeerReference, value.PeerReference))
 					continue;
-				if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) {
+				// JNIEnv.NewObject/JNIEnv.CreateInstance() compatibility.
+				// When two MCWs 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 must *not* replace an existing
+				// "replaceable" instance; see dotnet/android#9862. Doing so orphans any
+				// already-cached reference to the existing peer -- notably
+				// `Application.Context`, which caches the peer it first sees and never
+				// re-reads it -- so `PeekPeer()` then hands out a different peer for the
+				// same Java instance; see dotnet/android#10973.
+				//
+				// This mirrors `AndroidRuntime.JavaObjectValueManager`; MonoVM has always
+				// had the second condition, which is why this only ever fails on CoreCLR.
+				//
+				// [0]: If a Java ctor invokes an overridden virtual method, we transition
+				// into managed code w/o a registered instance, and thus create an
+				// "intermediary" via the (IntPtr, JniHandleOwnership) .ctor.
+				if (target.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable) &&
+						!value.JniManagedPeerState.HasFlag (JniManagedPeerStates.Replaceable)) {
+					RecordReplacement (key, target, value);
 					peer.Dispose ();
 					peers [i] = new ReferenceTrackingHandle (value);
 				} else {
-					WarnNotReplacing (key, value, target);
+					RecordNotReplaced (key, target, value);
+					if (JniEnvironment.Runtime.ObjectReferenceManager.LogGlobalReferenceMessages) {
+						// Now a normal startup path rather than a rare one, and the
+						// arguments below are evaluated eagerly -- including two JNI
+						// round-trips for `GetJniTypeNameFromInstance()` -- even though
+						// `WriteGlobalReferenceLine()` discards them when logging is off.
+						WarnNotReplacing (key, value, target);
+					}
 				}
 				GC.KeepAlive (target);
 				return;
 			}
 
+			RecordDuplicateAppend (key, value, peers.Count);
 			peers.Add (new ReferenceTrackingHandle (value));
 		}
 	}
@@ -192,6 +302,7 @@ public static void RemovePeer (IJavaPeerable value)
 				ReferenceTrackingHandle peer = peers [i];
 				IJavaPeerable? target = peer.Target;
 				if (ReferenceEquals (value, target)) {
+					RecordRemoved (key, value);
 					peers.RemoveAt (i);
 					peer.Dispose ();
 				}

tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/ContextPeerWatch.cs

@@ -0,0 +1,58 @@
+using System;
+
+using Android.App;
+
+using Java.Interop;
+
+using NUnit.Framework;
+using NUnit.Framework.Interfaces;
+
+[assembly: Android.RuntimeTests.ContextPeerWatchAttribute]
+
+namespace Android.RuntimeTests {
+
+	// `Application.Context` caches one managed peer for the lifetime of the process, so
+	// `JniValueManager.PeekPeer()` should keep returning that same peer forever.  On CoreCLR it
+	// intermittently stops doing so, and the first test to *notice* is
+	// `JnienvArrayMarshaling.GetObjectArray()` -- which is not necessarily the test that broke
+	// it.  Diagnostics there confirmed the registry was already wrong on entry, so sample after
+	// every test to name the test we were actually running when it diverged.
+	[AttributeUsage (AttributeTargets.Assembly)]
+	public sealed class ContextPeerWatchAttribute : Attribute, ITestAction {
+
+		// Only the *first* divergence is interesting; later tests just observe the same
+		// already-broken registry. Prefixed with "before"/"after" so a divergence that is
+		// already present when the very first test starts -- i.e. one that happened during
+		// app startup -- is distinguishable from one a test body caused.
+		public static string DivergedAfter;
+
+		public ActionTargets Targets => ActionTargets.Test;
+
+		public void BeforeTest (ITest test)
+		{
+			Check (test, "before");
+		}
+
+		public void AfterTest (ITest test)
+		{
+			Check (test, "after");
+		}
+
+		static void Check (ITest test, string when)
+		{
+			if (DivergedAfter != null)
+				return;
+
+			var context = Application.Context;
+			if (context == null || !context.PeerReference.IsValid)
+				return;
+
+			// `PeekPeer()` only reads the registry -- in particular it does not drain the
+			// collected-peer queue the way `GetSurfacedPeers()` does -- so sampling it this
+			// often does not itself perturb the behaviour under investigation.
+			var peeked = JniRuntime.CurrentRuntime.ValueManager.PeekPeer (context.PeerReference);
+			if (!ReferenceEquals (peeked, context))
+				DivergedAfter = $"{when} {test.FullName}";
+		}
+	}
+}

tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs

@@ -1,12 +1,16 @@
 using System;
 using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+using System.Text;
 
 using Android.App;
 using Android.Content;
 using Android.Graphics;
 using Android.Runtime;
 using Android.Views;
 
+using Java.Interop;
+
 using NUnit.Framework;
 
 using Java.LangTests;
@@ -326,25 +330,130 @@ public void SetArrayItem_JavaLangString ()
 		[Category ("JNIObjectArray")]
 		public void GetObjectArray ()
 		{
+			var context = Application.Context;
+			// Sample the registry at each step, so a failure says *when* `Application.Context`
+			// stopped being the registered peer rather than only that it did.
+			var atEntry = ProbeContextPeer (context);
 			using (var byteArray = new Java.Lang.Object (JNIEnv.NewArray (new byte[]{1,2,3}), JniHandleOwnership.TransferLocalRef)) {
 				object[] data = JNIEnv.GetObjectArray (byteArray.Handle, new[]{typeof (byte), typeof (byte), typeof (byte)});
 				AssertArrays ("GetObjectArray", data, (object) 1, (object) 2, (object) 3);
 			}
+			var beforeNewArray = ProbeContextPeer (context);
 			using (var objectArray =
 					new Java.Lang.Object (
 							JNIEnv.NewArray (
-								new Java.Lang.Object[]{Application.Context, 42L, "string"},
+								new Java.Lang.Object[]{context, 42L, "string"},
 								typeof (Java.Lang.Object)),
 						JniHandleOwnership.TransferLocalRef)) {
+				var afterNewArray = ProbeContextPeer (context);
 				object[] values = JNIEnv.GetObjectArray (objectArray.Handle, new[]{typeof(Context), typeof (int)});
 				Assert.AreEqual (3, values.Length);
-				Assert.AreSame (Application.Context, values [0], $"Expected existing Context peer, got {values [0]?.GetType ()}.");
+				// Deliberately not `Assert.AreSame()`: this intermittently fails on CoreCLR
+				// (dotnet/android#10973), and both peers render identically, so the default
+				// message tells us nothing. Only build the diagnostic when it actually fails.
+				if (!ReferenceEquals (context, values [0]))
+					Assert.Fail (DescribeContextPeerMismatch (context, values [0], atEntry, beforeNewArray, afterNewArray));
 				Assert.IsInstanceOf<int> (values [1], $"Expected converted Int32, got {values [1]?.GetType ()}: {values [1]}.");
 				Assert.AreEqual (42, (int)values [1]);
 				Assert.AreEqual ("string", values [2].ToString ());
 			}
 		}
 
+		// What the registry reports for `Application.Context` at one point in time. Records a
+		// description rather than the peer itself: retaining the peer would keep it alive and
+		// could perturb the very GC behaviour being investigated.
+		readonly struct PeerProbe {
+
+			readonly bool   isExpected;
+			readonly string description;
+
+			public PeerProbe (bool isExpected, string description)
+			{
+				this.isExpected  = isExpected;
+				this.description = description;
+			}
+
+			public override string ToString ()
+				=> $"{description} (is expected: {isExpected})";
+		}
+
+		static PeerProbe ProbeContextPeer (Context expected)
+		{
+			if (!expected.PeerReference.IsValid)
+				return new PeerProbe (false, "<invalid PeerReference>");
+			var peeked = JniRuntime.CurrentRuntime.ValueManager.PeekPeer (expected.PeerReference);
+			return new PeerProbe (ReferenceEquals (peeked, expected), Describe (peeked));
+		}
+
+		// `GetObjectArray()` should hand back the *same* managed peer that `Application.Context`
+		// holds, because `JniValueManager.GetPeer()` peeks the registry before creating a new peer.
+		// When that fails, the Java instance is the same but the managed peers differ, so dump
+		// enough of the registry to tell *why* they disagree: whether the entry was replaced by a
+		// second `AddPeer()`, evicted entirely, or its weak reference was cleared.
+		static string DescribeContextPeerMismatch (Context expected, object actual, PeerProbe atEntry, PeerProbe beforeNewArray, PeerProbe afterNewArray)
+		{
+			var sb = new StringBuilder ();
+			sb.AppendLine ("Expected `Application.Context` and `GetObjectArray ()[0]` to be the same managed peer.");
+			AppendPeer (sb, "expected (Application.Context)", expected);
+			AppendPeer (sb, "actual   (GetObjectArray ()[0])", actual);
+
+			// The registry over time. If the peer was already wrong `atEntry`, something evicted
+			// or replaced it *before* this test ran and the marshaling is only the messenger.
+			sb.AppendLine ("  PeekPeer (Application.Context) over time:");
+			sb.AppendLine ($"    at test entry:      {atEntry}");
+			sb.AppendLine ($"    before NewArray:    {beforeNewArray}");
+			sb.AppendLine ($"    after NewArray:     {afterNewArray}");
+			sb.AppendLine ($"  Registry first diverged: {ContextPeerWatchAttribute.DivergedAfter ?? "<not observed>"}");
+			// Set by JavaMarshalRegisteredPeers: the registry can only disagree with a cached
+			// `Application.Context` if a second managed peer was created for the same Java
+			// instance, so report how that happened.
+			sb.AppendLine ($"  CollectPeers() evicted live peers:  {AppContext.GetData ("Microsoft.Android.Runtime.LivePeerEvictions") ?? "<none>"}");
+			sb.AppendLine ($"  CollectPeers() evictions (total):   {AppContext.GetData ("Microsoft.Android.Runtime.DeadPeerEvictions") ?? "<none>"}");
+			sb.AppendLine ($"  AddPeer() replaced registrations:   {AppContext.GetData ("Microsoft.Android.Runtime.PeerReplacements") ?? "<none>"}");
+			sb.AppendLine ($"  AddPeer() appended duplicates:      {AppContext.GetData ("Microsoft.Android.Runtime.PeerDuplicateAppends") ?? "<none>"}");
+			// Which peer registered *first* -- the kept one -- and which was turned away.
+			sb.AppendLine ($"  AddPeer() declined to replace:      {AppContext.GetData ("Microsoft.Android.Runtime.PeerNotReplaced") ?? "<none>"}");
+			sb.AppendLine ($"  RemovePeer() removals:              {AppContext.GetData ("Microsoft.Android.Runtime.PeerRemoved") ?? "<none>"}");
+
+			// Did `Application.Context` itself change after we captured it?
+			sb.AppendLine ($"  Application.Context still == expected: {ReferenceEquals (Application.Context, expected)}");
+
+			if (actual is IJavaPeerable actualPeer && expected.PeerReference.IsValid && actualPeer.PeerReference.IsValid)
+				sb.AppendLine ($"  IsSameObject (expected, actual): {JniEnvironment.Types.IsSameObject (expected.PeerReference, actualPeer.PeerReference)}");
+
+			var manager = JniRuntime.CurrentRuntime.ValueManager;
+
+			// If this returns `actual`, the registry entry was replaced out from under
+			// `Application.Context`; if it returns null, the entry was evicted or collected.
+			var peeked = expected.PeerReference.IsValid ? manager.PeekPeer (expected.PeerReference) : null;
+			sb.AppendLine ($"  PeekPeer (expected.PeerReference) => {Describe (peeked)}");
+			sb.AppendLine ($"    is expected: {ReferenceEquals (peeked, expected)}; is actual: {ReferenceEquals (peeked, actual)}");
+
+			sb.AppendLine ($"  Surfaced peers with JniIdentityHashCode=0x{expected.JniIdentityHashCode:x}:");
+			foreach (var info in manager.GetSurfacedPeers ()) {
+				if (info.JniIdentityHashCode != expected.JniIdentityHashCode)
+					continue;
+				info.SurfacedPeer.TryGetTarget (out var target);
+				sb.AppendLine ($"    {Describe (target)} (is expected: {ReferenceEquals (target, expected)}; is actual: {ReferenceEquals (target, actual)})");
+			}
+			return sb.ToString ();
+		}
+
+		static void AppendPeer (StringBuilder sb, string label, object value)
+		{
+			sb.AppendLine ($"  {label}: {Describe (value)}");
+			if (value is not IJavaPeerable peer)
+				return;
+			sb.AppendLine ($"    JniIdentityHashCode=0x{peer.JniIdentityHashCode:x} PeerReference={peer.PeerReference} JniManagedPeerState={peer.JniManagedPeerState}");
+		}
+
+		// The Java-side `toString()` is identical for every peer over the same instance, so
+		// identify peers by their *managed* hash code instead.
+		static string Describe (object value)
+			=> value == null
+				? "<null>"
+				: $"{value.GetType ().FullName}@managed-0x{RuntimeHelpers.GetHashCode (value):x}";
+
 		[Test]
 		public void NewArray_Int32ArrayArray ()
 		{

tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj

@@ -129,6 +129,7 @@
     <Compile Include="Android.OS\BundleTest.cs" />
     <Compile Include="Android.OS\HandlerTest.cs" />
     <Compile Include="Android.Runtime\CharSequenceTest.cs" />
+    <Compile Include="Android.Runtime\ContextPeerWatch.cs" />
     <Compile Include="Android.Runtime\InputStreamInvokerTest.cs" />
     <Compile Include="Android.Runtime\JavaCollectionTest.cs" />
     <Compile Include="Android.Runtime\JnienvArrayMarshaling.cs" />