From 6b26b39cfaae62b5d335035e342b33dc02ae05b7 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Thu, 25 Jul 2024 14:35:14 -0400 Subject: [PATCH] [Mono.Android] JavaCast() should throw on unsupported types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/dotnet/java-interop/issues/910 Context: https://github.com/dotnet/java-interop/commit/1adb7964a2033c83c298c070f2d1ab896d92671b Context: 8928f1168a74f9edad705f4851f3e9252a64f90e Context: 35f41dcc7cf5eb65dc12d7c0a3421e67c2bdb6d6 Context: ab412a5629778598f9b33b9b976721898dbacc95 The `.JavaCast()` extension method is for checking type casts across the JNI boundary. As such, it can be expected to fail. Failure is indicated by throwing an exception. var n = new Java.Lang.Integer(42); var a = n.JavaCast(); // should throw, right? The last line should throw because, at this point anyway, `java.lang.Integer` doesn't implement the `java.lang.Appendable` interface. What *actually* happens as of ab412a56 is that `n.JavaCast()` ***does not throw***; instead, it returns `null`! This is a ***BREAK*** from .NET 8, in which an exception *is* thrown: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidCastException: Unable to convert instance of type 'crc64382c01b66da5f22f/MainActivity' to type 'java.lang.Appendable'. at Java.Lang.IAppendableInvoker.Validate(IntPtr handle) at Java.Lang.IAppendableInvoker..ctor(IntPtr handle, JniHandleOwnership transfer) at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Constructor(Object obj, IntPtr* args) at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr) --- End of inner exception stack trace --- at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr) at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at System.Reflection.ConstructorInfo.Invoke(Object[] parameters) at Java.Interop.TypeManager.CreateProxy(Type type, IntPtr handle, JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance(IntPtr handle, JniHandleOwnership transfer, Type targetType) at Java.Lang.Object.GetObject(IntPtr handle, JniHandleOwnership transfer, Type type) at Java.Interop.JavaObjectExtensions._JavaCast[IAppendable](IJavaObject instance) What happened™?! What happened is dotnet/java-interop@1adb7964 and 8928f116, which optimized interface invokers (dotnet/java-interop#910). Part of this optimization involved *removing* the `Validate(IntPtr)` method from the interface invoker classes. (Was this part of the optimization? No. It's just that @jonpryor didn't think that `Validate()` was needed anymore, and that the checks it performed was already done elsewhere. Turns Out™, that wasn't true!) Commit 35f41dcc actually added the checks that @jonpryor thought were already there, but as part of these checks `TypeManager.CreateInstance()` now returns `null` if the Java-side type checks fail (good…), but this change also impacts `JavaCast()` (bad). Fix `.JavaCast()` by updating `JavaObjectExtensions.JavaCast()` to throw an `InvalidCastException` if `TypeManager.CreateInstance()` returns null. Additionally, massively simplify the `.JavaCast()` codepaths by removing the semantic separation between interface and class types. This in turn introduces *new* (intentional!) semantic changes. Suppose we have an `ArrayList` instance: var list = new Java.Util.ArrayList(); and to ensure "appropriate magic" happens, *alias* `list`: var alias = new Java.Lang.Object (list.Handle, JniHandleOwnership.DoNotTransfer); We now have two managed instances referring to the same Java instance. Next, we want to use `alias` as an `AbstractList`: var abstractList = alias.JavaCast(); var newAlias = !object.ReferenceEquals(list, abstractList); As `java.util.ArrayList` inherits `java.util.AbstractList`, this will return a non-`null` value. *However*, what *type and instance* will `abstractList` be? In .NET 8 and .NET 9 *prior to this commit*, `newAlias` will be true and `abstractList` will be an `AbstractListInvoker`, meaning there will be *three* managed instances referring to the same Java-side `ArrayList`. Starting with this commit, `abstractList` will be `list` and `newAlias` will be false. --- .../Java.Interop/JavaObjectExtensions.cs | 49 +++---------------- src/Mono.Android/Java.Interop/TypeManager.cs | 1 - .../Java.Interop/JavaObjectExtensionsTests.cs | 26 ++++++++++ 3 files changed, 32 insertions(+), 44 deletions(-) diff --git a/src/Mono.Android/Java.Interop/JavaObjectExtensions.cs b/src/Mono.Android/Java.Interop/JavaObjectExtensions.cs index 345c664c61d..a3e817facb9 100644 --- a/src/Mono.Android/Java.Interop/JavaObjectExtensions.cs +++ b/src/Mono.Android/Java.Interop/JavaObjectExtensions.cs @@ -81,41 +81,9 @@ internal static TResult? _JavaCast< if (instance.Handle == IntPtr.Zero) throw new ObjectDisposedException (instance.GetType ().FullName); - Type resultType = typeof (TResult); - if (resultType.IsClass) { - return (TResult) CastClass (instance, resultType); - } - else if (resultType.IsInterface) { - return (TResult?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType); - } - else - throw new NotSupportedException (FormattableString.Invariant ($"Unable to convert type '{instance.GetType ().FullName}' to '{resultType.FullName}'.")); - } - - static IJavaObject CastClass ( - IJavaObject instance, - [DynamicallyAccessedMembers (Constructors)] - Type resultType) - { - var klass = JNIEnv.FindClass (resultType); - try { - if (klass == IntPtr.Zero) - throw new ArgumentException ("Unable to determine JNI class for '" + resultType.FullName + "'.", "TResult"); - if (!JNIEnv.IsInstanceOf (instance.Handle, klass)) - throw new InvalidCastException ( - FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{resultType.FullName}'.")); - } finally { - JNIEnv.DeleteGlobalRef (klass); - } - - if (resultType.IsAbstract) { - // TODO: keep in sync with TypeManager.CreateInstance() algorithm - var invokerType = GetInvokerType (resultType); - if (invokerType == null) - throw new ArgumentException ("Unable to get Invoker for abstract type '" + resultType.FullName + "'.", "TResult"); - resultType = invokerType; - } - return (IJavaObject) TypeManager.CreateProxy (resultType, instance.Handle, JniHandleOwnership.DoNotTransfer); + return (TResult) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, typeof (TResult)) ?? + throw new InvalidCastException ( + FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{typeof (TResult).FullName}'.")); } internal static IJavaObject? JavaCast ( @@ -132,14 +100,9 @@ static IJavaObject CastClass ( if (resultType.IsAssignableFrom (instance.GetType ())) return instance; - if (resultType.IsClass) { - return CastClass (instance, resultType); - } - else if (resultType.IsInterface) { - return (IJavaObject?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType); - } - else - throw new NotSupportedException (FormattableString.Invariant ($"Unable to convert type '{instance.GetType ().FullName}' to '{resultType.FullName}'.")); + return (IJavaObject?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType) ?? + throw new InvalidCastException ( + FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{resultType.FullName}'.")); } // typeof(Foo) -> FooInvoker diff --git a/src/Mono.Android/Java.Interop/TypeManager.cs b/src/Mono.Android/Java.Interop/TypeManager.cs index 3f21ea04435..334a9f542a8 100644 --- a/src/Mono.Android/Java.Interop/TypeManager.cs +++ b/src/Mono.Android/Java.Interop/TypeManager.cs @@ -336,7 +336,6 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership handleClass = JniEnvironment.Types.GetObjectClass (new JniObjectReference (handle)); if (!JniEnvironment.Types.IsAssignableFrom (handleClass, typeClass)) { - Logger.Log (LogLevel.Info, "*jonp*", $"# jonp: can't assign `{GetClassName(handleClass.Handle)}` to `{GetClassName(typeClass.Handle)}`"); return null; } } finally { diff --git a/tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs b/tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs index e6b133a4eaf..e5e56a839a6 100644 --- a/tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs +++ b/tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs @@ -40,6 +40,32 @@ public void JavaCast_InterfaceCast () } } + [Test] + public void JavaCast_BadInterfaceCast () + { + using var n = new Java.Lang.Integer (42); + var e = Assert.Catch(() => JavaObjectExtensions.JavaCast (n)); + if (e is System.Reflection.TargetInvocationException tie) { + // .NET 8 behavior + Assert.IsTrue (tie.InnerException is InvalidCastException); + } else if (e is InvalidCastException ice) { + // Yay + } else { + Assert.Fail ($"Unexpected exception type: {e.GetType ()}: {e.ToString ()}"); + } + } + + [Test] + public void JavaCast_ObtainOriginalInstance () + { + using var list = new Java.Util.ArrayList (); + using var copy = new Java.Lang.Object (list.Handle, JniHandleOwnership.DoNotTransfer); + using var al = JavaObjectExtensions.JavaCast (copy); + + // Semantic change: in .NET 8, `al` is a new `AbstractListInvoker` instance, not `list` + Assert.AreSame (list, al); + } + [Test] public void JavaCast_InvalidTypeCastThrows () {