Skip to content

Commit 9a27140

Browse files
jonpryorjonpryor
authored
[Mono.Android] JavaCast<T>() should throw on unsupported types (#9145)
Context: dotnet/java-interop#910 Context: dotnet/java-interop@1adb796 Context: 8928f11 Context: 35f41dc Context: ab412a5 The `.JavaCast<T>()` 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<Java.Lang.IAppendable>(); // this should throw, right? The last line should throw because, at this point anyway, [`java.lang.Integer`][0] doesn't implement the [`java.lang.Appendable`][1] interface. What *actually* happens as of ab412a5 is that `n.JavaCast<IAppendable>()` ***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 are (at least) two things: First is dotnet/java-interop@1adb7964 and 8928f11, 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!) The result of 8928f11 is that `.JavaCast<T>()` would *always* return a non-`null` value for the requested type. Worse, any method invocations on that value would ***CRASH THE PROCESS***: var n = new Java.Lang.Integer(42); var a = n.JavaCast<Java.Lang.IAppendable>(); // does NOT throw a.Append('a'); // app dies `adb logcat` contains: JNI DETECTED ERROR IN APPLICATION: can't call java.lang.Appendable java.lang.Appendable.append(char) on instance of java.lang.Integer in call to CallObjectMethodA Oops. Secondly, commit 35f41dc 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. This means that `.JavaCast<T>()` now returns `null`, which prevents the above `JNI DETECTED ERROR IN APPLICATION` crash (good), but means `.JavaCast<T>()` returns `null` which will likely result in `NullReferenceException`s if the value is used. Fix `.JavaCast<T>()` by updating `JavaObjectExtensions.JavaCast<T>()` to throw an `InvalidCastException` if `TypeManager.CreateInstance()` returns `null`. Additionally, massively simplify the `.JavaCast<T>()` code paths by removing the semantic distinction 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<Java.Util.AbstractList>(); 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` be the same value as `list`, have a runtime type of `ArrayList`, and `newAlias` will be false. [0]: https://developer.android.com/reference/java/lang/Integer [1]: https://developer.android.com/reference/java/lang/Appendable
1 parent ea7d8c2 commit 9a27140

File tree

3 files changed

+32
-44
lines changed

3 files changed

+32
-44
lines changed

‎src/Mono.Android/Java.Interop/JavaObjectExtensions.cs‎

Lines changed: 6 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -81,41 +81,9 @@ internal static TResult? _JavaCast<
8181
if (instance.Handle == IntPtr.Zero)
8282
throw new ObjectDisposedException (instance.GetType ().FullName);
8383

84-
Type resultType = typeof (TResult);
85-
if (resultType.IsClass) {
86-
return (TResult) CastClass (instance, resultType);
87-
}
88-
else if (resultType.IsInterface) {
89-
return (TResult?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType);
90-
}
91-
else
92-
throw new NotSupportedException (FormattableString.Invariant ($"Unable to convert type '{instance.GetType ().FullName}' to '{resultType.FullName}'."));
93-
}
94-
95-
static IJavaObject CastClass (
96-
IJavaObject instance,
97-
[DynamicallyAccessedMembers (Constructors)]
98-
Type resultType)
99-
{
100-
var klass = JNIEnv.FindClass (resultType);
101-
try {
102-
if (klass == IntPtr.Zero)
103-
throw new ArgumentException ("Unable to determine JNI class for '" + resultType.FullName + "'.", "TResult");
104-
if (!JNIEnv.IsInstanceOf (instance.Handle, klass))
105-
throw new InvalidCastException (
106-
FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{resultType.FullName}'."));
107-
} finally {
108-
JNIEnv.DeleteGlobalRef (klass);
109-
}
110-
111-
if (resultType.IsAbstract) {
112-
// TODO: keep in sync with TypeManager.CreateInstance() algorithm
113-
var invokerType = GetInvokerType (resultType);
114-
if (invokerType == null)
115-
throw new ArgumentException ("Unable to get Invoker for abstract type '" + resultType.FullName + "'.", "TResult");
116-
resultType = invokerType;
117-
}
118-
return (IJavaObject) TypeManager.CreateProxy (resultType, instance.Handle, JniHandleOwnership.DoNotTransfer);
84+
return (TResult) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, typeof (TResult)) ??
85+
throw new InvalidCastException (
86+
FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{typeof (TResult).FullName}'."));
11987
}
12088

12189
internal static IJavaObject? JavaCast (
@@ -132,14 +100,9 @@ static IJavaObject CastClass (
132100
if (resultType.IsAssignableFrom (instance.GetType ()))
133101
return instance;
134102

135-
if (resultType.IsClass) {
136-
return CastClass (instance, resultType);
137-
}
138-
else if (resultType.IsInterface) {
139-
return (IJavaObject?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType);
140-
}
141-
else
142-
throw new NotSupportedException (FormattableString.Invariant ($"Unable to convert type '{instance.GetType ().FullName}' to '{resultType.FullName}'."));
103+
return (IJavaObject?) Java.Lang.Object.GetObject (instance.Handle, JniHandleOwnership.DoNotTransfer, resultType) ??
104+
throw new InvalidCastException (
105+
FormattableString.Invariant ($"Unable to convert instance of type '{instance.GetType ().FullName}' to type '{resultType.FullName}'."));
143106
}
144107

145108
// typeof(Foo) -> FooInvoker

‎src/Mono.Android/Java.Interop/TypeManager.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,6 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership
336336

337337
handleClass = JniEnvironment.Types.GetObjectClass (new JniObjectReference (handle));
338338
if (!JniEnvironment.Types.IsAssignableFrom (handleClass, typeClass)) {
339-
Logger.Log (LogLevel.Info, "*jonp*", $"# jonp: can't assign `{GetClassName(handleClass.Handle)}` to `{GetClassName(typeClass.Handle)}`");
340339
return null;
341340
}
342341
} finally {

‎tests/Mono.Android-Tests/Java.Interop/JavaObjectExtensionsTests.cs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,32 @@ public void JavaCast_InterfaceCast ()
4040
}
4141
}
4242

43+
[Test]
44+
public void JavaCast_BadInterfaceCast ()
45+
{
46+
using var n = new Java.Lang.Integer (42);
47+
var e = Assert.Catch<Exception>(() => JavaObjectExtensions.JavaCast<Java.Lang.IAppendable> (n));
48+
if (e is System.Reflection.TargetInvocationException tie) {
49+
// .NET 8 behavior
50+
Assert.IsTrue (tie.InnerException is InvalidCastException);
51+
} else if (e is InvalidCastException ice) {
52+
// Yay
53+
} else {
54+
Assert.Fail ($"Unexpected exception type: {e.GetType ()}: {e.ToString ()}");
55+
}
56+
}
57+
58+
[Test]
59+
public void JavaCast_ObtainOriginalInstance ()
60+
{
61+
using var list = new Java.Util.ArrayList ();
62+
using var copy = new Java.Lang.Object (list.Handle, JniHandleOwnership.DoNotTransfer);
63+
using var al = JavaObjectExtensions.JavaCast<Java.Util.AbstractList> (copy);
64+
65+
// Semantic change: in .NET 8, `al` is a new `AbstractListInvoker` instance, not `list`
66+
Assert.AreSame (list, al);
67+
}
68+
4369
[Test]
4470
public void JavaCast_InvalidTypeCastThrows ()
4571
{

0 commit comments

Comments
 (0)