[Java.Interop] Java.Lang.Object, Mono.Android Unification Changes (#1293)

Context: dotnet/android#9640
Context: 25de1f3
Context: 3043d89

dotnet/android#9640 completes "unification" with dotnet/java-interop,
updating `Java.Lang.Object` to inherit `Java.Interop.JavaObject`,
just like `src/Java.Base`.

The biggest impediment with this unification step is that the
semantics for referring to a Java instance are quite different:

  * .NET for Android née Xamarin.Android née Mono for Android uses
    an `(IntPtr, JniHandleOwnership)` pair to specify the JNI object
    reference pointer value and what should be done with it.
    The *type* of the JNI object reference is *inferred* by the
    `JniHandleOwnership` value, e.g.
    `JniHandleOwnership.TransferLocalRef` means that the `IntPtr`
    value is a JNI local reference.

  * dotnet/java-interop uses a
    `(ref JniObjectReference, JniObjectReferenceOptions)` pair to
    specify the JNI object reference and what can be done with it.
    The `JniObjectReference` value contains the type, but -- due
    to "pointer trickery"; see 25de1f3 -- it might be *so* invalid
    that it cannot be touched at all, and `JniObjectReferenceOptions`
    will let us know.

Which brings us to the conundrum: how do we implement the
`Java.Lang.Throwable(IntPtr, JniHandleOwnership)` constructor?

	namespace Java.Lang {
	    public class Throwable {
	        public Throwable(IntPtr handle, JniHandleOwnership transfer);
	    }
	}

The "obvious and wrong" solution would be to use `ref` with a
temporary…

	public Throwable(IntPtr handle, JniHandleOwnership transfer)
	    : base (ref new JniObjectReference(handle), …)
	{
	}

…but that won't compile.

Additionally, we want to be able to provide `message` and
`innerException` parameters to `System.Exception`, but the
`JavaException(ref JniObjectReference, JniObjectReferenceOptions)`
constructor requires a valid JNI Object Reference to do that.

If `Java.Lang.Throwable` tries to "work around" this by using the
`JavaException(string, Exception)` constructor:

	public Throwable(IntPtr handle, JniHandleOwnership transfer)
	    : base (_GetMessage (handle), _GetInnerException (handle))
	{
	    …
	}

then the `JavaException(string, Exception)` constructor will try to
invoke the `E(String)` constructor on the Java side, which:

 1. Is semantically wrong, because we may already have a Java
    instance in `handle`, so why are we creating a new instance? But-

 2. This fails for constructor subclasses which don't provide a
    `E(String)` constructor!  Specifically, the
    [`JnienvTest.ActivatedDirectThrowableSubclassesShouldBeRegistered`][0]
    unit test starts crashing for "bizarre" reasons.

Fix these issues by adding the new `JavaException` constructor:

	namespace Java.Interop {
	    partial class JavaException {
		protected JavaException (
	                ref JniObjectReference reference,
	                JniObjectReferenceOptions transfer,
	                JniObjectReference throwableOverride);
	    }
	}

The new `throwableOverride` parameter is used to obtain the `message`
and `innerException` values to provide to the
`System.Exception(string, Exception)` constructor, allowing
`reference` to be a "do not touch" value.

Additionally, add a `protected SetJavaStackTrace()` method which
is used to set the `JavaException.JavaStackTrace` property based on
a `JniObjectReference` value.

Update `Java.Interop.JavaObject` to be `[Serializable]`, and to mark
its fields as *`[NonSerialized]`*.  This is needed to fix the 
`InstallAndRunTests.JsonDeserializationCreatesJavaHandle()` tests:

	E AndroidRuntime: android.runtime.JavaProxyThrowable: [System.Runtime.Serialization.InvalidDataContractException]: AttributedTypesCannotInheritFromNonAttributedSerializableTypes, Java.Lang.Object, Java.Interop.JavaObject

Finally, update
`Java.Interop.JniRuntime.JniValueManager.ConstructPeer()` to *not*
attempt to call `AddPeer()`/etc. when `peer.JniManagedPeerState`
has `JniManagedPeerStates.Activatable` set.  Failure to do so was
causing `BindingTests.JavaSideActivation()` to fail, as it was
"losing" a GREF.  `JavaSideActivation()` is the scenario described
in 3043d89, in which:

 1. The static Java method `CallMethodFromCtor.NewInstance()` creates
    an instance of `ConstructorTest`, from Java.

 2. The default constructor in the Java Callable Wrapper (JCW) for
    `ConstructorTest` chains to the Java `CallMethodFromCtor()` default
    constructor.

 3. The Java `CallMethodFromCtor()` default constructor invokes the
    method `calledFromCtor()`, which is overridden in the C#
    `ConstructorTest.CalledFromCtor()` method.

 4. This causes the `CallMethodFromCtor.n_CalledFromCtor()` marshal
    method to be invoked, which calls
    `Java.Lang.Object.GetObject<CallMethodFromCtor>()`, which is akin
    to `runtime.ValueManager.GetValue<CallMethodFromCtor>(…)`.

 5. `Object.GetObject<CallMethodFromCtor>()` doesn't find an already
    existing object mapping for the instance we're still trying to
    construct, so it creates a "proxy" object via
    [`TypeManager.CreateInstance()`][2], which winds up invoking the
    `ConstructorTest(IntPtr, JniHandleOwnership)` "activation"
    constructor to create the "proxy".  The activation constructor
    eventually invokes `JavaObject.Construct()` /
    `JniRuntime.JniValueManager.ConstructPeer()`.

    This is the first "important" GREF created.

    Once the proxy is created, it's state is set to
    `JniManagedPeerStates.Activatable`.

 6. The `ConstructorTest.CalledFromCtor()` C# override is invoked,
    control returns to the Java `CallMethodFromCtor` constructor,
    which completes, allowing the JCW `ConstructorTest` constructor
    to execute, which calls [`TypeManager.n_Activate(…)`][3], which
    is equivalent to `ManagedPeer.Construct()`.

 7. `TypeManager.n_Activate(…)` sees that the instance
    "needs activation", and invokes the
    C# `ConstructorTest()` default constructor.  This eventually hits
    (again) `JniRuntime.JniValueManager.ConstructPeer()`.

The problem was that on the second
`JniRuntime.JniValueManager.ConstructPeer()` invocation in (7),
another GREF was created, replacing the GREF created in (5), but
without *disposing* of the GREF created in (5).  This caused a GREF
leak, which the `JavaSideActivation()` test was looking for.

Updating `JniRuntime.JniValueManager.ConstructPeer()` to bail when
it encounters a Peer with `JniManagedPeerStates.Activatable` avoids
the GREF creation in (7), thus avoiding the GREF leak.

[0]: https://github.com/dotnet/android/blob/f9f421b109b2a0f8f805ed14ab1c8dbe651f563e/tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs#L285-L305
[1]: https://github.com/dotnet/android/blob/main/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs#L97-L115
[2]: https://github.com/dotnet/android/blob/f9f421b109b2a0f8f805ed14ab1c8dbe651f563e/src/Mono.Android/Java.Interop/TypeManager.cs#L240-L332
[3]: https://github.com/dotnet/android/blob/f9f421b109b2a0f8f805ed14ab1c8dbe651f563e/src/Mono.Android/Java.Interop/TypeManager.cs#L132-L167

Author: jonpryor

src/Java.Interop/Java.Interop/JavaException.cs

@@ -38,7 +38,7 @@ public unsafe JavaException ()
 			var peer = JniPeerMembers.InstanceMethods.StartCreateInstance ("()V", GetType (), null);
 			Construct (ref peer, JniObjectReferenceOptions.CopyAndDispose);
 			JniPeerMembers.InstanceMethods.FinishCreateInstance ("()V", this, null);
-			JavaStackTrace    = GetJavaStack (PeerReference);
+			SetJavaStackTrace ();
 		}
 
 		public unsafe JavaException (string message)
@@ -55,7 +55,7 @@ public unsafe JavaException (string message)
 			} finally {
 				JniObjectReference.Dispose (ref native_message, JniObjectReferenceOptions.CopyAndDispose);
 			}
-			JavaStackTrace    = GetJavaStack (PeerReference);
+			SetJavaStackTrace ();
 		}
 
 		public unsafe JavaException (string message, Exception innerException)
@@ -72,15 +72,22 @@ public unsafe JavaException (string message, Exception innerException)
 			} finally {
 				JniObjectReference.Dispose (ref native_message, JniObjectReferenceOptions.CopyAndDispose);
 			}
-			JavaStackTrace    = GetJavaStack (PeerReference);
+			SetJavaStackTrace ();
+		}
+
+		protected JavaException (ref JniObjectReference reference, JniObjectReferenceOptions transfer, JniObjectReference throwableOverride)
+			: base (GetMessage (throwableOverride), GetCause (throwableOverride))
+		{
+			Construct (ref reference, transfer);
+			SetJavaStackTrace (throwableOverride);
 		}
 
 		public JavaException (ref JniObjectReference reference, JniObjectReferenceOptions transfer)
 			: base (GetMessage (ref reference, transfer), GetCause (ref reference, transfer))
 		{
 			Construct (ref reference, transfer);
 			if (PeerReference.IsValid)
-				JavaStackTrace    = GetJavaStack (PeerReference);
+				SetJavaStackTrace ();
 		}
 
 		protected void Construct (ref JniObjectReference reference, JniObjectReferenceOptions options)
@@ -183,6 +190,13 @@ public override unsafe int GetHashCode ()
 		{
 			if (transfer == JniObjectReferenceOptions.None)
 				return null;
+			return GetMessage (reference);
+		}
+
+		static string? GetMessage (JniObjectReference reference)
+		{
+			if (!reference.IsValid)
+				return null;
 
 			var m = _members.InstanceMethods.GetMethodInfo ("getMessage.()Ljava/lang/String;");
 			var s = JniEnvironment.InstanceMethods.CallObjectMethod (reference, m);
@@ -193,12 +207,30 @@ public override unsafe int GetHashCode ()
 		{
 			if (transfer == JniObjectReferenceOptions.None)
 				return null;
+			return GetCause (reference);
+		}
+
+		static Exception? GetCause (JniObjectReference reference)
+		{
+			if (!reference.IsValid)
+				return null;
 
 			var m = _members.InstanceMethods.GetMethodInfo ("getCause.()Ljava/lang/Throwable;");
 			var e = JniEnvironment.InstanceMethods.CallObjectMethod (reference, m);
 			return JniEnvironment.Runtime.GetExceptionForThrowable (ref e, JniObjectReferenceOptions.CopyAndDispose);
 		}
 
+		protected void SetJavaStackTrace (JniObjectReference peerReferenceOverride = default)
+		{
+			if (!peerReferenceOverride.IsValid) {
+				peerReferenceOverride   = PeerReference;
+			}
+			if (!peerReferenceOverride.IsValid) {
+				return;
+			}
+			JavaStackTrace  = GetJavaStack (peerReferenceOverride);
+		}
+
 		unsafe string? GetJavaStack (JniObjectReference handle)
 		{
 			using (var StringWriter_class   = new JniType ("java/io/StringWriter"))

src/Java.Interop/Java.Interop/JavaObject.cs

@@ -2,29 +2,35 @@
 
 using System;
 using System.Diagnostics.CodeAnalysis;
+using System.Runtime.Serialization;
 
 namespace Java.Interop
 {
 	[JniTypeSignature ("java/lang/Object", GenerateJavaPeer=false)]
+	[Serializable]
 	unsafe public class JavaObject : IJavaPeerable
 	{
 		internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;
 
 		readonly static JniPeerMembers _members = new JniPeerMembers ("java/lang/Object", typeof (JavaObject));
 
-		public int                  JniIdentityHashCode { get; private set; }
-		public JniManagedPeerStates JniManagedPeerState { get; private set; }
+		[NonSerialized] int                     identityHashCode;
+		[NonSerialized] JniManagedPeerStates    managedPeerState;
+
+		public int                  JniIdentityHashCode => identityHashCode;
+
+		public JniManagedPeerStates JniManagedPeerState => managedPeerState;
 
 #if FEATURE_JNIOBJECTREFERENCE_SAFEHANDLES
-		JniObjectReference  reference;
+		[NonSerialized] JniObjectReference  reference;
 #endif  // FEATURE_JNIOBJECTREFERENCE_SAFEHANDLES
 #if FEATURE_JNIOBJECTREFERENCE_INTPTRS
-		IntPtr                  handle;
-		JniObjectReferenceType  handle_type;
+		[NonSerialized] IntPtr                  handle;
+		[NonSerialized] JniObjectReferenceType  handle_type;
 	#pragma warning disable 0169
 		// Used by JavaInteropGCBridge
-		IntPtr                  weak_handle;
-		int                     refs_added;
+		[NonSerialized] IntPtr                  weak_handle;
+		[NonSerialized] int                     refs_added;
 	#pragma warning restore 0169
 #endif  // FEATURE_JNIOBJECTREFERENCE_INTPTRS
 
@@ -151,12 +157,12 @@ void IJavaPeerable.Finalized ()
 
 		void IJavaPeerable.SetJniIdentityHashCode (int value)
 		{
-			JniIdentityHashCode = value;
+			identityHashCode    = value;
 		}
 
 		void IJavaPeerable.SetJniManagedPeerState (JniManagedPeerStates value)
 		{
-			JniManagedPeerState = value;
+			managedPeerState    = value;
 		}
 
 		void IJavaPeerable.SetPeerReference (JniObjectReference reference)

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

@@ -97,8 +97,13 @@ public void ConstructPeer (IJavaPeerable peer, ref JniObjectReference reference,
 
 				var newRef  = peer.PeerReference;
 				if (newRef.IsValid) {
-					// Activation! See ManagedPeer.RunConstructor
-					peer.SetJniManagedPeerState (peer.JniManagedPeerState | JniManagedPeerStates.Activatable);
+					JniObjectReference.Dispose (ref reference, options);
+
+					// Activation? See ManagedPeer.Construct, CreatePeer
+					// Instance was already added, don't add again
+					if (peer.JniManagedPeerState.HasFlag (JniManagedPeerStates.Activatable)) {
+						return;
+					}
 					JniObjectReference.Dispose (ref reference, options);
 					newRef   = newRef.NewGlobalRef ();
 				} else if (options == JniObjectReferenceOptions.None) {

src/Java.Interop/PublicAPI.Unshipped.txt

@@ -3,5 +3,7 @@ static Java.Interop.JniEnvironment.BeginMarshalMethod(nint jnienv, out Java.Inte
 static Java.Interop.JniEnvironment.EndMarshalMethod(ref Java.Interop.JniTransition transition) -> void
 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.JniTypeSignatureAttribute.InvokerType.get -> System.Type?
 Java.Interop.JniTypeSignatureAttribute.InvokerType.set -> void