[Java.Interop] Add JniRuntime.JniValueManager.GetPeer() (#1295)

Context: https://github.com/dotnet/android/pull/9630/files#r1891090085
Context: dotnet/android#9716

As part of NativeAOT prototyping within dotnet/android, we need to
update [`Java.Lang.Object.GetObject<T>()`][0] so that it uses
`Java.Interop.JniRuntime.JniValueManager` APIs instead of its own
`TypeManager.CreateInstance()` invocation, as
`TypeManager.CreateInstance()` hits P/Invokes which don't currently
work in the NativeAOT sample environment.

However, update it to *what*?  The obvious thing to do would be to
use `JniRuntime.JniValueManager.GetValue()`:

	partial class Object {
	    internal static IJavaPeerable? GetObject (IntPtr handle, JniHandleOwnership transfer, Type? type = null)
	    {
	        var r = PeekObject (handle, type);
	        if (r != null) {
	            JNIEnv.DeleteRef (handle, transfer);
	            return r;
	        }
	        var reference = new JniObjectReference (handle);
	        r = (IJavaPeerable) JNIEnvInit.ValueManager.GetValue (
	                ref reference,
	                JniObjectReferenceOptions.Copy,
	                type);
	        JNIEnv.DeleteRef (handle, transfer);
	        return r;
	    }
	}

The problem is that this blows up good:

	<System.InvalidCastException: Arg_InvalidCastException
	   at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	   at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
	   at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
	   at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
	   at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()>

because somewhere in that stack trace we have a `java.lang.Integer`
instance, and `.GetValue(Integer_ref, …)` returns a `System.Int32`
containing the underling value, *not* an `IJavaPeerable` value for
the `java.lang.Integer` instance.  Consider:

	var i_class = new JniType ("java/lang/Integer");
	var i_ctor  = i_class.GetConstructor ("(I)V");
	JniArgumentValue* i_args = stackalloc JniArgumentValue [1];
	i_args [0]  = new JniArgumentValue (42);
	var i_value = i_class.NewObject (i_ctor, i_args);
	var v       = JniEnvironment.Runtime.ValueManager.GetValue (ref i_value, JniObjectReferenceOptions.CopyAndDispose, null);
	Console.WriteLine ($"v? {v} {v?.GetType ()}");

which prints `v? 42 System.Int32`.

This was expected and desirable, until we try to use `GetValue()` for
`Object.GetObject<T>()`; the semantics don't match.

Add a new `JniRuntime.JniValueManager.GetPeer()` method, which better
matches the semantics that `Object.GetObject<T>()` requires, allowing:

	partial class Object {
	    internal static IJavaPeerable? GetObject (IntPtr handle, JniHandleOwnership transfer, Type? type = null)
	    {
	        var r = JNIEnvInit.ValueManager.GetPeer (new JniObjectReference (handle));
	        JNIEnv.DeleteRef (handle, transfer);
	        return r;
	    }
	}

Finally, add a new `JniRuntimeJniValueManagerContract` unit test,
so that we have "more formalized" semantic requirements on
`JniRuntime.JniValueManager` implementations.

[0]: https://github.com/dotnet/android/blob/cc35a263e046444c2123e7a7dba106b18e2bbebb/src/Mono.Android/Java.Lang/Object.cs#L133-L166

Author: jonpryor

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

@@ -32,7 +32,7 @@ partial class CreationOptions {
 			public  JniValueManager?        ValueManager                {get; set;}
 		}
 
-		JniValueManager?                    valueManager;
+		internal    JniValueManager?        valueManager;
 		public  JniValueManager             ValueManager                {
 			get => valueManager ?? throw new NotSupportedException ();
 		}
@@ -271,6 +271,28 @@ static Type GetPeerType ([DynamicallyAccessedMembers (Constructors)] Type type)
 				return type;
 			}
 
+			public IJavaPeerable? GetPeer (
+					JniObjectReference reference,
+					[DynamicallyAccessedMembers (Constructors)]
+					Type? targetType = null)
+			{
+				if (disposed) {
+					throw new ObjectDisposedException (GetType ().Name);
+				}
+
+				if (!reference.IsValid) {
+					return null;
+				}
+
+				var peeked  = PeekPeer (reference);
+				if (peeked != null &&
+						(targetType == null ||
+						 targetType.IsAssignableFrom (peeked.GetType ()))) {
+					return peeked;
+				}
+				return CreatePeer (ref reference, JniObjectReferenceOptions.Copy, targetType);
+			}
+
 			public virtual IJavaPeerable? CreatePeer (
 					ref JniObjectReference reference,
 					JniObjectReferenceOptions transfer,

src/Java.Interop/PublicAPI.Unshipped.txt

@@ -5,5 +5,6 @@ virtual Java.Interop.JniRuntime.OnEnterMarshalMethod() -> void
 virtual Java.Interop.JniRuntime.OnUserUnhandledException(ref Java.Interop.JniTransition transition, System.Exception! e) -> void
 Java.Interop.JavaException.JavaException(ref Java.Interop.JniObjectReference reference, Java.Interop.JniObjectReferenceOptions transfer, Java.Interop.JniObjectReference throwableOverride) -> void
 Java.Interop.JavaException.SetJavaStackTrace(Java.Interop.JniObjectReference peerReferenceOverride = default(Java.Interop.JniObjectReference)) -> void
+Java.Interop.JniRuntime.JniValueManager.GetPeer(Java.Interop.JniObjectReference reference, System.Type? targetType = null) -> Java.Interop.IJavaPeerable?
 Java.Interop.JniTypeSignatureAttribute.InvokerType.get -> System.Type?
 Java.Interop.JniTypeSignatureAttribute.InvokerType.set -> void

tests/Java.Interop-Tests/Java.Interop-Tests.csproj

@@ -3,6 +3,8 @@
   <PropertyGroup>
     <TargetFramework>$(DotNetTargetFramework)</TargetFramework>
     <IsPackable>false</IsPackable>
+    <SignAssembly>true</SignAssembly>
+    <AssemblyOriginatorKeyFile>..\..\product.snk</AssemblyOriginatorKeyFile>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <DefineConstants>$(DefineConstants);NO_MARSHAL_MEMBER_BUILDER_SUPPORT;NO_GC_BRIDGE_SUPPORT</DefineConstants>
   </PropertyGroup>

tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs

@@ -0,0 +1,297 @@
+#nullable enable
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+using System.Threading;
+
+using Java.Interop;
+
+using NUnit.Framework;
+
+namespace Java.InteropTests {
+
+	// Android doesn't support `[NonParallelizable]`, but runs tests sequentially by default.
+#if !__ANDROID__
+	// Modifies JniRuntime.valueManager instance field; can't be done in parallel
+	[NonParallelizable]
+#endif  // !__ANDROID__
+	public abstract class JniRuntimeJniValueManagerContract : JavaVMFixture {
+
+		protected abstract Type ValueManagerType {
+			get;
+		}
+
+		protected virtual JniRuntime.JniValueManager CreateValueManager ()
+		{
+			var manager = Activator.CreateInstance (ValueManagerType) as JniRuntime.JniValueManager;
+			return manager ?? throw new InvalidOperationException ($"Could not create instance of `{ValueManagerType}`!");
+		}
+
+#pragma warning disable CS8618
+		JniRuntime.JniValueManager  systemManager;
+		JniRuntime.JniValueManager  valueManager;
+#pragma warning restore CS8618
+
+		[SetUp]
+		public void CreateVM ()
+		{
+			systemManager   = JniRuntime.CurrentRuntime.valueManager!;
+			valueManager    = CreateValueManager ();
+			valueManager.OnSetRuntime (JniRuntime.CurrentRuntime);
+			JniRuntime.CurrentRuntime.valueManager  = valueManager;
+		}
+
+		[TearDown]
+		public void DestroyVM ()
+		{
+			JniRuntime.CurrentRuntime.valueManager  = systemManager;
+			systemManager   = null!;
+			valueManager?.Dispose ();
+			valueManager    = null!;
+		}
+
+		[Test]
+		public void AddPeer ()
+		{
+		}
+
+		int GetSurfacedPeersCount ()
+		{
+			return valueManager.GetSurfacedPeers ().Count;
+		}
+
+		[Test]
+		public void AddPeer_NoDuplicates ()
+		{
+			int startPeerCount  = GetSurfacedPeersCount ();
+			using (var v = new MyDisposableObject ()) {
+				// MyDisposableObject ctor implicitly calls AddPeer();
+				Assert.AreEqual (startPeerCount + 1, GetSurfacedPeersCount (), DumpPeers ());
+				valueManager.AddPeer (v);
+				Assert.AreEqual (startPeerCount + 1, GetSurfacedPeersCount (), DumpPeers ());
+			}
+		}
+
+		[Test]
+		public void ConstructPeer_ImplicitViaBindingConstructor_PeerIsInSurfacedPeers ()
+		{
+			int startPeerCount  = GetSurfacedPeersCount ();
+
+			var g               = new GetThis ();
+			var surfaced        = valueManager.GetSurfacedPeers ();
+			Assert.AreEqual (startPeerCount + 1, surfaced.Count);
+
+			var found           = false;
+			foreach (var pr in surfaced) {
+				if (!pr.SurfacedPeer.TryGetTarget (out var p))
+					continue;
+				if (object.ReferenceEquals (g, p)) {
+					found = true;
+				}
+			}
+			Assert.IsTrue (found);
+
+			var localRef        = g.PeerReference.NewLocalRef ();
+			g.Dispose ();
+			Assert.AreEqual (startPeerCount, GetSurfacedPeersCount ());
+			Assert.IsNull (valueManager.PeekPeer (localRef));
+			JniObjectReference.Dispose (ref localRef);
+		}
+
+		[Test]
+		public void ConstructPeer_ImplicitViaBindingMethod_PeerIsInSurfacedPeers ()
+		{
+			int startPeerCount  = GetSurfacedPeersCount ();
+
+			var g               = new GetThis ();
+			var surfaced        = valueManager.GetSurfacedPeers ();
+			Assert.AreEqual (startPeerCount + 1, surfaced.Count);
+
+			var found           = false;
+			foreach (var pr in surfaced) {
+				if (!pr.SurfacedPeer.TryGetTarget (out var p))
+					continue;
+				if (object.ReferenceEquals (g, p)) {
+					found = true;
+				}
+			}
+			Assert.IsTrue (found);
+
+			var localRef        = g.PeerReference.NewLocalRef ();
+			g.Dispose ();
+			Assert.AreEqual (startPeerCount, GetSurfacedPeersCount ());
+			Assert.IsNull (valueManager.PeekPeer (localRef));
+			JniObjectReference.Dispose (ref localRef);
+		}
+
+
+		[Test]
+		public void CollectPeers ()
+		{
+			// TODO
+		}
+
+		[Test]
+		public void CreateValue ()
+		{
+			using (var o = new JavaObject ()) {
+				var r = o.PeerReference;
+				var x = (IJavaPeerable) valueManager.CreateValue (ref r, JniObjectReferenceOptions.Copy)!;
+				Assert.AreNotSame (o, x);
+				x.Dispose ();
+
+				x = valueManager.CreateValue<IJavaPeerable> (ref r, JniObjectReferenceOptions.Copy);
+				Assert.AreNotSame (o, x);
+				x!.Dispose ();
+			}
+		}
+
+		[Test]
+		public void GetValue_ReturnsAlias ()
+		{
+			var local   = new JavaObject ();
+			local.UnregisterFromRuntime ();
+			Assert.IsNull (valueManager.PeekValue (local.PeerReference));
+			// GetObject must always return a value (unless handle is null, etc.).
+			// However, since we called local.UnregisterFromRuntime(),
+			// JniRuntime.PeekObject() is null (asserted above), but GetObject() must
+			// **still** return _something_.
+			// In this case, it returns an _alias_.
+			// TODO: "most derived type" alias generation. (Not relevant here, but...)
+			var p       = local.PeerReference;
+			var alias   = JniRuntime.CurrentRuntime.ValueManager.GetValue<IJavaPeerable> (ref p, JniObjectReferenceOptions.Copy);
+			Assert.AreNotSame (local, alias);
+			alias!.Dispose ();
+			local.Dispose ();
+		}
+
+		[Test]
+		public void GetValue_ReturnsNullWithNullHandle ()
+		{
+			var r = new JniObjectReference ();
+			var o = valueManager.GetValue (ref r, JniObjectReferenceOptions.Copy);
+			Assert.IsNull (o);
+		}
+
+		[Test]
+		public void GetValue_ReturnsNullWithInvalidSafeHandle ()
+		{
+			var invalid = new JniObjectReference ();
+			Assert.IsNull (valueManager.GetValue (ref invalid, JniObjectReferenceOptions.CopyAndDispose));
+		}
+
+		[Test]
+		public unsafe void GetValue_FindBestMatchType ()
+		{
+#if !NO_MARSHAL_MEMBER_BUILDER_SUPPORT
+			using (var t = new JniType (TestType.JniTypeName)) {
+				var c = t.GetConstructor ("()V");
+				var o = t.NewObject (c, null);
+				using (var w = valueManager.GetValue<IJavaPeerable> (ref o, JniObjectReferenceOptions.CopyAndDispose)) {
+					Assert.AreEqual (typeof (TestType), w!.GetType ());
+					Assert.IsTrue (((TestType) w).ExecutedActivationConstructor);
+				}
+			}
+#endif  // !NO_MARSHAL_MEMBER_BUILDER_SUPPORT
+		}
+
+		[Test]
+		public void PeekPeer ()
+		{
+			Assert.IsNull (valueManager.PeekPeer (new JniObjectReference ()));
+
+			using (var v = new MyDisposableObject ()) {
+				Assert.IsNotNull (valueManager.PeekPeer (v.PeerReference));
+				Assert.AreSame (v, valueManager.PeekPeer (v.PeerReference));
+			}
+		}
+
+		[Test]
+		public void PeekValue ()
+		{
+			JniObjectReference lref;
+			using (var o = new JavaObject ()) {
+				lref = o.PeerReference.NewLocalRef ();
+				Assert.AreSame (o, valueManager.PeekValue (lref));
+			}
+			// At this point, the Java-side object is kept alive by `lref`,
+			// but the wrapper instance has been disposed, and thus should
+			// be unregistered, and thus unfindable.
+			Assert.IsNull (valueManager.PeekValue (lref));
+			JniObjectReference.Dispose (ref lref);
+		}
+
+		[Test]
+		public void PeekValue_BoxedObjects ()
+		{
+			var marshaler   = valueManager.GetValueMarshaler<object> ();
+			var ad          = AppDomain.CurrentDomain;
+
+			var proxy       = marshaler.CreateGenericArgumentState (ad);
+			Assert.AreSame (ad, valueManager.PeekValue (proxy.ReferenceValue));
+			marshaler.DestroyGenericArgumentState (ad, ref proxy);
+
+			var ex  = new InvalidOperationException ("boo!");
+			proxy   = marshaler.CreateGenericArgumentState (ex);
+			Assert.AreSame (ex, valueManager.PeekValue (proxy.ReferenceValue));
+			marshaler.DestroyGenericArgumentState (ex, ref proxy);
+		}
+
+		void AllNestedRegistrationScopeTests ()
+		{
+			AddPeer ();
+			AddPeer_NoDuplicates ();
+			ConstructPeer_ImplicitViaBindingConstructor_PeerIsInSurfacedPeers ();
+			CreateValue ();
+			GetValue_FindBestMatchType ();
+			GetValue_ReturnsAlias ();
+			GetValue_ReturnsNullWithInvalidSafeHandle ();
+			GetValue_ReturnsNullWithNullHandle ();
+			PeekPeer ();
+			PeekValue ();
+			PeekValue_BoxedObjects ();
+		}
+
+		string DumpPeers ()
+		{
+			return DumpPeers (valueManager.GetSurfacedPeers ());
+		}
+
+		static string DumpPeers (IEnumerable<JniSurfacedPeerInfo> peers)
+		{
+			return string.Join ("," + Environment.NewLine, peers);
+		}
+
+
+		// also test:
+		// Singleton scenario
+		// Types w/o "activation" constructors -- need to support checking parent scopes
+		// nesting of scopes
+		// Adding an instance already added in a previous scope?
+	}
+
+	public abstract class JniRuntimeJniValueManagerContract<T> : JniRuntimeJniValueManagerContract {
+
+		protected override Type ValueManagerType => typeof (T);
+	}
+
+#if !__ANDROID__
+#if !NETCOREAPP
+	[TestFixture]
+	public class JniRuntimeJniValueManagerContract_Mono : JniRuntimeJniValueManagerContract {
+		static Type MonoRuntimeValueManagerType = Type.GetType ("Java.Interop.MonoRuntimeValueManager, Java.Runtime.Environment", throwOnError:true)!;
+
+		protected override Type ValueManagerType => MonoRuntimeValueManagerType;
+	}
+#endif	// !NETCOREAPP
+
+	[TestFixture]
+	public class JniRuntimeJniValueManagerContract_NoGCIntegration : JniRuntimeJniValueManagerContract {
+		static Type ManagedValueManagerType = Type.GetType ("Java.Interop.ManagedValueManager, Java.Runtime.Environment", throwOnError:true)!;
+
+		protected override Type ValueManagerType => ManagedValueManagerType;
+	}
+#endif  // !__ANDROID__
+}

tests/TestJVM/TestJVM.csproj

@@ -4,6 +4,8 @@
     <TargetFramework>$(DotNetTargetFramework)</TargetFramework>
     <Nullable>enable</Nullable>
     <IsPackable>false</IsPackable>
+    <SignAssembly>true</SignAssembly>
+    <AssemblyOriginatorKeyFile>..\..\product.snk</AssemblyOriginatorKeyFile>
   </PropertyGroup>
 
   <Import Project="..\..\TargetFrameworkDependentValues.props" />