Skip to content
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.

Sign up for GitHub

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] Java.Lang.Object.GetObject<T>() implementation #9630

Closed
wants to merge 2 commits into from
Closed

[Mono.Android] Java.Lang.Object.GetObject<T>() implementation #9630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fea602
[Mono.Android] `Java.Lang.Object.GetObject<T>()` implementation
jonathanpeppers Dec 17, 2024
d0186ab
Merge branch 'main' into dev/peppers/AndroidValueManager
jonathanpeppers Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Mono.Android/Android.App/Activity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ namespace Android.App {

partial class Activity {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

public T? FindViewById<
[DynamicallyAccessedMembers (Constructors)]
T
Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.App/Dialog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ protected Dialog (Android.Content.Context context, bool cancelable, EventHandler
: this (context, cancelable, new Android.Content.IDialogInterfaceOnCancelListenerImplementor () { Handler = cancelHandler }) {}

public T? FindViewById<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
T
> (int id)
where T : Android.Views.View
Expand Down
2 changes: 0 additions & 2 deletions src/Mono.Android/Android.App/FragmentManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
#if ANDROID_11
namespace Android.App {
public partial class FragmentManager {
const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

public T? FindFragmentById<
[DynamicallyAccessedMembers (Constructors)]
T
Expand Down
2 changes: 0 additions & 2 deletions src/Mono.Android/Android.OS/AsyncTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ public abstract class AsyncTask<
TResult
> : AsyncTask {

const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

static IntPtr java_class_handle;
internal static IntPtr class_ref {
get {
Expand Down
31 changes: 30 additions & 1 deletion src/Mono.Android/Android.Runtime/JNIEnv.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,29 @@ public static IntPtr FindClass (System.Type type)
}
}

internal static JniObjectReferenceOptions ToJniObjectReferenceOptions (JniHandleOwnership transfer)
{
JniObjectReferenceOptions options = default;
if (transfer.HasFlag (JniHandleOwnership.TransferLocalRef) ||
transfer.HasFlag (JniHandleOwnership.TransferGlobalRef))
options |= JniObjectReferenceOptions.CopyAndDispose;
if (transfer.HasFlag (JniHandleOwnership.DoNotRegister))
options |= JniObjectReferenceOptions.CopyAndDoNotRegister;
if (transfer == JniHandleOwnership.DoNotTransfer) // 0; can't use with .HasFlag()
options |= JniObjectReferenceOptions.Copy;

return options;
}

internal static JniObjectReference CreateJniObjectReference (IntPtr handle, JniHandleOwnership transfer)
{
if (transfer.HasFlag (JniHandleOwnership.TransferLocalRef))
return new JniObjectReference (handle, JniObjectReferenceType.Local);
if (transfer.HasFlag (JniHandleOwnership.TransferGlobalRef))
return new JniObjectReference (handle, JniObjectReferenceType.Global);
return new JniObjectReference (handle); // there be possible dragons here!
}

const int nameBufferLength = 1024;
[ThreadStatic] static char[]? nameBuffer;

Expand Down Expand Up @@ -704,7 +727,13 @@ public static void CopyArray (IntPtr src, string[] dest)
AssertIsJavaObject (type);

IntPtr elem = GetObjectArrayElement (source, index);
return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type);
return GetObject (elem, type);

// FIXME: Since a Dictionary<Type, Func> is used here, the trimmer will not be able to properly analyze `Type t`
// error IL2111: Method 'lambda expression' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method.
[UnconditionalSuppressMessage ("Trimming", "IL2067", Justification = "FIXME: https://github.com/xamarin/xamarin-android/issues/8724")]
static object? GetObject (IntPtr e, Type t) =>
Java.Lang.Object.GetObject (e, JniHandleOwnership.TransferLocalRef, t);
} },
{ typeof (Array), (type, source, index) => {
IntPtr elem = GetObjectArrayElement (source, index);
Expand Down
7 changes: 5 additions & 2 deletions src/Mono.Android/Android.Runtime/JavaArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
using System;
using System.Collections;
using System.Collections.Generic;

using System.Diagnostics.CodeAnalysis;

namespace Android.Runtime {

[Register ("mono/android/runtime/JavaArray", DoNotGenerateAcw=true)]
public sealed class JavaArray<T> : Java.Lang.Object, IList<T> {
public sealed class JavaArray<
[DynamicallyAccessedMembers (Constructors)]
T
> : Java.Lang.Object, IList<T> {

public JavaArray (IntPtr handle, JniHandleOwnership transfer)
: base (handle, transfer)
Expand Down
2 changes: 0 additions & 2 deletions src/Mono.Android/Android.Runtime/JavaCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ namespace Android.Runtime {
// java.util.Collection allows null values
public class JavaCollection : Java.Lang.Object, System.Collections.ICollection {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

internal static IntPtr collection_class = JNIEnv.FindClass ("java/util/Collection");

internal static IntPtr id_add;
Expand Down
2 changes: 0 additions & 2 deletions src/Mono.Android/Android.Runtime/JavaDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ namespace Android.Runtime {
// java.util.HashMap allows null keys and values
public class JavaDictionary : Java.Lang.Object, System.Collections.IDictionary {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

class DictionaryEnumerator : IDictionaryEnumerator {

IEnumerator simple_enumerator;
Expand Down
1 change: 0 additions & 1 deletion src/Mono.Android/Android.Runtime/JavaList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace Android.Runtime {
// java.util.ArrayList allows null values
public partial class JavaList : Java.Lang.Object, System.Collections.IList {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;
internal static readonly JniPeerMembers list_members = new XAPeerMembers ("java/util/List", typeof (JavaList), isInterface: true);

//
Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.Runtime/JavaSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ public static IntPtr ToLocalJniHandle (ICollection? items)
[Register ("java/util/HashSet", DoNotGenerateAcw=true)]
// java.util.HashSet allows null
public class JavaSet<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
T
> : JavaSet, ICollection<T> {

Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.Util/SparseArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Android.Util
{
[Register ("android/util/SparseArray", DoNotGenerateAcw=true)]
public partial class SparseArray<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
E
> : SparseArray
{
Expand Down
3 changes: 0 additions & 3 deletions src/Mono.Android/Android.Views/View.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ public enum SystemUiFlags {
#endif

public partial class View {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

#if ANDROID_16
[Obsolete ("This method uses wrong enum type. Please use PerformAccessibilityAction(Action) instead.")]
public bool PerformAccessibilityAction (GlobalAction action, Bundle arguments)
Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.Views/Window.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Android.Views {
partial class Window {

public T? FindViewById<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
T
> (int id)
where T : Android.Views.View
Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.Widget/AdapterView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public event EventHandler ItemSelectionCleared {

[Register ("android/widget/AdapterView", DoNotGenerateAcw=true)]
public abstract class AdapterView<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
T
> : AdapterView where T : IAdapter {

Expand Down
2 changes: 1 addition & 1 deletion src/Mono.Android/Android.Widget/ArrayAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace Android.Widget {

[Register ("android/widget/ArrayAdapter", DoNotGenerateAcw=true)]
public partial class ArrayAdapter<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
[DynamicallyAccessedMembers (Constructors)]
T
> : ArrayAdapter {

Expand Down
27 changes: 22 additions & 5 deletions src/Mono.Android/Java.Lang/Object.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ public partial class Object : IDisposable, IJavaObject, IJavaObjectEx
, IJavaPeerable
#endif // JAVA_INTEROP
{
internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

[NonSerialized] IntPtr key_handle;
#pragma warning disable CS0649, CS0169, CS0414 // Suppress fields are never used warnings, these fields are used directly by monodroid-glue.cc
[NonSerialized] int refs_added;
Expand Down Expand Up @@ -263,28 +265,41 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
return (T?)PeekObject (handle, typeof (T));
}

public static T? GetObject<T> (IntPtr jnienv, IntPtr handle, JniHandleOwnership transfer)
public static T? GetObject<
[DynamicallyAccessedMembers (Constructors)]
T
> (IntPtr jnienv, IntPtr handle, JniHandleOwnership transfer)
where T : class, IJavaObject
{
JNIEnv.CheckHandle (jnienv);
return GetObject<T> (handle, transfer);
}

public static T? GetObject<T> (IntPtr handle, JniHandleOwnership transfer)
public static T? GetObject<
[DynamicallyAccessedMembers (Constructors)]
T
> (IntPtr handle, JniHandleOwnership transfer)
where T : class, IJavaObject
{
return _GetObject<T>(handle, transfer);
}

internal static T? _GetObject<T> (IntPtr handle, JniHandleOwnership transfer)
internal static T? _GetObject<
[DynamicallyAccessedMembers (Constructors)]
T
> (IntPtr handle, JniHandleOwnership transfer)
{
if (handle == IntPtr.Zero)
return default (T);

return (T?) GetObject (handle, transfer, typeof (T));
}

internal static IJavaPeerable? GetObject (IntPtr handle, JniHandleOwnership transfer, Type? type = null)
internal static IJavaPeerable? GetObject (
IntPtr handle,
JniHandleOwnership transfer,
[DynamicallyAccessedMembers (Constructors)]
Type? type = null)
{
if (handle == IntPtr.Zero)
return null;
Expand All @@ -295,7 +310,9 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer)
return r;
}

return Java.Interop.TypeManager.CreateInstance (handle, transfer, type);
JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer);
JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer);
return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong about always wanting this cast.

That said, IIRC a previous version of this PR instead used as IJavaPeerable, which also seems wrong.

The problem: Java.InteropTests.JnienvTest.MoarThreadingTests is failing:

No exception should be thrown [t2]! Got: System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()
Expected: null
But was:  <System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()>

android/tests/Mono.Android-Tests/Java.Interop/JnienvTest.cs

Lines 363 to 386 in 8d77130

IntPtr lrefJliArray = JNIEnv.NewObjectArray<int> (new[]{1});
IntPtr grefJliArray = JNIEnv.NewGlobalRef (lrefJliArray);
JNIEnv.DeleteLocalRef (lrefJliArray);
Exception ignore_t1 = null;
Exception ignore_t2 = null;
var t1 = new Thread (() => {
int[] output_array1 = new int[1];
for (int i = 0; i < 2000; ++i) {
Console.WriteLine ("# t1 iter: {0}", i);
try {
JNIEnv.CopyObjectArray (grefJliArray, output_array1);
} catch (Exception e) {
ignore_t1 = e;
break;
}
}
});
var t2 = new Thread (() => {
for (int i = 0; i < 2000; ++i) {
Console.WriteLine ("# t2 iter: {0}", i);
try {
JNIEnv.GetObjectArray (grefJliArray, new[]{typeof (int)});

JnienvTest.cs:386 is the source of the JNIEnv.GetObjectArray() invocation at the root of the stack trace.

The cast on this line is the source of the InvalidCastException.

The question is: how did this previously work, and why is it failing now?

Tracing through how it previously worked:

  1. JNIEnv.GetObjectArray() is called; element_types=new[]{typeof(int)}.
  2. JNIEnv.GetObjectArray() calls GetConverter<T>(NativeArrayElementToManaged, null, array_ptr)
  3. GetConverter<T>() sees that array_ptr/array is not null; calls GetClassNameFromInstance(array).
  4. GetClassNameFromInstance(array) should be [Ljava/lang/Object;, because the array was created via JNIEnv.NewObjectArray<int> (new[]{1}).
  5. GetConverter<T>() thus skips the switch on lines 732-749
  6. GetConvter<T>() should thus return a Func<Type?, IntPtr, int, object?> for IJavaObject:

    android/src/Mono.Android/Android.Runtime/JNIEnv.cs

    Lines 703 to 708 in 8d77130

    { typeof (IJavaObject), (type, source, index) => {
    AssertIsJavaObject (type);
    IntPtr elem = GetObjectArrayElement (source, index);
    return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type);
    } },
  7. Returning to GetObjectArray(), line 1110 targetType will be typeof(int), we call converter(null, array_ptr, i).
  8. value should thus be a java.lang.Integer/Java.Lang.Integer instance, and the Convert.ChangeType() on line should turn it into an int, as Integer implements IConvertible.

So why's this change fail? Probably because AndroidValueManager?.GetValue(…) has different semantics from TypeManager.CreateInstance(…). I need to further investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonpryor wrote:

So why's this change fail? Probably because AndroidValueManager?.GetValue(…) has different semantics from TypeManager.CreateInstance(…). I need to further investigate.

Time for further investigation! Consider the following patch to java-interop/samples/Hello-Java.Base:

diff --git a/samples/Hello-Java.Base/Program.cs b/samples/Hello-Java.Base/Program.cs
index b1e0d5ba..9e63f3ba 100644
--- a/samples/Hello-Java.Base/Program.cs
+++ b/samples/Hello-Java.Base/Program.cs
@@ -61,10 +61,15 @@ namespace Hello
 			CreateJLO ();
 		}
 
-		static void CreateJLO ()
+		static unsafe void CreateJLO ()
 		{
-			var jlo = new Java.Lang.Object ();
-			Console.WriteLine ($"binding? {jlo.ToString ()}");
+			var i_class = new JniType ("java/lang/Integer");
+			var i_ctor  = i_class.GetConstructor ("(I)V");
+			JniArgumentValue* i_args = stackalloc JniArgumentValue [1];
+			i_args [0] = new JniArgumentValue (42);
+			var i_value = i_class.NewObject (i_ctor, i_args);
+			var v = JniEnvironment.Runtime.ValueManager.GetValue (ref i_value, JniObjectReferenceOptions.CopyAndDispose, null);
+			Console.WriteLine ($"v? {v} {v?.GetType ()}");
 		}
 
 		static void ReportTiming ()

This creates a new java.lang.Integer(42), invokes Runtime.ValueManager.GetValue(), and prints the resulting value and its type.

The output is:

% ~/Downloads/dotnet-sdk-8.0.206-osx-x64/dotnet run --project samples/Hello-Java.Base/*.csproj
…
v? 42 System.Int32

(I have to use .NET 8.0.206 because anything later crashes. Hopefully this will be fixed in the January .NET 8 service release…)

Note that Runtime.GetValue(objref, …, null) converts to a System.Int32, not a Java.Lang.Integer. This is why there's an InvalidCastException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the cause of the InvalidCastException is understood, what do we do about it?

There are at least two paths worth pursuing:

  1. Don't Do That™ (don't use ValueManager.GetValue() here)
  2. Don't Do That™ (update ValueManager.GetValue() to not auto-unbox like that.)

(1) should be possible, by e.g. updating TypeManager.CreateInstance() to use methods on JniEnvironment.Runtime.TypeManager instead of using the P/Invokes within GetClassName()/etc.

(2) should also be possible, but may be more time consuming, depending on how many unit tests it breaks.

}

[EditorBrowsable (EditorBrowsableState.Never)]
Expand Down
1 change: 1 addition & 0 deletions tests/api-compatibility/api-compat-exclude-attributes.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ T:System.Runtime.CompilerServices.IteratorStateMachineAttribute
T:System.Runtime.CompilerServices.NullableAttribute
T:System.Runtime.CompilerServices.NullableContextAttribute
T:System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute
T:System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute