Comparing 35.0.7...35.0.24 · dotnet/android (original) (raw)

…9545)

Fixes: #9535

Context: a017561 Context: 35f41dc

Create a new .NET for Android project (dotnet new android), add a @(PackageReference) to Xamarin.AndroidX.Media3.ExoPlayer, and add the following code:

var builder = new AndroidX.Media3.ExoPlayer.ExoPlayerBuilder(this);
var player = builder.Build();

Build and run in Debug configuration, and it crashes:

System.ArgumentException: Could not determine Java type corresponding to `AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null`. (Parameter 'targetType')
   at Java.Interop.TypeManager.CreateInstance(IntPtr handle, JniHandleOwnership transfer, Type targetType) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Interop/TypeManager.cs:line 323
   at Java.Lang.Object.GetObject(IntPtr handle, JniHandleOwnership transfer, Type type) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 302
   at Java.Lang.Object._GetObject[IExoPlayer](IntPtr handle, JniHandleOwnership transfer) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 288
   at Java.Lang.Object.GetObject[IExoPlayer](IntPtr handle, JniHandleOwnership transfer) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Java.Lang/Object.cs:line 280
   at AndroidX.Media3.ExoPlayer.ExoPlayerBuilder.Build() in D:\a\_work\1\s\generated\androidx.media3.media3-exoplayer\obj\Release\net8.0-android\generated\src\AndroidX.Media3.ExoPlayer.IExoPlayer.cs:line 749
   at media3.MainActivity.OnCreate(Bundle savedInstanceState) in /Users/moljac/Downloads/1036/media3/MainActivity.cs:line 16
   at Android.App.Activity.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_savedInstanceState) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/obj/Release/net9.0/android-35/mcw/Android.App.Activity.cs:line 3196
   at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0) in /Users/runner/work/1/s/xamarin-android/src/Mono.Android/Android.Runtime/JNINativeWrapper.g.cs:line 121

☹️

The problem, as often appears to be the case these days, is an unexpected interaction between different things.

Firstly, the immediate cause of the crash is 35f41dc, which updated TypeManager.CreateInstance() to attempt to lookup the Java type that corresponds to a possible *Invoker type:

// `type` is `typeof(AndroidX.Media3.ExoPlayer.IExoPlayerInvoker)`
var typeSig  = JNIEnvInit.androidRuntime?.TypeManager.GetTypeSignature (type) ?? default;
if (!typeSig.IsValid || typeSig.SimpleReference == null) {
        throw new ArgumentException ($"Could not determine Java type corresponding to `{type.AssemblyQualifiedName}`.", nameof (targetType));
}

The underlying cause of the crash, and what 35f41dc uncovered, is that there is no type mapping for IExoPlayerInvoker. This is (not quite) readily seen by examining the generated file:

% grep IExoPlayerInvoker obj/Debug/net*android*/android/typemaps.arm64-v8a.ll
# no match

This is also true on .NET 8: there is no type mapping for IExoPlayerInvoker.

There are type mappings for other interface types, e.g. IIntSupplier:

% grep IIntSupplier obj/Debug/net*android*/android/typemaps.arm64-v8a.ll
@.TypeMapEntry.16871_from = private unnamed_addr constant [47 x i8] c"Java.Util.Functions.IIntSupplier, Mono.Android\00", align 1
@.TypeMapEntry.16873_from = private unnamed_addr constant [54 x i8] c"Java.Util.Functions.IIntSupplierInvoker, Mono.Android\00", align 1

A bit more digging around, and the "problem" is as follows: type map generation had a type ordering requirement, and needed the Invoker types to follow their corresponding abstract types.

# Mono.Android.dll: IIntSupplier is before IIntSupplierInvoker
% monodis --typedef obj/Debug/net9.0-android/android/assets/arm64-v8a/Mono.Android.dll | grep IIntSupplier
7676: Java.Util.Functions.IIntSupplier (flist=57465, mlist=135759, flags=0x1000a1, extends=0x0)
7677: Java.Util.Functions.IIntSupplierInvoker (flist=57465, mlist=135760, flags=0x100000, extends=0x7c28)

# Xamarin.AndroidX.Media3.ExoPlayer.dll: IExoPlayerInvoker is *before* IExoPlayer
% monodis --typedef obj/Debug/net9.0-android/android/assets/arm64-v8a/Xamarin.AndroidX.Media3.ExoPlayer.dll| grep IExoPlayer
139: AndroidX.Media3.ExoPlayer.IExoPlayerInvoker (flist=20, mlist=567, flags=0x100000, extends=0x85)
…
166: AndroidX.Media3.ExoPlayer.IExoPlayer (flist=598, mlist=1671, flags=0x1000a1, extends=0x0)

Reasonable aside: why is IExoPlayerInvoker before IExoPlayer? Because it's a "real" checked-in copy of generated code, not generated code that follows the IExoPlayer generated declaration.

The reason why ordering matters comes from a017561:

Update typemap generation code in Xamarin.Android.Build.Tasks.dll so that all the duplicate Java type names will point to the same managed type name. Additionally, make sure we select the managed type in the same fashion the old typemap generator in Java.Interop worked: prefer base types to the derived ones if the type is an interface or an abstract class. The effect of this change is that no matter which entry EmbeddedAssemblies::binary_search() ends up selecting it will always return the same managed type name for all aliased Java types.

The TypeMapGenerator.HandleDebugDuplicates() method from a017561 implicitly preferred the first TypeDefinition encountered for a given Java type. When the *Invoker came first, it attempted to treat it as a duplicate, but wound up removing it entirely:

oldEntry.TypeDefinition = td;
oldEntry.ManagedName = GetManagedTypeName (td);

oldEntry.TypeDefinition would refer to IExoPlayerInvoker, and setting it to td would instead set it to IExoPlayer. IExoPlayerInvoker is lost.

Fix this by updating HandleDebugDuplicates() to be clearer about intent:

// oldEntry == typeof(IExoPlayerInvoker)
// td == typeof(IExoPlayer)
if ((td.IsAbstract || td.IsInterface) &&
        !oldEntry.TypeDefinition.IsAbstract &&
        !oldEntry.TypeDefinition.IsInterface &&
        td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
    // We found the `Invoker` type *before* the declared type
    // Fix things up so the abstract type is first, and the `Invoker` is considered a duplicate.
    duplicates.Insert (0, entry);
    oldEntry.SkipInJavaToManaged = false;
}

Now, when the "new" TypeDefinition is:

1. An abstract class or interface type, and
2. The original entry is neither an abstract class nor interface, &
3. The old entry can be assigned to the new entry,

then we assume that this is a *Invoker scenario, ensure things are consistent with the "expected" { IExoPlayer, IExoPlayerInvoker } order:

1. Inserting the new TypeDefinition first in duplicates.

   This ensures expected ordering.

2. Setting oldEntry.SkipInJavaToManaged to false.

   This ensures that IExoPlayerInvoker is emitted in the typemaps.

WORKAROUND: Building the app in Release configuration avoids the crash. (Then cry about inner dev loop performance.)

"But what about tests?" Originally, @jonpryor thought that JavaObjectExtensionsTests.JavaCast_InterfaceCast() was a sufficient test. This was wrong, because it didn't test what it thought it did:

IntPtr g;
using (var n = new Java.Lang.Integer (42)) {
    g = JNIEnv.NewGlobalRef (n.Handle);
}
// We want a Java.Lang.Object so that we create an IComparableInvoker
// instead of just getting back the original instance.
using (var o = Java.Lang.Object.GetObject<Java.Lang.Object> (g, JniHandleOwnership.TransferGlobalRef)) {
    var c = JavaObjectExtensions.JavaCast<Java.Lang.IComparable> (o);
    c.Dispose ();
}

The comment is not quite right: while it says we want a Java.Lang.Object, that's not the runtime type o.GetType(). The runtime type o.GetType() would be Java.Lang.Integer (!), because the first thing that TypeManager.CreateInstance() tries to do is see if we have an existing type mapping for the runtime type of the JNI handle. As the runtime type is java.lang.Integer, we do have such a binding, and that bound type is implicitly convertible to Java.Lang.Object.

Meaning this test never actually tested an interface cast!

So for starters, we need to fix this to something that ensures we get an unbound type in the base class hierarchy, so that we can actually test interface checking behavior.

Enter @(AndroidJavaSource), ValueProvider.java, and Example.java. Example.getValueProvider() returns an anonymous inner class that implements ValueProvider, ensuring that the only bound type that will work is Java.Lang.Object. We then can use JavaObjectExtensions.JavaCast<T>() as originally intended.

We could have used any interface type instead of introducing a new ValueProvider interface, but introducing a new type is the only way to attempt to reproduce the TypeDefinition ordering issue, by adding a new partial class for IValueProviderInvoker.

Locally, we get the intended "wrong" ordering of Invoker before abstract type:

% monodis --typedef tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/obj/Debug/net9.0-android/Mono.Android.NET-Tests.dll | grep IValueProvider
5: Net.Dot.Android.Test.IValueProviderInvoker (flist=10, mlist=20, flags=0x100000, extends=0x89)
7: Net.Dot.Android.Test.IValueProvider (flist=13, mlist=37, flags=0x1000a1, extends=0x0)

and the test passes as desired.