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

jonpryor · web-flow · commit d5dfa0aac009 · 2025-01-16T18:04:07.000-05:00
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
diff --git a/src/Java.Interop/Java.Interop/JavaException.cs b/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"))
diff --git a/src/Java.Interop/Java.Interop/JavaObject.cs b/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)
diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/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) {
diff --git a/src/Java.Interop/PublicAPI.Unshipped.txt b/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
