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.

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

[Media3] Attempting to create ExoPlayer instance crashes #9535

Open
tipa opened this issue Nov 5, 2024 · 26 comments · May be fixed by #9545
Open

[Media3] Attempting to create ExoPlayer instance crashes #9535

tipa opened this issue Nov 5, 2024 · 26 comments · May be fixed by #9545
Assignees
Labels
Area: Bindings Issues in Java Library Binding projects.

Comments

@tipa
Copy link

tipa commented Nov 5, 2024

Android framework version

net9.0-android

Affected platform version

.NET 9

Description

The code throw an exception when trying to create an instance of ExoPlayer

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')

Steps to Reproduce

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

Example project:
media3.zip

Given other developers seem to already be using the media3 bindings, I wonder if I am doing anything wrong...?

Did you find any workaround?

No response

Relevant log output

No response

@bmaluijb
Copy link

I have the exact same issue. I've tried many variations, including a clean MAUI project running .NET 9 (full version, not pre-release).

@moljac
Copy link
Contributor

moljac commented Nov 13, 2024

Repro Exception

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

@ne0rrmatrix
Copy link

ne0rrmatrix commented Nov 14, 2024

Here is an example of how to use media 3 with Maui. CommunityToolkit/Maui#2076

var Player = new ExoPlayerBuilder(Platform.AppContext).Build() ?? throw new InvalidOperationException("Player cannot be null");

Above is how you create an new instance of the player

@bmaluijb
Copy link

@ne0rrmatrix Have you tested this with .NET 9? I cannot get it working, just get the exception.

@ne0rrmatrix
Copy link

I have tested against dotnet 8 over the last few months. I am currently unable to test using either dotnet 8 or 9 and can't provide any feedback. After updating to dotnet 9 I cannot build anything. Once I get my mac's and pc's working with new dotnet I will see if any changes are needed.

@moljac
Copy link
Contributor

moljac commented Nov 14, 2024

Let me create repro sample for both net9.0 and net8.0, so we can narrow it down.

@moljac
Copy link
Contributor

moljac commented Nov 14, 2024

Archive.zip

Repro:

dotnet build net8.0/media3/media3.csproj -t:run -f:net8.0-android

OK!

dotnet build net9.0/media3/media3.csproj -t:run -f:net9.0-android

Crashes!!!

@ne0rrmatrix
Copy link

ne0rrmatrix commented Nov 14, 2024

SDK version needs to be set for dotnet 9.
Example:

{
   "sdk": 
   {
       "version": "9.0.100",
       "rollForward": "patch"
   },
   "msbuild-sdks": 
   {
       "MSBuild.Sdk.Extras": "3.0.44",
       "Microsoft.Build.Traversal": "4.1.0",
       "Microsoft.Build.NoTargets": "3.7.56",
       "Xamarin.Legacy.Sdk": "0.2.0-alpha4"
   }
}

Sample builds fine in Debug or Release mode when using current 9.0 version of dotnet from VS 2022 install.

edit: Sample build fine but does crash when app loads.

@moljac
Copy link
Contributor

moljac commented Nov 14, 2024

Sample builds fine in Debug or Release mode when using current 9.0 version of dotnet from VS 2022 install.

Yes. Builds fine, but crashes during running.

@jonpryor
Copy link
Member

I believe that this was broken by: 35f41dc

What I don't understand is why this wasn't caught by existing unit tests, e.g.

var c = JavaObjectExtensions.JavaCast<Java.Lang.IComparable> (o);

@moljac
Copy link
Contributor

moljac commented Nov 19, 2024

@ne0rrmatrix

Does your app crash during running [loading]?

Just to be certain

@ne0rrmatrix
Copy link

App crashes after launching activity. It also only crashes when exoplayer build() instruction is executed

@ne0rrmatrix
Copy link

To be specific it crashes only when executing Build(). If I run

var player = new ExoplayerBuilder(); // App does not crash. Return type is `ExoplayerBuilder`
player.Build(); // App crashes on this line. Return type is `IExoPlayer`

@moljac
Copy link
Contributor

moljac commented Nov 19, 2024

@ne0rrmatrix

Thanks a lot. Did you use repro sample provided in this issue?

Just asking, so other can use it for debugging.

thx

@ne0rrmatrix
Copy link

Yes I tried the sample app. It has exact same behavior. I also tested by modifying it like above to try and test if ExoPlayerBuilder was working. It is working for that class. It is only the Build() method that causes the crash.

@jpobst
Copy link
Contributor

jpobst commented Nov 20, 2024

Moving this to dotnet/android for better visibility/tracking, as initial investigation appears to be a .NET 9 regression in Java.Interop.

@jpobst jpobst transferred this issue from dotnet/android-libraries Nov 20, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-triage Issues that need to be assigned. label Nov 20, 2024
@jpobst jpobst added Area: Bindings Issues in Java Library Binding projects. and removed needs-triage Issues that need to be assigned. labels Nov 20, 2024
@jpobst jpobst added this to the .NET 9 Servicing milestone Nov 20, 2024
@jonpryor
Copy link
Member

@jonpryor wrote:

What I don't understand is why this wasn't caught by existing unit tests, e.g.

var c = JavaObjectExtensions.JavaCast<Java.Lang.IComparable> (o);

The answer is that before we try to do anything with *Invoker types, we first try to use an existing binding for the runtime type:

string? class_name = GetClassName (class_ptr);
lock (TypeManagerMapDictionaries.AccessLock) {
while (class_ptr != IntPtr.Zero && !TypeManagerMapDictionaries.JniToManaged.TryGetValue (class_name, out type)) {
type = GetJavaToManagedType (class_name);
if (type != null) {
TypeManagerMapDictionaries.JniToManaged.Add (class_name, type);
break;
}
IntPtr super_class_ptr = JNIEnv.GetSuperclass (class_ptr);
JNIEnv.DeleteLocalRef (class_ptr);
class_name = null;
class_ptr = super_class_ptr;
if (class_ptr != IntPtr.Zero) {
class_name = GetClassName (class_ptr);
}
}
}

Because the runtime type is java.lang.Integer, which is bound, we never actually hit the *Invoker codepath.

(The test wasn't testing what I thought it was testing!)

@jonpryor
Copy link
Member

For me, media3.zip works on .NET 9 when you build in Release configuration.

@ne0rrmatrix
Copy link

I can confirm it works in Release mode in .NET 9. It does not work in Debug mode though.

@jonpryor
Copy link
Member

…which brings us to the question: What's Happening™?

Answer: type maps. (Borderline "duh.") Which differ in structure and content between Debug and Release builds.

For reasons I'm still digging into, the Debug config type maps also differ between Mono.Android.dll and other assemblies. The type map for Mono.Android.dll contains duplicate entries for *Invoker types, e.g. from obj/Debug/net9.0-android/android/typemaps.arm64-v8a.ll:

@.TypeMapEntry.17434_from = private unnamed_addr constant [47 x i8] c"Java.Util.Functions.IIntSupplier, Mono.Android\00", align 1
@.TypeMapEntry.17435_to = private unnamed_addr constant [31 x i8] c"java/util/function/IntSupplier\00", align 1
@.TypeMapEntry.17436_from = private unnamed_addr constant [54 x i8] c"Java.Util.Functions.IIntSupplierInvoker, Mono.Android\00", align 1
…
  %struct.TypeMapEntry {
    ptr @.TypeMapEntry.17434_from, ; char* from
    ptr @.TypeMapEntry.17435_to; char* to
  }, ; 9766
  %struct.TypeMapEntry {
    ptr @.TypeMapEntry.17436_from, ; char* from
    ptr @.TypeMapEntry.17435_to; char* to
  }, ; 9767

It does not contain *Invoker entries for other assemblies:

@.TypeMapEntry.13525_from = private unnamed_addr constant [72 x i8] c"AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer\00", align 1
@.TypeMapEntry.13526_to = private unnamed_addr constant [36 x i8] c"androidx/media3/exoplayer/ExoPlayer\00", align 1
# no entry for IExoPlayerInvoker

though oddly the TypeMapEntry array does have a duplicate! With the same from and to values:

  %struct.TypeMapEntry {                                                        
    ptr @.TypeMapEntry.13525_from, ; char* from
    ptr @.TypeMapEntry.13526_to; char* to
  }, ; 7491
  %struct.TypeMapEntry {
    ptr @.TypeMapEntry.13525_from, ; char* from
    ptr @.TypeMapEntry.13526_to; char* to
  }, ; 7492

Open Questions:

  1. What's going on with Release builds that it works?

  2. Why does Mono.Android.dll get "duplicates" for *Invoker types while non-Mono.Android.dll assemblies don't?

  3. Should the type maps be updated to not contain the duplicates? (Impacts?)

  4. (Building upon (2) and (3)): what's the correct fix?

    1. Update type maps so that we reliably have duplicates?

    2. Update type maps so they reliably don't have duplicates, update TypeManager.CreateInstance() to appropriately deal with this (do type map lookup before determining Invoker type)

    3. Leave type maps alone, update TypeManager.CreateInstance() to appropriately deal with this (do type map lookup before determining Invoker type)

@jonpryor
Copy link
Member

Still digging, but this appears to be an ordering issue: duplicate detection appears to want the abstract class/interface declaration to appear before the Invoker definition, e.g.:

% monodis --typedef obj/Debug/net9.0-android/android/assets/arm64-v8a/Mono.Android.dll
…
7676: Java.Util.Functions.IIntSupplier (flist=57464, mlist=135763, flags=0x1000a1, extends=0x0)
7677: Java.Util.Functions.IIntSupplierInvoker (flist=57464, mlist=135764, flags=0x100000, extends=0x7d58)

This isn't the case with 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)

Note that the type definition for IExoPlayerInvoker comes before the definition for IExoPlayer.

This presumably happens because there is an in-tree (non-generated) partial class definition of IExoPlayerInvoker: https://github.com/dotnet/android-libraries/blob/3c21f4643c5dfe74e64fec54e3679baf4dc8d067/source/androidx.media3/media3-exoplayer/Additions/AndroidX.Media3.ExoPlayer.IExoPlayer.cs#L10

(Aside: why is all this generated code checked in?)

The cause of the bug is somewhere within TypeMapGenerator: with tons of extra logging, compare "normal" IIntSupplier (Invoker follows interface):

# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: entry={JavaName=java/util/function/IntSupplier, ManagedName=Java.Util.Functions.IIntSupplier, Mono.Android, SkipInJavaToManaged=True}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: entry={JavaName=java/util/function/IntSupplier, ManagedName=Java.Util.Functions.IIntSupplierInvoker, Mono.Android, SkipInJavaToManaged=False}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:   java/util/function/IntSupplier:
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:     TypeMapDebugEntry{JavaName=java/util/function/IntSupplier, ManagedName=Java.Util.Functions.IIntSupplier, Mono.Android, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=True, DuplicateForJavaToManaged=}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:     TypeMapDebugEntry{JavaName=java/util/function/IntSupplier, ManagedName=Java.Util.Functions.IIntSupplierInvoker, Mono.Android, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=False, DuplicateForJavaToManaged=TypeMapDebugEntry{JavaName=java/util/function/IntSupplier, ManagedName=Java.Util.Functions.IIntSupplier, Mono.Android, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=True, DuplicateForJavaToManaged=}}

to IExoPlayer:

# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: entry={JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer, SkipInJavaToManaged=False}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: entry={JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer, SkipInJavaToManaged=True}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:   androidx/media3/exoplayer/ExoPlayer:
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:     TypeMapDebugEntry{JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=False, DuplicateForJavaToManaged=}
# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:     TypeMapDebugEntry{JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=True, DuplicateForJavaToManaged=TypeMapDebugEntry{JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=False, DuplicateForJavaToManaged=}}

Note in particular the last two lines, which come from javaDuplicates after the SyncDebugDuplicates(javaDuplicates) call:

SyncDebugDuplicates (javaDuplicates);

With IExoPlayer, IExoPlayerInvoker isn't mentioned at all within javaDuplicates. Why?

@jonpryor
Copy link
Member

The bug appears to be within HandleDuplicates():

void HandleDebugDuplicates (Dictionary<string, List<TypeMapDebugEntry>> javaDuplicates, TypeMapDebugEntry entry, TypeDefinition td, TypeDefinitionCache cache)
{
List<TypeMapDebugEntry> duplicates;
if (!javaDuplicates.TryGetValue (entry.JavaName, out duplicates)) {
javaDuplicates.Add (entry.JavaName, new List<TypeMapDebugEntry> { entry });
} else {
duplicates.Add (entry);
TypeMapDebugEntry oldEntry = duplicates[0];
if (td.IsAbstract || td.IsInterface || oldEntry.TypeDefinition.IsAbstract || oldEntry.TypeDefinition.IsInterface) {
if (td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
oldEntry.TypeDefinition = td;
oldEntry.ManagedName = GetManagedTypeName (td);
}
}
}
}

@@ -309,8 +335,11 @@ namespace Xamarin.Android.Tasks
                                TypeMapDebugEntry oldEntry = duplicates[0];
                                if (td.IsAbstract || td.IsInterface || oldEntry.TypeDefinition.IsAbstract || oldEntry.TypeDefinition.IsInterface) {
                                        if (td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
+                                               log.LogDebugMessage ($"# jonp: !!! {td.FullName} IsAssignableFrom {oldEntry.TypeDefinition.FullName}");
+                                               log.LogDebugMessage ($"# jonp: !!! entry={oldEntry}");
                                                oldEntry.TypeDefinition = td;
                                                oldEntry.ManagedName = GetManagedTypeName (td);
+                                               log.LogDebugMessage ($"# jonp: !!! updated entry={oldEntry}");
                                        }
                                }
                        }

prints the following log messages:

# jonp: !!! AndroidX.Media3.ExoPlayer.IExoPlayer IsAssignableFrom AndroidX.Media3.ExoPlayer.IExoPlayerInvoker
# jonp: !!! entry=TypeMapDebugEntry{JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=False, DuplicateForJavaToManaged=}
# jonp: !!! updated entry=TypeMapDebugEntry{JavaName=androidx/media3/exoplayer/ExoPlayer, ManagedName=AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer, JavaIndex=0, ManagedIndex=0, SkipInJavaToManaged=False, DuplicateForJavaToManaged=}

IExoPlayerInvoker is "'removed".

Within the repro, there are two other types that trigger the above message::

# jonp: !!! Kotlin.Collections.AbstractList IsAssignableFrom Kotlin.Collections.AbstractListInvoker
# jonp: !!! Kotlin.Collections.AbstractSet IsAssignableFrom Kotlin.Collections.AbstractSetInvoker

Unsurprisingly, those also have partial class declarations within dotnet/android-libraries…

@jonpryor
Copy link
Member

jonpryor commented Nov 21, 2024

@grendello: what's the rationale for this code?

TypeMapDebugEntry oldEntry = duplicates[0];
if (td.IsAbstract || td.IsInterface || oldEntry.TypeDefinition.IsAbstract || oldEntry.TypeDefinition.IsInterface) {
if (td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
oldEntry.TypeDefinition = td;
oldEntry.ManagedName = GetManagedTypeName (td);
}
}

If I remove that code -- such that the else block is just duplicates.Add(entry) -- then the app builds and runs without a crash.

@jonpryor
Copy link
Member

jonpryor commented Nov 21, 2024

Commit a017561 appears to have added the HandleDebugDuplicates() logic. I think this is the explanation:

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.

@jonpryor
Copy link
Member

@jonpryor wrote:

If I remove that code -- such that the else block is just duplicates.Add(entry) -- then the app builds and runs without a crash.

It might not crash, but it's also not right either. From obj/Debug/net9.0-android/android/typemaps.arm64-v8a.ll:

@map_java_to_managed = internal dso_local constant [12070 x %struct.TypeMapEntry] [
	…
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13528_to, ; char* from
		ptr @.TypeMapEntry.13540_from; char* to
	}, ; 6921
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13528_to, ; char* from
		ptr @.TypeMapEntry.13540_from; char* to
	}, ; 6922
…
@.TypeMapEntry.13527_from = private unnamed_addr constant [72 x i8] c"AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer\00", align 1
@.TypeMapEntry.13528_to = private unnamed_addr constant [36 x i8] c"androidx/media3/exoplayer/ExoPlayer\00", align 1
…
@.TypeMapEntry.13540_from = private unnamed_addr constant [79 x i8] c"AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer\00", align 1

The first entry should be 13527_from, not 13540_from. As-is, there is no entry for IExoPlayer!

@jonpryor
Copy link
Member

jonpryor commented Nov 21, 2024

@grendello: my current attempted fix is:

diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs b/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs
index a6dbce277..91b552af3 100644
--- a/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs
+++ b/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs
@@ -77,6 +77,11 @@ namespace Xamarin.Android.Tasks
 			public TypeDefinition TypeDefinition;
 			public bool SkipInJavaToManaged;
 			public TypeMapDebugEntry DuplicateForJavaToManaged;
+
+			public override string ToString ()
+			{
+				return $"TypeMapDebugEntry{{JavaName={JavaName}, ManagedName={ManagedName}, JavaIndex={JavaIndex}, ManagedIndex={ManagedIndex}, SkipInJavaToManaged={SkipInJavaToManaged}, DuplicateForJavaToManaged={DuplicateForJavaToManaged}}}";
+			}
 		}
 
 		// Widths include the terminating nul character but not the padding!
@@ -170,6 +175,7 @@ namespace Xamarin.Android.Tasks
 
 		bool GenerateDebug (bool skipJniAddNativeMethodRegistrationAttributeScan, string outputDirectory, bool generateNativeAssembly)
 		{
+			log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebug: generateNativeAssembly: {generateNativeAssembly}");
 			if (generateNativeAssembly) {
 				return GenerateDebugNativeAssembly (skipJniAddNativeMethodRegistrationAttributeScan, outputDirectory);
 			}
@@ -192,11 +198,13 @@ namespace Xamarin.Android.Tasks
 
 			var javaDuplicates = new Dictionary<string, List<TypeMapDebugEntry>> (StringComparer.Ordinal);
 			foreach (TypeDefinition td in state.AllJavaTypes) {
+				log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles: td={td.FullName}");
 				UpdateApplicationConfig (td);
 				string moduleName = td.Module.Assembly.Name.Name;
 				ModuleDebugData module;
 
 				if (!modules.TryGetValue (moduleName, out module)) {
+					log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles: adding new module: {moduleName}");
 					string outputFileName = $"{moduleName}{TypemapExtension}";
 					module = new ModuleDebugData {
 						EntryCount = 0,
@@ -219,6 +227,8 @@ namespace Xamarin.Android.Tasks
 				}
 
 				TypeMapDebugEntry entry = GetDebugEntry (td, state.TypeCache);
+				log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles: entry={{JavaName={entry.JavaName}, ManagedName={entry.ManagedName}, SkipInJavaToManaged={entry.SkipInJavaToManaged}}}");
+
 				HandleDebugDuplicates (javaDuplicates, entry, td, state.TypeCache);
 				if (entry.JavaName.Length > module.JavaNameWidth)
 					module.JavaNameWidth = (uint)entry.JavaName.Length + 1;
@@ -230,6 +240,13 @@ namespace Xamarin.Android.Tasks
 				module.ManagedToJavaMap.Add (entry);
 			}
 			SyncDebugDuplicates (javaDuplicates);
+			log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles: javaDuplicates:");
+			foreach (var e in javaDuplicates) {
+				log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles:   {e.Key}:");
+				foreach (var entry in e.Value) {
+					log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugFiles:     {entry}");
+				}
+			}
 
 			foreach (ModuleDebugData module in modules.Values) {
 				PrepareDebugMaps (module);
@@ -251,6 +268,7 @@ namespace Xamarin.Android.Tasks
 
 		bool GenerateDebugNativeAssembly (bool skipJniAddNativeMethodRegistrationAttributeScan, string outputDirectory)
 		{
+			log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugNativeAssembly");
 			var javaToManaged = new List<TypeMapDebugEntry> ();
 			var managedToJava = new List<TypeMapDebugEntry> ();
 
@@ -259,12 +277,20 @@ namespace Xamarin.Android.Tasks
 				UpdateApplicationConfig (td);
 
 				TypeMapDebugEntry entry = GetDebugEntry (td, state.TypeCache);
+				log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: entry={{JavaName={entry.JavaName}, ManagedName={entry.ManagedName}, SkipInJavaToManaged={entry.SkipInJavaToManaged}}}");
 				HandleDebugDuplicates (javaDuplicates, entry, td, state.TypeCache);
 
 				javaToManaged.Add (entry);
 				managedToJava.Add (entry);
 			}
 			SyncDebugDuplicates (javaDuplicates);
+			log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugNativeAssembly: javaDuplicates:");
+			foreach (var e in javaDuplicates) {
+				log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:   Java type: {e.Key}:");
+				foreach (var entry in e.Value) {
+					log.LogDebugMessage ($"# jonp: TypeMapGenerator.GenerateDebugNativeAssembly:     {entry}");
+				}
+			}
 
 			var data = new ModuleDebugData {
 				EntryCount = (uint)javaToManaged.Count,
@@ -305,14 +331,30 @@ namespace Xamarin.Android.Tasks
 			if (!javaDuplicates.TryGetValue (entry.JavaName, out duplicates)) {
 				javaDuplicates.Add (entry.JavaName, new List<TypeMapDebugEntry> { entry });
 			} else {
-				duplicates.Add (entry);
 				TypeMapDebugEntry oldEntry = duplicates[0];
+				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;
+				} else {
+					// ¯\_(ツ)_/¯
+					duplicates.Add (entry);
+				}
+				#if false
 				if (td.IsAbstract || td.IsInterface || oldEntry.TypeDefinition.IsAbstract || oldEntry.TypeDefinition.IsInterface) {
 					if (td.IsAssignableFrom (oldEntry.TypeDefinition, cache)) {
+						log.LogDebugMessage ($"# jonp: !!! {td.FullName} IsAssignableFrom {oldEntry.TypeDefinition.FullName}");
+						log.LogDebugMessage ($"# jonp: !!! entry={oldEntry}");
 						oldEntry.TypeDefinition = td;
 						oldEntry.ManagedName = GetManagedTypeName (td);
+						log.LogDebugMessage ($"# jonp: !!! updated entry={oldEntry}");
 					}
 				}
+				#endif
 			}
 		}
 

The good news is that this appears to work.

The bad news is that despite working, the resulting typemaps.arm64-v8a.ll doesn't look right at all:

@map_managed_to_java = internal dso_local constant [12070 x %struct.TypeMapEntry] [
	…
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13527_from, ; char* from
		ptr @.TypeMapEntry.13528_to; char* to
	}, ; 7491
	…
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13540_from, ; char* from
		ptr @.TypeMapEntry.13528_to; char* to
	}, ; 7499
	…
], align 8

@map_java_to_managed = internal dso_local constant [12070 x %struct.TypeMapEntry] [
	…
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13528_to, ; char* from
		ptr null; char* to
	}, ; 6921
	%struct.TypeMapEntry {
		ptr @.TypeMapEntry.13528_to, ; char* from
		ptr null; char* to
	}, ; 6922
	…
], align 8

@.TypeMapEntry.13527_from = private unnamed_addr constant [72 x i8] c"AndroidX.Media3.ExoPlayer.IExoPlayer, Xamarin.AndroidX.Media3.ExoPlayer\00", align 1
@.TypeMapEntry.13528_to = private unnamed_addr constant [36 x i8] c"androidx/media3/exoplayer/ExoPlayer\00", align 1
…
@.TypeMapEntry.13540_from = private unnamed_addr constant [79 x i8] c"AndroidX.Media3.ExoPlayer.IExoPlayerInvoker, Xamarin.AndroidX.Media3.ExoPlayer\00", align 1

The map_managed_to_java entries look correct.

The map_java_to_managed entries confuse me, mapping 13528_to (androidx/media3/exoplayer/ExoPlayer) to null?!

But this appears to be what .NET 9 GA does with IIntSupplier, so maybe that's fine?

jonpryor added a commit that referenced this issue Nov 21, 2024
jonpryor added a commit that referenced this issue Nov 22, 2024
Fixes: #9535

Context: a017561
Context: 35f41dc

Create a new .NET for Android project (`dotnet new android`), add
a `@(PackageReference)` to [Xamarin.AndroidX.Media3.ExoPlayer][0],
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][1], 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.

[0]: https://www.nuget.org/packages/Xamarin.AndroidX.Media3.ExoPlayer/1.4.1
[1]: https://github.com/dotnet/android-libraries/blob/3c21f4643c5dfe74e64fec54e3679baf4dc8d067/source/androidx.media3/media3-exoplayer/Additions/AndroidX.Media3.ExoPlayer.IExoPlayer.cs#L10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Bindings Issues in Java Library Binding projects.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants