[Xamarin.Android.Build.Tasks] Invokers after abstracts in typemaps by jonpryor · Pull Request #9545 · dotnet/android (original) (raw)

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.