-
Notifications
You must be signed in to change notification settings - Fork 531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Mono.Android] JavaCast<T>() should throw on unsupported types #9145
Merged
jonpryor
merged 1 commit into
main
from
dev/jonp/jonp-javacast-throws-on-unsupported-types
Jul 26, 2024
Merged
[Mono.Android] JavaCast<T>() should throw on unsupported types #9145
jonpryor
merged 1 commit into
main
from
dev/jonp/jonp-javacast-throws-on-unsupported-types
Jul 26, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>(); // 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 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 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!) 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 (good…), but this change also impacts `JavaCast<T>()` (bad). Fix `.JavaCast<T>()` by updating `JavaObjectExtensions.JavaCast<T>()` to throw an `InvalidCastException` if `TypeManager.CreateInstance()` returns null. Additionally, massively simplify the `.JavaCast<T>()` 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<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` will be `list` and `newAlias` will be false.
jonathanpeppers
approved these changes
Jul 25, 2024
jonathanpeppers
pushed a commit
that referenced
this pull request
Jul 26, 2024
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.The last line should throw because, at this point anyway,
java.lang.Integer
doesn't implement thejava.lang.Appendable
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:
What happened™?!
What happened 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 thatValidate()
was needed anymore, and that the checks it performed was already done elsewhere. Turns Out™, that wasn't true!)Commit 35f41dc actually added the checks that @jonpryor thought were already there, but as part of these checks
TypeManager.CreateInstance()
now returnsnull
if the Java-side type checks fail (good…), but this change also impactsJavaCast<T>()
(bad).Fix
.JavaCast<T>()
by updatingJavaObjectExtensions.JavaCast<T>()
to throw anInvalidCastException
ifTypeManager.CreateInstance()
returns null.Additionally, massively simplify the
.JavaCast<T>()
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:and to ensure "appropriate magic" happens, alias
list
:We now have two managed instances referring to the same Java instance. Next, we want to use
alias
as anAbstractList
:As
java.util.ArrayList
inheritsjava.util.AbstractList
, this will return a non-null
value. However, what type and instance willabstractList
be?In .NET 8 and .NET 9 prior to this commit,
newAlias
will be true andabstractList
will be anAbstractListInvoker
, meaning there will be three managed instances referring to the same Java-sideArrayList
.Starting with this commit,
abstractList
will belist
andnewAlias
will be false.