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

Bump to xamarin/java.interop/main@07c73009 #8681

Merged
merged 14 commits into from
Feb 2, 2024
Merged

Conversation

jonpryor
Copy link
Member

Context: dotnet/java-interop#1181
Context: #8625
Context: dotnet/java-interop@005c914

Does It Build™?

PR #8625 has turned into a cluster, probably because of dotnet/java-interop@005c9141, which implicitly requires typemap support for non-Java.Lang.Object subclasses such as the CallVirtualFromConstructorDerived test type.

Java.Interop#1181 updates typemap and JCW generation to support Java.Interop.JavaObject and Java.Interop.JavaException subclasses, which will hopefully allow the
CallVirtualFromConstructorDerived-using tests to work.

Context: dotnet/java-interop#1181
Context: #8625
Context: dotnet/java-interop@005c914

Does It Build™?

PR #8625 has turned into a cluster, *probably* because of
dotnet/java-interop@005c9141, which implicitly requires typemap
support for *non*-`Java.Lang.Object` subclasses such as
the `CallVirtualFromConstructorDerived` test type.

Java.Interop#1181 updates typemap and JCW generation to support
`Java.Interop.JavaObject` and `Java.Interop.JavaException`
subclasses, which will *hopefully* allow the
`CallVirtualFromConstructorDerived`-using tests to work.
@jonpryor
Copy link
Member Author

The (other) Story So Far: #8625 (comment)

  1. The binding for java/lang/Object is coming from Java.Interop-Tests, not Mono.Android (?!)
    [Java.Interop] Typemap support for JavaObject & [JniTypeSignature] java-interop#1181 has a fix for this, and we're not applying the fix yet because we believe that it will hide (1).

Welp, this PR is testing that fix, and it also crashes, so… Channeling #8625 (comment) but with:

adb shell setprop debug.mono.log "'default,assembly,debug,gc,timing,bundle,network,netlink,lref+,gref+'"

Now we're maybe getting somewhere?

I monodroid-lref: +l+ lrefc 1 handle 0x71/L from thread '(null)'(1)
D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.CreatedLocalReference(JniObjectReference , Int32& )
D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.CreatedLocalReference(JniEnvironmentInfo , JniObjectReference )
D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(JniObjectReference )
D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(IntPtr )
D monodroid-gref:    at Java.Interop.JniEnvironment.InstanceMethods.CallObjectMethod(JniObjectReference , JniMethodInfo )
D monodroid-gref:    at Android.Runtime.JNIEnv.CallObjectMethod(IntPtr , IntPtr )
D monodroid-gref:    at Android.Runtime.JavaSet.Iterator()
D monodroid-gref:    at Android.Runtime.JavaSet`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetEnumerator()
D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ProcessArguments()
D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnCreate(Bundle arguments)
D monodroid-gref:    at Android.App.Instrumentation.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_arguments)
D monodroid-gref:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0)
…
I monodroid-lref: -l- lrefc 0 handle 0x71/L from thread '(null)'(1)
D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.DeleteLocalReference(JniObjectReference& , Int32& )
D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.DeleteLocalReference(JniEnvironmentInfo , JniObjectReference& )
D monodroid-gref:    at Java.Interop.JniObjectReference.Dispose(JniObjectReference& reference)
D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteLocalRef(IntPtr )
D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteRef(IntPtr , JniHandleOwnership )
D monodroid-gref:    at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
D monodroid-gref:    at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
D monodroid-gref:    at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
D monodroid-gref:    at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
D monodroid-gref:    at Android.Runtime.JavaSet.Iterator()
D monodroid-gref:    at Android.Runtime.JavaSet`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetEnumerator()
D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ProcessArguments()
D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnCreate(Bundle arguments)
D monodroid-gref:    at Android.App.Instrumentation.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_arguments)
D monodroid-gref:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0)
D monodroid-gref: 
E droid.NET_Test: JNI ERROR (app bug): accessed stale Local 0x71  (index 7 in a table of size 7)

A crash ~immediately after the -l- 0x71 message. This is nice and clean log output.

I'll need to investigate further.

@jonpryor
Copy link
Member Author

Emulator log of the crash I'm looking at: log.txt

@jonpryor
Copy link
Member Author

Current conjecture is that things fail badly if the binding for java/lang/Object is not Java.Lang.Object, Mono.Android, and when Java.Interop.JavaObject subclasses participate in typemaps, we "lose control" of that mapping.

dotnet/java-interop@6b3637d attempted to avoid that.

It apparently failed. :-/

# in xamarin-android checkout
% ./dotnet-local.sh build -c Release tools/decompress-assemblies/*.csproj

# from wherever you have a `.aab` file…
% …/xamarin-android/bin/Release/bin/decompress-assemblies "../Mono.Android.NET_Tests-Signed.aab"

% ikdasm "uncompressed-Mono.Android.NET_Tests-Signed/Java.Interop-Tests.dll"
…
.class private auto ansi beforefieldinit Java.InteropTests.JavaLangRemappingTestObject
       extends [Java.Interop]Java.Interop.JavaObject
{
  .custom instance void [Java.Interop]Java.Interop.JniTypeSignatureAttribute::.ctor(string) = ( 01 00 10 6A 61 76 61 2F 6C 61 6E 67 2F 4F 62 6A   // ...java/lang/Obj
                                                                                                65 63 74 01 00 54 02 10 47 65 6E 65 72 61 74 65   // ect..T..Generate
                                                                                                4A 61 76 61 50 65 65 72 00 )                      // JavaPeer.
  .field static assembly literal string JniTypeName = "java/lang/Object"

Looks like I missed a type.

@jonpryor
Copy link
Member Author

… AND I FOUND THE PROBLEM. 🤦

https://github.com/xamarin/xamarin-android/blob/9798fb889ee251b65264b0b4637454a3c68ebe91/src/Mono.Android/Java.Interop/TypeManager.cs#L289-L294

Line 290, we call JNIEnv.DeleteRef(handle, transfer). This invalidates handle.

Line 292, we call JNIEnv.GetClassNameFromInstance (handle). THIS IS THE PROBLEM.

@jonpryor
Copy link
Member Author

The problem was introduced in https://github.com/xamarin/monodroid/commit/e3e4f123d8, in 2011-Oct-19, over 12 years ago, and we never encountered the broken error reporting.

/me cries.

Context: xamarin/monodroid@e3e4f123d8
Context: dotnet/java-interop@005c9141
Context: dotnet/java-interop#1181

We've been trying to track down a JNI error which occurs when trying
to use dotnet/java-interop@005c9141, resembling:

	I monodroid-lref: +l+ lrefc 1 handle 0x71/L from thread '(null)'(1)
	D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.CreatedLocalReference(JniObjectReference , Int32& )
	D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.CreatedLocalReference(JniEnvironmentInfo , JniObjectReference )
	D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(JniObjectReference )
	D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(IntPtr )
	D monodroid-gref:    at Java.Interop.JniEnvironment.InstanceMethods.CallObjectMethod(JniObjectReference , JniMethodInfo )
	D monodroid-gref:    at Android.Runtime.JNIEnv.CallObjectMethod(IntPtr , IntPtr )
	D monodroid-gref:    at Android.Runtime.JavaSet.Iterator()
	D monodroid-gref:    at Android.Runtime.JavaSet`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetEnumerator()
	D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ProcessArguments()
	D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnCreate(Bundle arguments)
	D monodroid-gref:    at Android.App.Instrumentation.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_arguments)
	D monodroid-gref:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0)
	…
	I monodroid-lref: -l- lrefc 0 handle 0x71/L from thread '(null)'(1)
	D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.DeleteLocalReference(JniObjectReference& , Int32& )
	D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.DeleteLocalReference(JniEnvironmentInfo , JniObjectReference& )
	D monodroid-gref:    at Java.Interop.JniObjectReference.Dispose(JniObjectReference& reference)
	D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteLocalRef(IntPtr )
	D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteRef(IntPtr , JniHandleOwnership )
	D monodroid-gref:    at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	D monodroid-gref:    at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	D monodroid-gref:    at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
	D monodroid-gref:    at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
	D monodroid-gref:    at Android.Runtime.JavaSet.Iterator()
	D monodroid-gref:    at Android.Runtime.JavaSet`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetEnumerator()
	D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ProcessArguments()
	D monodroid-gref:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].OnCreate(Bundle arguments)
	D monodroid-gref:    at Android.App.Instrumentation.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_arguments)
	D monodroid-gref:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0)
	D monodroid-gref:
	E droid.NET_Test: JNI ERROR (app bug): accessed stale Local 0x71  (index 7 in a table of size 7)
	F droid.NET_Test: java_vm_ext.cc:570] JNI DETECTED ERROR IN APPLICATION: use of deleted local reference 0x71
	…
	F droid.NET_Test: runtime.cc:630]   native: #13 pc 00000000003ce865  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::CheckJNI::GetObjectClass(_JNIEnv*, _jobject*)+837)

This has been "fun".

The problem:

 1. dotnet/java-interop@005c9141 relies on/requires additional
    typemaps in order to "fix" some linker warnings.

    This felt "fine" at the time.

 2. However, the Java.Interop *unit tests* which test (1) involve
    "hand-written" typemap entries to allow things to work.

 3. In .NET Android, those hand-written typemap entries aren't used;
    instead, the normal .NET Android typemaps are used.

 4. .NET Android typemaps did not contain entries for the types
    introduced in (2), so various tests started failing.

 5. dotnet/java-interop#1181 attempts to fix this by extending
    Java Callable Wrappers and associated typemaps to support
    `Java.Interop.JavaObject` subclasses, which brings the new types
    in (2) into the "normal" typemap fold.

 6. However, some of those types "alias" `java.lang.Object`, and --
    for some "bizarre" random ordering reason -- a type within
    `Java.Interop-Tests.dll` becomes the preferred `System.Type`
    to return when looking up `java/lang/Object`.

 7. Which would *probably* be okay (if *really* weird), except that
    `GetJavaToManagedType()` returns null when the binding is within
    an assembly that hasn't been loaded yet.  As this codepath is
    getting hit during app startup, `Java.Interop-Tests` hasn't been
    loaded, so `GetJavaToManagedType()` returns null.

 8. Which means we're now in the scenario of being unable to find a
    binding/"wrapper class" for `java/lang/Object`, which we consider
    to be an error state.

Because it's an error state, we dutifully throw.

…except we've never actually hit this error state before --
HOW COULD WE?! -- which means we've found a bug in our error handling.

Quick, find the problem!

	JNIEnv.DeleteRef (handle, transfer);
	throw new NotSupportedException (
			FormattableString.Invariant ($"Internal error finding wrapper class for '{JNIEnv.GetClassNameFromInstance (handle)}'. (Where is the Java.Lang.Object wrapper?!)"),
			CreateJavaLocationException ());

The problem is a "use after free" bug:
`JNIEnv.DeleteRef(handle, transfer)` *invalidates `handle`*, and then
*immediately* afterward we call
`JNIEnv.GetClassNameFromInstance(handle)`, on the now invalid value.

BOOM goes the Android runtime.

(The `DeleteRef()` call was introduced in xamarin/monodroid@e3e4f123d8,
on 2011-Oct-19.  Over 12 years to encounter this scenario!)

Unfortunately, *just* fixing the "use-after-free" bug is insufficient;
if we throw that `NotSupportedException`, things *will* break
elsewhere.  We'll just have an "elegant unhandled exception" app crash
instead of a "THE WORLD IS ENDING" failed assertion crash.

We could go with the simple fix for the crash, but this means that in
order to integrate dotnet/java-interop@005c9141 &
dotnet/java-interop#1181 we'd have to figure out how to *ensure* that
`java/lang/Object` is bound as `Java.Lang.Object, Mono.Android`, not
`Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`.

There may be a *slightly* more complicated fix which fixes both issues:
consider the `-l-` callstack:

	at Android.Runtime.JNIEnv.DeleteRef(IntPtr , JniHandleOwnership )
	at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
	at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
	at Android.Runtime.JavaSet.Iterator()

This is part of a generic `Object.GetObject<IIterator>()` invocation!
Additionally, because `IIterator` is an interface, in *normal* use
the `type` variable within `TypeManager.CreateInstance()` would be
`Java.Lang.Object, Mono.Android` and then ~immediately "discarded"
because `Java.Lang.Object` cannot be assigned to `IIterator`.

If we move the type compatibility check to *before* the
`type == null` check, we *may* also fix the
"`java/lang/Object` is bound as some unloadable type" issue.

Let's try that!
@jonpryor
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member Author

Progress! The tests no longer hit the JNI assert and crash; f0305ba does in fact fix the crash.

It's not perfect, though: we have unit test failures, e.g. Mono.Android.NET_Tests, Java.InteropTests.JnienvTest.NewOpenGenericTypeThrows / Release:

Java.Lang.ClassNotFoundException : crc641855b07eca6dcc03.GenericHolder_1
----> Java.Lang.ClassNotFoundException : Didn't find class "crc641855b07eca6dcc03.GenericHolder_1" on path: DexPathList[[zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk"],nativeLibraryDirectories=[/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/lib/x86_64, /system/fake-libs64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk!/lib/x86_64, /system/lib64, /system/product/lib64]]
   at Java.Interop.JniEnvironment.StaticMethods.CallStaticObjectMethod(JniObjectReference , JniMethodInfo , JniArgumentValue* )
   at Android.Runtime.JNIEnv.FindClass(String )
   at Android.Runtime.JNIEnv.FindClass(Type )
   at Android.Runtime.JNIEnv.AllocObject(Type )
   at Android.Runtime.JNIEnv.StartCreateInstance(Type , String , JValue* )
   at Android.Runtime.JNIEnv.StartCreateInstance(Type , String , JValue[] )
   at Java.InteropTests.JnienvTest.NewOpenGenericTypeThrows()
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
  --- End of managed Java.Lang.ClassNotFoundException stack trace ---
java.lang.ClassNotFoundException: crc641855b07eca6dcc03.GenericHolder_1
	at java.lang.Class.classForName(Native Method)
	at java.lang.Class.forName(Class.java:454)
	at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
	at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)
Caused by: java.lang.ClassNotFoundException: Didn't find class "crc641855b07eca6dcc03.GenericHolder_1" on path: DexPathList[[zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk"],nativeLibraryDirectories=[/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/lib/x86_64, /system/fake-libs64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk!/lib/x86_64, /system/lib64, /system/product/lib64]]
	at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:196)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:379)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:312)
	... 5 more
	Suppressed: java.io.IOException: No original dex files found for dex location /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk
		at dalvik.system.DexFile.openDexFileNative(Native Method)
		at dalvik.system.DexFile.openDexFile(DexFile.java:365)
		at dalvik.system.DexFile.<init>(DexFile.java:107)
		at dalvik.system.DexFile.<init>(DexFile.java:80)
		at dalvik.system.DexPathList.loadDexFile(DexPathList.java:444)
		at dalvik.system.DexPathList.makeDexElements(DexPathList.java:403)
		at dalvik.system.DexPathList.<init>(DexPathList.java:164)
		at dalvik.system.BaseDexClassLoader.<init>(BaseDexClassLoader.java:126)
		at dalvik.system.BaseDexClassLoader.<init>(BaseDexClassLoader.java:101)
		at dalvik.system.PathClassLoader.<init>(PathClassLoader.java:74)
		at com.android.internal.os.ClassLoaderFactory.createClassLoader(ClassLoaderFactory.java:87)
		at com.android.internal.os.ClassLoaderFactory.createClassLoader(ClassLoaderFactory.java:116)
		at android.app.ApplicationLoaders.getClassLoader(ApplicationLoaders.java:114)
		at android.app.ApplicationLoaders.getClassLoaderWithSharedLibraries(ApplicationLoaders.java:60)
		at android.app.LoadedApk.createOrUpdateClassLoaderLocked(LoadedApk.java:851)
		at android.app.LoadedApk.getClassLoader(LoadedApk.java:950)
		at android.app.LoadedApk.getResources(LoadedApk.java:1188)
		at android.app.ContextImpl.createAppContext(ContextImpl.java:2462)
		at android.app.ContextImpl.createAppContext(ContextImpl.java:2454)
		at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6343)
		at android.app.ActivityThread.access$1300(ActivityThread.java:219)
		at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1859)
		at android.os.Handler.dispatchMessage(Handler.java:107)
		at android.os.Looper.loop(Looper.java:214)

@jonpryor
Copy link
Member Author

jonpryor commented Jan 27, 2024

This is odd; I downloaded the .aab, looked at base/dex/classes.dex, and there is only one GenericHolder type:

% dexdump base/dex/classes.dex| grep 'Class desc.*GenericHolder'
  Class descriptor  : 'Lcrc647f71ae71536c47dc/GenericHolder_1;'

There should be two, with different package names:

Additionally, the xamarin/Java.Interop version explicitly sets the type name to net/dot/jni/test/tests/GenericHolder, which is missing from classes.dex.

So we have at least two underlying issues here:

  1. Where did net/dot/jni/test/tests/GenericHolder go?!
  2. Why does the typemap table contain crc641855b07eca6dcc03/GenericHolder_1, when crc641855b07eca6dcc03/GenericHolder_1 does not exist, and classes.dex instead has crc647f71ae71536c47dc/GenericHolder_1?

@jonpryor
Copy link
Member Author

@jonpryor asked:

  1. Where did net/dot/jni/test/tests/GenericHolder go?!

It didn't go anywhere; the C# declaration didn't contain [JniTypeSignature], so it wasn't actually setting the Java type name at all!

This can be further verified by using dexdump -d:

% dexdump -d base/dex/classes.dex
…
Class #50            -
  Class descriptor  : 'Lcrc647f71ae71536c47dc/GenericHolder_1;'
…
  Direct methods    -
    #0              : (in Lcrc647f71ae71536c47dc/GenericHolder_1;)
      name          : '<clinit>'
      type          : '()V'
      access        : 0x10008 (STATIC CONSTRUCTOR)
      code          -
      registers     : 3
      ins           : 0
      outs          : 3
      insns size    : 10 16-bit code units
010764:                                        |[010764] crc647f71ae71536c47dc.GenericHolder_1.<clinit>:()V
010774: 1a00 dd01                              |0000: const-string v0, "Java.InteropTests.GenericHolder`1, Java.Interop-Tests" // string@01dd
010778: 1c01 d401                              |0002: const-class v1, Lcrc647f71ae71536c47dc/GenericHolder_1; // type@01d4
01077c: 1a02 0000                              |0004: const-string v2, "" // string@0000
010780: 7130 fd01 1002                         |0006: invoke-static {v0, v1, v2}, Lmono/android/Runtime;.register:(Ljava/lang/String;Ljava/lang/Class;Ljava/lang/String;)V // method@01fd
010786: 0e00                                   |0009: return-void
      catches       : (none)
      positions     : 
        0x0006 line=14
      locals        : 

Note offset 010774, which declares Java.InteropTests.GenericHolder1, Java.Interop-Tests`.

This is fixed in dotnet/java-interop@18a7e02.

@jonpryor
Copy link
Member Author

@jonpryor asked a variation on:

  1. What's going on with crc641855b07eca6dcc03/GenericHolder_1?!

When I build locally, tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/obj/*/net9.0-android/acw-map.txt contains:

% grep 'crc641855b07eca6dcc03.GenericHolder_1' obj/Debug/net9.0-android/acw-map.txt
Java.InteropTests.GenericHolder`1, Mono.Android.NET-Tests;crc641855b07eca6dcc03.GenericHolder_1
Java.InteropTests.GenericHolder`1;crc641855b07eca6dcc03.GenericHolder_1
java.interoptests.GenericHolder_1;crc641855b07eca6dcc03.GenericHolder_1

Additionally, my local build of the project shows that classes.dex does contain crc641855b07eca6dcc03.GenericHolder_1:

% dexdump obj/Debug/net9.0-android/android/bin/classes.dex | grep 'Class desc.*crc641855b07eca6dcc03.GenericHolder_1'
  Class descriptor  : 'Lcrc641855b07eca6dcc03/GenericHolder_1;'

Additionally, the run-Mono.Android.NET_Tests-Release.binlog build log on CI contains:

Task "GenerateJavaStubs" (TaskId:801)
…
…/Microsoft.Android.Sdk.Darwin/34.99.0-ci.pr.gh8681.150/tools/Xamarin.Android.Common.targets(1486,3): warning XA4214: The managed type `Java.InteropTests.GenericHolder`1` exists in multiple assemblies: Java.Interop-Tests, Mono.Android.NET-Tests. Please refactor the managed type names in these assemblies so that they are not identical.
…/Microsoft.Android.Sdk.Darwin/34.99.0-ci.pr.gh8681.150/tools/Xamarin.Android.Common.targets(1486,3): warning XA4214: References to the type `Java.InteropTests.GenericHolder`1` will refer to `Java.InteropTests.GenericHolder`1, Java.Interop-Tests`.

which I think is good, followed by:

Added Item(s): 
    _JavaStubFiles=
        …
        obj/Release/net9.0-android/android/src/crc641855b07eca6dcc03/GenericHolder_1.java
        …
        obj/Release/net9.0-android/android/src/crc647f71ae71536c47dc/GenericHolder_1.java
…
04:08:50.144   1:7>Target "_CompileJava: (TargetId:1108)" … (target "_CompileToDalvik" depends on it):
Skipping target "_CompileJava" because all output files are up-to-date with respect to the input files.
Input files: 
    …
    obj/Release/net9.0-android/android/src/crc641855b07eca6dcc03/GenericHolder_1.java
    …
    obj/Release/net9.0-android/android/src/crc647f71ae71536c47dc/GenericHolder_1.java

In lots of places, when crc641855b07eca6dcc03/GenericHolder_1.java is mentioned, so is crc647f71ae71536c47dc/GenericHolder_1.java!

…but about that _CompileJava target. One of the above messages says that _CompileJava was skipped; what happened during that first invocation?

I don't know; the first invocation isn't in that build log. (?!)

Where is the first _CompileJava invocation?

We need to get the binlog for the *first* build of
`tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/*.csproj`.
Right now, it's auto-retrying, so we only capture the binlog for the
*second* build, and I need the first binlog to figure out what's
going wrong.

Temporarily disable retries.
Context: #8681 (comment)

For some reason we see `crc641855b07eca6dcc03/GenericHolder_1.java`
in the log output, part of `@(_JavaStubFiles)`, but as far as we can
tell it *isn't* being compiled.

Which makes no sense.

My best conjecture is that `@(_JavaBindingSource)` doesn't include
everything within `@(_JavaStubFiles)`.

Update the `<Javac/>` task to print out the paths of all files that
at added to the response file, so that we can see what `javac`
is *actually* compiling.
@jonpryor
Copy link
Member Author

@jonpryor asked:

Where is the first _CompileJava invocation?

Turns out we had "auto retry" enabled, to rerun a stage when it fails. The stage was failing, it was rerun, and the original logs were not preserved. Disabled auto-retries in 8dc95b5, which allowed us to get the first _CompileJava invocation, which looked (mostly) fine, with one oddity: the JavaCompileToolTask.JavaSourceFiles property wasn't printed in the diagnostic log output (?!).

Enter eacd356, which updates the <Javac/> task so that each file provided to JavaCompileToolTask.JavaSourceFiles is printed to the diagnostic log, and we see that obj/Release/net9.0-android/android/src/crc641855b07eca6dcc03/GenericHolder_1.java is provided to the Java compiler. This matches the crc641855b07eca6dcc03.GenericHolder_1 type which is missing, according to the unit test failure, and is also not present within classes.dex.

What?!

@jonpryor
Copy link
Member Author

Not sure what I did differently, but I now have a local build in which classes.dex is missing crc641855b07eca6dcc03/GenericHolder_1, even though it does exist within obj/….

@jonpryor
Copy link
Member Author

Behold the power of a complete build tree that has the issue!

% cd tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk
% grep -ril crc641855b07eca6dcc03.GenericHolder_1 | sort > has-crcgh.txt
% grep -ril GenericHolder | sort > has-gh.txt
% diff -u has-crcgh.txt has-gh.txt
--- has-crcgh.txt	2024-01-30 08:13:31
+++ has-gh.txt	2024-01-30 08:13:51
@@ -1,8 +1,25 @@
+./base/dex/classes.dex
+./has-crcgh.txt
 ./obj/Release/net9.0-android/Mono.Android.NET-Tests.csproj.FileListAbsolute.txt
+./obj/Release/net9.0-android/Mono.Android.NET-Tests.dll
 ./obj/Release/net9.0-android/acw-map.txt
+./obj/Release/net9.0-android/android-arm/linked/Java.Interop-Tests.dll
+./obj/Release/net9.0-android/android-arm/linked/Mono.Android.NET-Tests.dll
+./obj/Release/net9.0-android/android-arm/linked/shrunk/Java.Interop-Tests.dll
+./obj/Release/net9.0-android/android-arm/linked/shrunk/Mono.Android.NET-Tests.dll
+./obj/Release/net9.0-android/android-arm64/linked/Java.Interop-Tests.dll
+./obj/Release/net9.0-android/android-arm64/linked/Mono.Android.NET-Tests.dll
+./obj/Release/net9.0-android/android-x64/linked/Java.Interop-Tests.dll
+./obj/Release/net9.0-android/android-x64/linked/Mono.Android.NET-Tests.dll
+./obj/Release/net9.0-android/android-x86/linked/Java.Interop-Tests.dll
+./obj/Release/net9.0-android/android-x86/linked/Mono.Android.NET-Tests.dll
 ./obj/Release/net9.0-android/android/bin/Mono.Android.NET_Tests.apks
+./obj/Release/net9.0-android/android/bin/classes.dex
 ./obj/Release/net9.0-android/android/bin/classes.zip
 ./obj/Release/net9.0-android/android/bin/classes/crc641855b07eca6dcc03/GenericHolder_1.class
+./obj/Release/net9.0-android/android/bin/classes/net/dot/jni/test/tests/GenericHolder.class
+./obj/Release/net9.0-android/android/src/crc641855b07eca6dcc03/GenericHolder_1.java
+./obj/Release/net9.0-android/android/src/net/dot/jni/test/tests/GenericHolder.java
 ./obj/Release/net9.0-android/android/typemaps.arm64-v8a.ll
 ./obj/Release/net9.0-android/android/typemaps.arm64-v8a.o
 ./obj/Release/net9.0-android/android/typemaps.armeabi-v7a.ll
@@ -15,3 +32,4 @@
 ./obj/Release/net9.0-android/app_shared_libraries/armeabi-v7a/libxamarin-app.so
 ./obj/Release/net9.0-android/app_shared_libraries/x86/libxamarin-app.so
 ./obj/Release/net9.0-android/app_shared_libraries/x86_64/libxamarin-app.so
+./obj/Release/net9.0-android/proguard/proguard_project_primary.cfg

Many of those changes are to be expected: Mono.Android.NET-Tests.dll contains a GenericHolder<T> type, but not crc641855b07eca6dcc03/GenericHolder_1.

What surprises me is obj/Release/net9.0-android/proguard/proguard_project_primary.cfg:

-keep class net.dot.jni.test.JavaDisposedObject { *; }
-keep class net.dot.jni.test.MyDisposableObject { *; }
-keep class net.dot.jni.test.tests.GenericHolder { *; }
-keep class crc6456ab8145c81c4100.CustomTextView { *; }
-keep class crc643df67da7b13bb6b1.TestInstrumentation_1 { *; }
-keep class crc64682b60dcf97b305c.NUnitTestInstrumentation { *; }
-keep class crc642cb891c3ff69e7c2.OptionsActivity { *; }
-keep class crc642cb891c3ff69e7c2.TestResultActivity { *; }
-keep class crc642cb891c3ff69e7c2.TestSuiteActivity { *; }
-keep class crc642cb891c3ff69e7c2.TestSuiteActivity_TestDataAdapter { *; }
-keep class crc642cb891c3ff69e7c2.TestSuiteActivity_TestData { *; }
-keep class crc642cb891c3ff69e7c2.TestSuiteInstrumentation { *; }
-keep class xamarin.android.runtimetests.MainActivity { *; }
-keep class crc6485ecb990e61befa6.MyFragment { *; }
-keep class crc6485ecb990e61befa6.MyIntent { *; }
-keep class xamarin.android.runtimetests.NUnitInstrumentation { *; }
-keep class crc6485ecb990e61befa6.CustomButton { *; }
-keep class com.xamarin.android.runtimetests.CreateInstance_OverrideAbsListView_Adapter { *; }
-keep class crc64f5a8f3fc0b61af8b.MyDisposableObject { *; }
-keep class from.NewThreadOne { *; }
-keep class from.NewThreadTwo { *; }
-keep class crc641855b07eca6dcc03.MyRegistrationThread { *; }
-keep class crc641855b07eca6dcc03.MyCb { *; }
-keep class crc641855b07eca6dcc03.ContainsExportedMethods { *; }
-keep class crc641855b07eca6dcc03.ThrowableActivatedFromJava { *; }
-keep class crc641855b07eca6dcc03.MyPaint { *; }
-keep class crc641855b07eca6dcc03.MyObject { *; }
-keep class crc64b06ada0299a6bcca.CanOverrideAbsListView_Adapter { *; }
-keep class crc64f5183e500568449d.App { *; }
-keep class crc64f5183e500568449d.RenamedActivity { *; }

crc641855b07eca6dcc03.GenericHolder_1 isn't in there! Why?

@jonpryor
Copy link
Member Author

@jonpryor asked:

crc641855b07eca6dcc03.GenericHolder_1 isn't in there! Why?

proguard_project_primary.cfg is created by <R8/>:

https://github.com/xamarin/xamarin-android/blob/eacd356cfed39fc8ba227376c86578af5c04fa11/src/Xamarin.Android.Build.Tasks/Tasks/R8.cs#L86-L96

What's interesting is that for loop: we only process every three lines, e.g.

Mono.Android_Test.Library.CustomTextView, Mono.Android-Test.Library.NET;crc6456ab8145c81c4100.CustomTextView
Mono.Android_Test.Library.CustomTextView;crc6456ab8145c81c4100.CustomTextView
mono.android_test.library.CustomTextView;crc6456ab8145c81c4100.CustomTextView

The problem is, not every set of entries takes three lines, particularly when warning XA4214 is emitted. For crc641855b07eca6dcc03.GenericHolder_1, acw-map.txt contains one line:

Java.InteropTests.GenericHolder`1, Mono.Android.NET-Tests;crc641855b07eca6dcc03.GenericHolder_1

As such, it Just Happens™ to be skipped.

Possibly "worse", it means that everything after that is off-by-one. That might not be an actual problem, but is mildly distressing.

`acw-map.txt` contains mappings from .NET types to Java types,
and implicitly vice-versa; see (TODO commit).

*Normally* it contains three entries:

 1. The fully-qualified .NET type name
 2. The .NET type name, no assembly
 3. (2) with a lowercased namespace name.

For example:

	Mono.Android_Test.Library.CustomTextView, Mono.Android-Test.Library.NET;crc6456ab8145c81c4100.CustomTextView
	Mono.Android_Test.Library.CustomTextView;crc6456ab8145c81c4100.CustomTextView
	mono.android_test.library.CustomTextView;crc6456ab8145c81c4100.CustomTextView

However, when XA4214 is emitted, there is a "collision" on the
.NET side (but *not* the Java side); (2) and (3) are *ambiguous*,
so one .NET type is arbitrarily chosen.

The first line is still possible, because of assembly qualification.

Enter ``Java.InteropTests.GenericHolder`1``: this type is present in
*both* `Java.Interop-Tests.dll` *and* `Mono.Android-Tests.dll`.
Before dotnet/java-interop#1181, this was "fine" because the
`GenericHolder<T>` within `Java.Interop-Tests.dll` did not participate
in typemap generation.  Now it does, resulting in the XA4214.
XA4214 *also* means that instead of three lines, it's *one* line:

	Java.InteropTests.GenericHolder`1, Mono.Android.NET-Tests;crc641855b07eca6dcc03.GenericHolder_1

Enter `<R8/>`, which parses `acw-map.txt` to create a
`proguard_project_primary.cfg` file.  `<R8/>` did it's *own* parsing
of `acw-map.txt`, parsing only *one of every three lines*, on the
assumption that *all* entries took three lines.

This breaks in the presence of XA4214, because some entries only take
one line, not three lines.

Update `<R8/>` to instead use `MonoAndroidHelper.LoadMapFile()`,
which reads all lines within `acw-map.txt`.  This should result in
a `proguard_project_primary.cfg` file which properly contains a
`-keep` entry for `crc641855b07eca6dcc03.GenericHolder_1`.
@jonpryor
Copy link
Member Author

jonpryor commented Jan 31, 2024

WIP commit message:

Context: https://github.com/xamarin/java.interop/issues/1165
Context: https://github.com/xamarin/java.interop/commit/005c914170a0af9069ff18fd4dd9d45463dd5dc6
Context: https://github.com/xamarin/xamarin-android/pull/8543
Context: https://github.com/xamarin/java.interop/commit/07c73009571d3b5d36ae79d0d4d69a02062dd6e8
Context: https://github.com/xamarin/xamarin-android/pull/8625
Context: https://github.com/xamarin/monodroid/commit/e3e4f123d8e6c7ca34fb6d9b96a6710a3a877eca
Context: https://github.com/xamarin/monodroid/commit/a04b73b30e5bbd81b3773865c313a8084f0a21d1
Context: efbec22767427f5704b6b20b3b36ff6c3c7067f2

Changes: https://github.com/xamarin/java.interop/compare/8b85462e5f304d9aa2d91c02966d7dc9751516c7...07c73009571d3b5d36ae79d0d4d69a02062dd6e8

  * xamarin/java.interop@07c73009: [Java.Interop] Typemap support for JavaObject & `[JniTypeSignature]` (xamarin/java.interop#1181)
  * xamarin/java.interop@d529f3be: Bump to xamarin/xamarin-android-tools/main@ed102fc (xamarin/java.interop#1182)
  * xamarin/java.interop@def5bc0d: [ci] Add API Scan job (xamarin/java.interop#1178)
  * xamarin/java.interop@d5afa0af: [invocation-overhead] Add generated source files (xamarin/java.interop#1175)
  * xamarin/java.interop@473ef74c: Bump to xamarin/xamarin-android-tools/main@4889bf0 (xamarin/java.interop#1172)
  * xamarin/java.interop@005c9141: [Java.Interop] Avoid `Type.GetType()` in `ManagedPeer` (xamarin/java.interop#1168)
  * xamarin/java.interop@0f1efebd: [Java.Interop] Use PublicApiAnalyzers to ensure we do not break API (xamarin/java.interop#1170)

(From the "infinite scream" department…)

It started with a desire to remove some linker warnings
(xamarin/java.interop#1165):

	external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs(93,19,93,112):
	warning IL2057: Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String, Boolean)'.
	It's not possible to guarantee the availability of the target type.

xamarin/java.interop@005c9141 attempted to fix this by requiring the
use of "typemaps" mapping Java type signatures to managed types,
replacing e.g.:

	Type            type            = Type.GetType ("Example.Type, AssemblyName", throwOnError: true)!;
	Type[]          parameterTypes  = GetParameterTypes ("System.Int32:System.Int32");
	ConstructorInfo ctor            = type.GetConstructor (ptypes);
	// ctor=Example.Type(int, int) constructor

with (not exactly, but for expository purposes):

	Type            type            = GetTypeFromSignature("crc64…/Type");
	Type[]          parameterTypes  = GetConstructorCandidateParameterTypes ("(II)V");
	ConstructorInfo ctor            = type.GetConstructor (ptypes);
	// ctor=Example.Type(int, int) constructor
	
among other changes.

This was a *significant* change that would alter *Java.Interop*
semantics but *not* .NET Android semantics -- .NET Android uses
`Java.Interop.TypeManager.n_Activate()` (in this repo) for Java-side
"activation" scenarios, not `Java.Interop.ManagedPeer` -- so in an
abundance of caution we did a manual integration test in
xamarin/xamarin-android#8543 to make sure nothing broke before
merging it.

Something was apparently "off" in that integration.  (We're still not
sure what was off, or why it was completely green.)

Ever since xamarin/java.interop@005c9141 was merged, every attempt to
bump xamarin/Java.Interop has failed, in a number of ways described
below.  However, instead of reverting xamarin/java.interop@005c9141
we took this as an opportunity to understand *how and why* things
were failing, as apparently we had encountered some *long-standing*
corner cases in How Things Work.

The oversights and failures include:

 1. In order to make the Java.Interop unit tests work in .NET Android,
    the (largely hand-written) Java.Interop test types *also* need to
    participate with .NET Android typemap support, so that there is a
    typemap entry mapping `net/dot/jni/test/GenericHolder` to
    `Java.InteropTests.GenericHolder<T>` and vice-versa.

    xamarin/java.interop@07c73009 updates
    `Java.Interop.Tools.JavaCallableWrappers` to support creating
    typemap entries for `Java.Interop.JavaObject` subclasses,
    introducing a new `TypeDefinition.HasJavaPeer()` extension method.

 2. (1) meant that, for the first time ever, types in
    `Java.Interop-Tests` participated in .NET Android type mapping.
    This *sounds* fine, except that `Java.Interop-Tests` contains
    "competing bindings" for `java.lang.Object`:

        [JniTypeSignature ("java/lang/Object", GenerateJavaPeer=false)]
        partial class JavaLangRemappingTestObject : JavaObject {
        }

 3. (2) means that, for the first time ever, we *could* have the
    typemap entry for `java/lang/Object` map to
    `Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`,
    *not* `Java.Lang.Object, Mono.Android`.

    Arguably a bug, arguably "meh", but this setup triggered some
    never previously encountered error conditions:

 4. `EmbeddedAssemblies::typemap_java_to_managed()` within
    `libmonodroid.so` returns a `System.Type` that corresponds to a
    JNI type.  `typemap_java_to_managed()` has a bug/corner case
    wherein it will only provide `Type` instances from assemblies
    which have already been loaded.

    Early in startup, `Java.Interop-Tests` hasn't been loaded yet, so
    when `java/lang/Object` was mapped to
    `Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`,
    `typemap_java_to_managed()` would return `null`.

    This is a bug/corner case, which is being investigated in
    xamarin/xamarin-android#8625.

 5. Calls to `Java.Lang.Object.GetObject<T>()` call
    `Java.Interop.TypeManager.CreateInstance()`, which loops through
    the type and all base types to find a known binding/wrapper.
    Because of (3)+(4), if (when) we try to find the wrapper for
    `java/lang/Object`, we would find *no* mapping.

    This would cause an `JNI DETECTED ERROR IN APPLICATION` *crash*.

    This was due to a "use after free" bug.

    See the "TypeManager.CreateInstance() Use After Free Bug" section.

 6. Once (5) is fixed we encounter our next issue: the
    `Java.InteropTests.JnienvTest.NewOpenGenericTypeThrows()` unit
    test started failing because
    `crc641855b07eca6dcc03.GenericHolder_1` couldn't be found.

    This was caused by a bug in `acw-map.txt` parsing within `<R8/>`.

    See the "`<R8/>` and `acw-map.txt` parsing.`" section.

 7. Once (6) was fixed, (3) caused a *new* set of failures:
    multiple tests started failing because `java/lang/Object` was
    being mapped to the wrong managed type.

    (3) becomes less "meh" and more "definitely a bug".

    See the "Correct `java/lang/Object` mappings" section.

*Now* things should work reliably.


~~ TypeManager.CreateInstance() Use After Free Bug ~~

On 2011-Oct-19, xamarin/monodroid@e3e4f123d8 introduced a
use-after-free bug within `TypeManager.CreateInstance()`:

	JNIEnv.DeleteRef (handle, transfer);
	throw new NotSupportedException (
	        FormattableString.Invariant ($"Internal error finding wrapper class for '{JNIEnv.GetClassNameFromInstance (handle)}'. (Where is the Java.Lang.Object wrapper?!)"),
	        CreateJavaLocationException ());

`handle` *cannot be used* after `JNIEnv.DeleteRef(handle)`.
Failure to do so results in a `JNI DETECTED ERROR IN APPLICATION`
crash; with `adb shell setprop debug.mono.log lref+` set, we see:

	I monodroid-lref: +l+ lrefc 1 handle 0x71/L from thread '(null)'(1)
	D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.CreatedLocalReference(JniObjectReference , Int32& )
	D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.CreatedLocalReference(JniEnvironmentInfo , JniObjectReference )
	D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(JniObjectReference )
	D monodroid-gref:    at Java.Interop.JniEnvironment.LogCreateLocalRef(IntPtr )
	D monodroid-gref:    at Java.Interop.JniEnvironment.InstanceMethods.CallObjectMethod(JniObjectReference , JniMethodInfo )
	D monodroid-gref:    …
	
	I monodroid-lref: -l- lrefc 0 handle 0x71/L from thread '(null)'(1)
	D monodroid-gref:    at Android.Runtime.AndroidObjectReferenceManager.DeleteLocalReference(JniObjectReference& , Int32& )
	D monodroid-gref:    at Java.Interop.JniRuntime.JniObjectReferenceManager.DeleteLocalReference(JniEnvironmentInfo , JniObjectReference& )
	D monodroid-gref:    at Java.Interop.JniObjectReference.Dispose(JniObjectReference& reference)
	D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteLocalRef(IntPtr )
	D monodroid-gref:    at Android.Runtime.JNIEnv.DeleteRef(IntPtr , JniHandleOwnership )
	D monodroid-gref:    at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	D monodroid-gref:    at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	D monodroid-gref:    at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
	D monodroid-gref:    at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
	D monodroid-gref:    …
	D monodroid-gref:
	E droid.NET_Test: JNI ERROR (app bug): accessed stale Local 0x71  (index 7 in a table of size 7)
	F droid.NET_Test: java_vm_ext.cc:570] JNI DETECTED ERROR IN APPLICATION: use of deleted local reference 0x71
	
	F droid.NET_Test: runtime.cc:630]   native: #13 pc 00000000003ce865  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::CheckJNI::GetObjectClass(_JNIEnv*, _jobject*)+837)

The immediate fix is Don't Do That™; use a temporary:

	class_name = JNIEnv.GetClassNameFromInstance (handle);
	JNIEnv.DeleteRef (handle, transfer);
	throw new NotSupportedException (
	        FormattableString.Invariant ($"Internal error finding wrapper class for '{class_name}'. (Where is the Java.Lang.Object wrapper?!)"),
	        CreateJavaLocationException ());

Unfortunately, *just* fixing the "use-after-free" bug is insufficient;
if we throw that `NotSupportedException`, things *will* break
elsewhere.  We'll just have an "elegant unhandled exception" app crash
instead of a "THE WORLD IS ENDING" failed assertion crash.

We could go with the simple fix for the crash, but this means that in
order to integrate xamarin/java.interop@005c9141 &
xamarin/java.interop@07c73009 we'd have to figure out how to *ensure*
that `java/lang/Object` is bound as `Java.Lang.Object, Mono.Android`,
not `Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`.
(We actually need to do this *anyway*; see the
"Correct `java/lang/Object` mappings" section.  At the time we I was
trying to *avoid* special-casing `Mono.Android.dll`…)

There is a*slightly* more complicated approach which fixes (5)
while supporting (4) `typemap_java_to_managed()` returning null;
consider the `-l-` callstack:

	at Android.Runtime.JNIEnv.DeleteRef(IntPtr , JniHandleOwnership )
	at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
	at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
	at Android.Runtime.JavaSet.Iterator()

This is part of a generic `Object.GetObject<IIterator>()` invocation!
Additionally, because `IIterator` is an interface, in *normal* use
the `type` variable within `TypeManager.CreateInstance()` would be
`Java.Lang.Object, Mono.Android` and then *immediately discarded*
because `Java.Lang.Object` cannot be assigned to `IIterator`.

Moving the type compatibility check to *before* the
`type == null` check fixes *an* issue with `typemap_java_to_managed()`
returning null.


~~ `<R8/>` and `acw-map.txt` parsing.` ~~

There are many ways for Android+Java code to refer to managed types.

For example, consider the following View subclass:

	namespace Example {
	  partial class MyCoolView : Android.Views.View {
	    // …
	  }
	}

Within layout `.axml` files, you can mention an `Android.Views.View`
subclass by:

  * Using the .NET Full Class Name as an element name.

        <Example.MyCoolView />

  * Using the .NET Full Class Name with a *lowercased* namespace
    name as the element name.

        <example.MyCoolView />

  * Use the Java-side name directly.

        <crc64….NiftyView />

Within Fragments, you can also use the *assembly-qualified name*:

	<fragment class="Example.MyCoolView, AssemblyName" />

At build time, all instances of the .NET type names will be
*replaced* with the Java type names before the Android toolchain
processes the files.

The association between .NET type names and Java names is stored
within `$(IntermediateOutputPath)acw-map.txt`, which was introduced
in xamarin/monodroid@a04b73b3.

*Normally* `acw-map.txt` contains three entries:

 1. The fully-qualified .NET type name
 2. The .NET type name, no assembly
 3. (2) with a lowercased namespace name, *or* the `[Register]`
    value, if provided.

For example:

	Mono.Android_Test.Library.CustomTextView, Mono.Android-Test.Library.NET;crc6456ab8145c81c4100.CustomTextView
	Mono.Android_Test.Library.CustomTextView;crc6456ab8145c81c4100.CustomTextView   
	mono.android_test.library.CustomTextView;crc6456ab8145c81c4100.CustomTextView   
	Java.InteropTests.GenericHolder`1, Java.Interop-Tests;net.dot.jni.test.tests.GenericHolder
	Java.InteropTests.GenericHolder`1;net.dot.jni.test.tests.GenericHolder          
	net.dot.jni.test.tests.GenericHolder;net.dot.jni.test.tests.GenericHolder    

However, when warning XA4214 is emitted (efbec227), there is a
"collision" on the .NET side (but *not* the Java side); (2) and (3)
are potentially *ambiguous*, so one .NET type is arbitrarily chosen.
(Collisions on the Java side result in XA4215 *errors*.)

The first line is still possible, because of assembly qualification.

Enter ``Java.InteropTests.GenericHolder`1``: this type is present in
*both* `Java.Interop-Tests.dll` *and* `Mono.Android-Tests.dll`.
xamarin/java.interop@07c73009, this was "fine" because the
`GenericHolder<T>` within `Java.Interop-Tests.dll` did not participate
in typemap generation.  Now it does, resulting in the XA4214 warning.
XA4214 *also* means that instead of three lines, it's *one* line:

	Java.InteropTests.GenericHolder`1, Mono.Android.NET-Tests;crc641855b07eca6dcc03.GenericHolder_1

Enter `<R8/>`, which parses `acw-map.txt` to create a
`proguard_project_primary.cfg` file.  `<R8/>` did it's *own* parsing
of `acw-map.txt`, parsing only *one of every three lines*, on the
assumption that *all* entries took three lines.

This breaks in the presence of XA4214, because some entries only take
one line, not three lines.  This in turn meant that
`proguard_project_primary.cfg` could *miss* types, which could mean
that `r8` would *remove* the unspecified types, resulting in
`ClassNotFoundException` at runtime:

	Java.Lang.ClassNotFoundException : crc641855b07eca6dcc03.GenericHolder_1
	----> Java.Lang.ClassNotFoundException : Didn't find class "crc641855b07eca6dcc03.GenericHolder_1" on path: DexPathList[[zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk", zip file "/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk"],nativeLibraryDirectories=[/data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/lib/x86_64, /system/fake-libs64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/base.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.x86_64.apk!/lib/x86_64, /data/app/Mono.Android.NET_Tests-2stBqO43ov5F6bHfYemJHQ==/split_config.xxhdpi.apk!/lib/x86_64, /system/lib64, /system/product/lib64]]
	   at Java.Interop.JniEnvironment.StaticMethods.CallStaticObjectMethod(JniObjectReference , JniMethodInfo , JniArgumentValue* )
	   at Android.Runtime.JNIEnv.FindClass(String )

Update `<R8/>` to instead use `MonoAndroidHelper.LoadMapFile()`,
which reads all lines within `acw-map.txt`.  This results in a
`proguard_project_primary.cfg` file which properly contains a `-keep`
entry for XA4214-related types, such as
`crc641855b07eca6dcc03.GenericHolder_1`.


~~ Correct `java/lang/Object` mappings ~~`

Previous valiant efforts to allow `java/lang/Object` to be mapped to
"anything", not just `Java.Lang.Object, Mono.Android`, eventually
resulted in lots of unit test failures, e.g.:

`Android.RuntimeTests.XmlReaderPullParserTest.ToLocalJniHandle()`:

	System.NotSupportedException : Unable to activate instance of type Java.InteropTests.JavaLangRemappingTestObject from native handle 0x19 (key_handle 0x2408476).
	----> System.MissingMethodException : No constructor found for Java.InteropTests.JavaLangRemappingTestObject::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership)
	----> Java.Interop.JavaLocationException : Exception_WasThrown, Java.Interop.JavaLocationException
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership )
	   at Android.Runtime.XmlResourceParserReader.FromNative(IntPtr , JniHandleOwnership )
	   at Android.Runtime.XmlResourceParserReader.FromJniHandle(IntPtr handle, JniHandleOwnership transfer)
	   at Android.Content.Res.Resources.GetXml(Int32 )
	   at Android.RuntimeTests.XmlReaderPullParserTest.ToLocalJniHandle()
	   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
	   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
	--MissingMethodException
	   at Java.Interop.TypeManager.CreateProxy(Type , IntPtr , JniHandleOwnership )
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )

With a partially heavy heart, we need to special-case typemap entries
by processing `Mono.Android.dll` *first*, so that it gets first dibs
at bindings for `java/lang/Object` and other types.

Update `NativeTypeMappingData` to process types from `Mono.Android`
before processing any other module.

Note that the special-casing needs to happen in `NativeTypeMappingData`
because typemaps were formerly processed in *sorted module order*, in
which the sort order is based on the *byte representation* of the
module's MVID (a GUID).  Additionally, *linking changes the MVID*,
which means module order is *effectively random*.  Consequently,
trying to special case typemap ordering anywhere else is ineffective.


~~ Other ~~

Update `JavaCompileToolTask` to log the contents of its response file.

Update LLVM-IR -related types within
`src/Xamarin.Android.Build.Tasks/Utilities` to use `TaskLoggingHelper`
for logging purposes, *not* `Action<string>`.  Update related types
to accept `TaskLoggingHelper`, so that we can more easily add
diagnostic messages to these types in the future.

Shouldn't change anything on this side…
The remaining unit test failures are because `java/lang/Object`
is bound as
`Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`,
NOT `Java.Lang.Object, Mono.Android`.

`Android.RuntimeTests.XmlReaderPullParserTest.ToLocalJniHandle()`:

	System.NotSupportedException : Unable to activate instance of type Java.InteropTests.JavaLangRemappingTestObject from native handle 0x19 (key_handle 0x2408476).
	----> System.MissingMethodException : No constructor found for Java.InteropTests.JavaLangRemappingTestObject::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership)
	----> Java.Interop.JavaLocationException : Exception_WasThrown, Java.Interop.JavaLocationException
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership )
	   at Android.Runtime.XmlResourceParserReader.FromNative(IntPtr , JniHandleOwnership )
	   at Android.Runtime.XmlResourceParserReader.FromJniHandle(IntPtr handle, JniHandleOwnership transfer)
	   at Android.Content.Res.Resources.GetXml(Int32 )
	   at Android.RuntimeTests.XmlReaderPullParserTest.ToLocalJniHandle()
	   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
	   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
	--MissingMethodException
	   at Java.Interop.TypeManager.CreateProxy(Type , IntPtr , JniHandleOwnership )
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	--JavaLocationException
	Java.Lang.Error: Exception_WasThrown, Java.Lang.Error

	  --- End of managed Java.Lang.Error stack trace ---
	java.lang.Error: Java callstack:
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

	  --- End of managed Java.Lang.Error stack trace ---
	java.lang.Error: Java callstack:
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

`Java.InteropTests.JniRuntimeJniValueManagerTests.CreateValue()`:

	System.NotSupportedException : Unable to activate instance of type Java.InteropTests.JavaLangRemappingTestObject from native handle 0x3432 (key_handle 0x9d3f1d1).
	----> System.MissingMethodException : No constructor found for Java.InteropTests.JavaLangRemappingTestObject::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership)
	----> Java.Interop.JavaLocationException : Exception_WasThrown, Java.Interop.JavaLocationException
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	   at Android.Runtime.AndroidValueManager.CreatePeer(JniObjectReference& , JniObjectReferenceOptions , Type )
	   at Java.Interop.JavaPeerableValueMarshaler.CreateGenericValue(JniObjectReference& , JniObjectReferenceOptions , Type )
	   at Java.Interop.JniValueMarshaler`1[[Java.Interop.IJavaPeerable, Java.Interop, Version=7.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065]].CreateValue(JniObjectReference& , JniObjectReferenceOptions , Type )
	   at Java.Interop.JniRuntime.JniValueManager.CreateValue(JniObjectReference& , JniObjectReferenceOptions , Type )
	   at Java.InteropTests.JniRuntimeJniValueManagerTests.CreateValue()
	   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
	   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
	--MissingMethodException
	   at Java.Interop.TypeManager.CreateProxy(Type , IntPtr , JniHandleOwnership )
	   at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	--JavaLocationException
	Java.Lang.Error: Exception_WasThrown, Java.Lang.Error

	  --- End of managed Java.Lang.Error stack trace ---
	java.lang.Error: Java callstack:
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

	  --- End of managed Java.Lang.Error stack trace ---
	java.lang.Error: Java callstack:
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)

etc., etc.

(*Actually*, part of the problem is that `AndroidRuntime` doesn't
support Java.Interop-style activation constructors *at all*…
…and I don't want to deal with that right now, either.)

It was borderline bananas to expect anything to work when
`java/lang/Object` *wasn't* bound as `Java.Lang.Object, Mono.Android`.
I'm frankly suprised there are *only* 23 failures, most of them
duplicates of the same 8 failures, two of which are above.

Update `<GenerateJavaStubs/>` and related typemap code to special-case
`Mono.Android.dll` so that it is processed *first*.  This *should*
ensure that `Java.Lang.Object` is *preferred* over any other possible
binding of `java/lang/Object`.
@jonpryor
Copy link
Member Author

Most tests currently pass. We just have failures in Mono.Android.NET_Tests-NoAab, which appears to be because Mono.Android.dll isn't the first source used for typemap data. (e38e3b7 fixed the same issues for the other variants.)

What's special about -NoAab?

Context: https://github.com/xamarin/xamarin-android/blob/1858b55ed7ecced8ffb52a82a7151d639625e240/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs#L472
Context: https://github.com/xamarin/xamarin-android/blob/1858b55ed7ecced8ffb52a82a7151d639625e240/src/Xamarin.Android.Build.Tasks/Utilities/NativeTypeMappingData.cs#L24-L32

e38e3b7 was a valiant effort, but for naught, because the order of
`allJavaTypes` within `<GenerateJavaStubs/>` does not matter.

Nor does the order of `GenerateJavaStubs.ResolvedAssemblies` and
`GenerateJavaStubs.ResolvedUserAssemblies` matter.

What matters is two things:

 1. The `NativeTypeMappingData` constructor creates the typemap
    entries, checking for (and skipping) duplicate bindings, and
    it generates typemap entries in *in module order*.

 2. Module order is determined by sorting the MVIDs of the assemblies.

(Also, it's not *strictly* the MVID; it's the *byte representation*
of the MVID, as the MVID is treated as a sequence of bytes.)

We now turn to the assemblies in question [^0]:

	% ikdasm obj/Release/net9.0-android/android-arm64/linked/Mono.Android.dll | grep MVID
	// MVID: {0FE503F4-2262-4B58-91A7-6F50A0B71838}

	% ikdasm obj/Release/net9.0-android/android-x86/linked/Java.Interop-Tests.dll | grep MVID
	// MVID: {50B3412B-DF73-4BBB-9797-1BABC080BF56}

Normal GUID output doesn't help; we need bytes.  Enter our favorite
C# REPL, and:

	var mono_android_dll        = Guid.Parse("0FE503F4-2262-4B58-91A7-6F50A0B71838");
	mono_android_dll.ToByteArray();
	// == { 244, 3, 229, 15, 98, 34, 88, 75, 145, 167, 111, 80, 160, 183, 24, 56 }

	var java_interop_tests_dll  = Guid.Parse("50B3412B-DF73-4BBB-9797-1BABC080BF56");
	java_interop_tests_dll.ToByteArray();
	// == { 43, 65, 179, 80, 115, 223, 187, 75, 151, 151, 27, 171, 192, 128, 191, 86 }

`Java.Interop-Tests.dll` *always* sorts before `Mono.Android.dll` [^2].

"Revert" e38e3b7, and instead special-case `Mono.Android.dll` within
`NativeTypeMappingData`: if `Mono.Android.dll` is a module, process it
*first*, so that it has priority for binding `java/lang/Object`.
Then process all the other modules.

This feels both like a hack, *and* more reliable, so there we go.

Additionally, I NEED MY PRINTF DEBUGGING.

Plumb `Action<string> logger` through `NativeTypeMappingData` and
related types so that `NativeTypeMappingData` can print a message
when a typemap "conflict" is encountered:

	Skipping typemap entry for `Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`; `java/lang/Object` is already mapped.

[^0]:  Note that we're using linked output, so the MVIDs *will* vary
       from build to build, as well as their ordering [^1].

[^1]:  Which makes this even more fun: *sometimes* things could work,
       sometimes not, all at the whim of the random number generator
       used to create the MVIDs.

[^2]:  But not quite the whole story, either: in local testing, *yes*,
       `Java.Interop-Tests.dll` consistently appears in `map_modules`
       before `Mono.Android.dll`.  ***However***, *sometimes* `map_java`
       would map `java/lang/Object` to `Java.Lang.Object, Mono.Android`
       for e.g. arm64-v8a, while -- ***for the same build*** -- would
       map `java/lang/Object` to
       `Java.InteropTests.JavaLangRemappingTestRuntime, Java.Interop-Tests`
       for *everything else*: armeabi-v7a, x86, and x86_64.

       The `map_modules` tables were identical across ABIs.

       If it were *just* a matter of module sorting, then all ABIs
       would have the same typemaps, but they don't.

       I have no explanation for this yet.
@jonpryor
Copy link
Member Author

jonpryor commented Feb 2, 2024

@jonpryor asked

What's special about -NoAab?

Nothing is special about -NoAab. At least if the current hypothesis is correct: be2a63b

The current hypothesis is that typemap generation is based upon module ordering which is based on the byte[] representation of the MVID which is random, because linking changes the MVID. Meaning the tests passed because of pure luck, and -NoAab failed because of pure luck.


Context: https://github.com/xamarin/xamarin-android/blob/1858b55ed7ecced8ffb52a82a7151d639625e240/src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs#L472
Context: https://github.com/xamarin/xamarin-android/blob/1858b55ed7ecced8ffb52a82a7151d639625e240/src/Xamarin.Android.Build.Tasks/Utilities/NativeTypeMappingData.cs#L24-L32

e38e3b7 was a valiant effort, but for naught, because the order of
allJavaTypes within <GenerateJavaStubs/> does not matter.

Nor does the order of GenerateJavaStubs.ResolvedAssemblies and
GenerateJavaStubs.ResolvedUserAssemblies matter.

What matters is two things:

  1. The NativeTypeMappingData constructor creates the typemap
    entries, checking for (and skipping) duplicate bindings, and
    it generates typemap entries in in module order.

  2. Module order is determined by sorting the MVIDs of the assemblies.

(Also, it's not strictly the MVID; it's the byte representation
of the MVID, as the MVID is treated as a sequence of bytes.)

We now turn to the assemblies in question 1:

% ikdasm obj/Release/net9.0-android/android-arm64/linked/Mono.Android.dll | grep MVID
// MVID: {0FE503F4-2262-4B58-91A7-6F50A0B71838}

% ikdasm obj/Release/net9.0-android/android-x86/linked/Java.Interop-Tests.dll | grep MVID
// MVID: {50B3412B-DF73-4BBB-9797-1BABC080BF56}

Normal GUID output doesn't help; we need bytes. Enter our favorite
C# REPL, and:

var mono_android_dll        = Guid.Parse("0FE503F4-2262-4B58-91A7-6F50A0B71838");
mono_android_dll.ToByteArray();
// == { 244, 3, 229, 15, 98, 34, 88, 75, 145, 167, 111, 80, 160, 183, 24, 56 }

var java_interop_tests_dll  = Guid.Parse("50B3412B-DF73-4BBB-9797-1BABC080BF56");
java_interop_tests_dll.ToByteArray();
// == { 43, 65, 179, 80, 115, 223, 187, 75, 151, 151, 27, 171, 192, 128, 191, 86 }

Java.Interop-Tests.dll always sorts before Mono.Android.dll 2.

"Revert" e38e3b7, and instead special-case Mono.Android.dll within
NativeTypeMappingData: if Mono.Android.dll is a module, process it
first, so that it has priority for binding java/lang/Object.
Then process all the other modules.

This feels both like a hack, and more reliable, so there we go.

Additionally, I NEED MY PRINTF DEBUGGING.

Plumb Action<string> logger through NativeTypeMappingData and
related types so that NativeTypeMappingData can print a message
when a typemap "conflict" is encountered:

Skipping typemap entry for `Java.InteropTests.JavaLangRemappingTestObject, Java.Interop-Tests`; `java/lang/Object` is already mapped.

Footnotes

  1. Note that we're using linked output, so the MVIDs will vary
    from build to build, as well as their ordering 3.

  2. But not quite the whole story, either: in local testing, yes,
    Java.Interop-Tests.dll consistently appears in map_modules
    before Mono.Android.dll. However, sometimes map_java
    would map java/lang/Object to Java.Lang.Object, Mono.Android
    for e.g. arm64-v8a, while -- for the same build -- would
    map java/lang/Object to
    Java.InteropTests.JavaLangRemappingTestRuntime, Java.Interop-Tests
    for everything else: armeabi-v7a, x86, and x86_64.

    The map_modules tables were identical across ABIs.

    If it were just a matter of module sorting, then all ABIs
    would have the same typemaps, but they don't.

    I have no explanation for this yet.

  3. Which makes this even more fun: sometimes things could work,
    sometimes not, all at the whim of the random number generator
    used to create the MVIDs.

@jonpryor
Copy link
Member Author

jonpryor commented Feb 2, 2024

Behold, a green build!

Now for the cleanup…

jonpryor added a commit to dotnet/java-interop that referenced this pull request Feb 2, 2024
…1181)

Context: dotnet/android#8543
Context: dotnet/android#8625
Context: dotnet/android#8681
Context: #1168
Context: def5bc0
Context: 005c914

dotnet/android#8543 tested PR #1168, was Totally Green™ --
finding no issues -- and so we merged PR #1168 into 005c914.

Enter dotnet/android#8625, which bumps xamarin-android to
use def5bc0, which includes 005c914.  dotnet/android#8625
contains *failing unit tests* (?!), including
`Java.InteropTests.InvokeVirtualFromConstructorTests()`:

	Java.Lang.LinkageError : net.dot.jni.test.CallVirtualFromConstructorDerived
	----> System.NotSupportedException : Could not find System.Type corresponding to Java type JniTypeSignature(TypeName=net/dot/jni/test/CallVirtualFromConstructorDerived ArrayRank=0 Keyword=False) .
	   at Java.Interop.JniEnvironment.StaticMethods.GetStaticMethodID(JniObjectReference , String , String )
	   at Java.Interop.JniType.GetStaticMethod(String , String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String , String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.GetMethodInfo(String )
	   at Java.Interop.JniPeerMembers.JniStaticMethods.InvokeObjectMethod(String , JniArgumentValue* )
	   at Java.InteropTests.CallVirtualFromConstructorDerived.NewInstance(Int32 value)
	   at Java.InteropTests.InvokeVirtualFromConstructorTests.ActivationConstructor()
	   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
	   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
	  --- End of managed Java.Lang.LinkageError stack trace ---
	java.lang.NoClassDefFoundError: net.dot.jni.test.CallVirtualFromConstructorDerived
		at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onStart(Native Method)
		at crc643df67da7b13bb6b1.TestInstrumentation_1.onStart(TestInstrumentation_1.java:35)
		at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:2189)
	Caused by: android.runtime.JavaProxyThrowable: [System.NotSupportedException]: Could not find System.Type corresponding to Java type JniTypeSignature(TypeName=net/dot/jni/test/CallVirtualFromConstructorDerived ArrayRank=0 Keyword=False) .
		at Java.Interop.ManagedPeer.GetTypeFromSignature(Unknown Source:0)
		at Java.Interop.ManagedPeer.RegisterNativeMembers(Unknown Source:0)
		at net.dot.jni.ManagedPeer.registerNativeMembers(Native Method)
		at net.dot.jni.test.CallVirtualFromConstructorDerived.<clinit>(CallVirtualFromConstructorDerived.java:12)
		... 3 more

	--NotSupportedException
	   at Java.Interop.ManagedPeer.GetTypeFromSignature(JniTypeManager , JniTypeSignature , String )
	   at Java.Interop.ManagedPeer.RegisterNativeMembers(IntPtr jnienv, IntPtr klass, IntPtr n_nativeClass, IntPtr n_methods)

:shocked-pikachu-face: (But dotnet/android#8543 was green!)

The problem is twofold:

 1. 005c914 now requires the presence of typemap entries from e.g.
    `net.dot.jni.test.CallVirtualFromConstructorDerived` to
    `Java.InteropTests.CallVirtualFromConstructorDerived`.

 2. `Java.Interop.Tools.JavaCallableWrappers` et al doesn't create
    typemap entries for `Java.Interop.JavaObject` subclasses which
    have `[JniTypeSignature]`.

Consequently, our units tests fail (and apparently weren't *run* on
dotnet/android#8543?!  Still not sure what happened.)

Update typemap generation by adding a new `TypeDefinition.HasJavaPeer()`
extension method to replace all the `.IsSubclassOf("Java.Lang.Object")`
and similar checks, extending it to also check for
`Java.Interop.JavaObject` and `Java.Interop.JavaException` base types.
(Continuing to use base type checks is done instead of just relying
on implementation of `Java.Interop.IJavaPeerable` as a performance
optimization, as there could be *lots* of interface types to check.)

Additionally, @jonathanpeppers -- while trying to investigate all
this -- ran across a build failure:

	obj\Debug\net9.0-android\android\src\java\lang\Object.java(7,15): javac.exe error JAVAC0000:  error: cyclic inheritance involving Object

This suggests that `Java.Interop.Tools.JavaCallableWrappers` was
encountering `Java.Interop.JavaObject` -- or some other type which
has `[JniTypeSignature("java/lang/Object")]` -- which is why
`java/lang/Object.java` was being generated.

Audit all `[JniTypeSignature]` attributes, and add
`GenerateJavaPeer=false` to all types which should *not* hava a
Java Callable Wrapper generated for them.  This includes nearly
everything within `Java.Interop-Tests.dll`.  (We want the typemaps!
We *don't* want generated Java source, as we have hand-written Java
peer types for those tests.)

Add `[JniTypeSignature]` to `GenericHolder<T>`.  This type mapping
isn't *actually* required, but it *is* used in `JavaVMFixture`, and
it confuses people (me!) if things are inconsistent.  Additionally,
remove `tests/` from the Java-side name, for consistency.

~~ Avoid multiple java/lang/Object bindings ~~

A funny thing happened when in dotnet/android#8681 -- which
tested this commit -- when using an intermediate version of this
commit: unit tests started crashing!

	E monodroid-assembly: typemap: unable to load assembly 'Java.Interop-Tests' when looking up managed type corresponding to Java type 'java/lang/Object'

What appears to be happening is an Unfortunate Interaction™:

 1. `Java.Interop-Tests.dll` contained *multiple bindings* for
    `java/lang/Object`. e.g.

        [JniTypeSignature ("java/lang/Object", GenerateJavaPeer=false)]
        partial class JavaDisposedObject : JavaObject {
        }

 2. The typemap generator has no functionality to "prioritize" one
    binding vs. another; it's random.  As such, there is nothing to
    cause `Java.Lang.Object, Mono.Android` to be used as the
    preferred binding for `java/lang/Object`.

This meant that when we hit the typemap codepath in .NET Android,
we looked for the C# type that corresponded to `java/lang/Object`,
found *some random type* from `Java.Interop-Tests`, and…

…and then we hit another oddity: that codepath only supported looking
for C# types in assemblies which had already been loaded.  This was
occurring during startup, so `Java.Interop-Tests` had not yet been
loaded yet, so it errored out, returned `nullptr`, and later Android
just aborts things:

	F droid.NET_Test: runtime.cc:638] JNI DETECTED ERROR IN APPLICATION: use of deleted local reference 0x79

Just…eep!

This didn't happen before because `Java.Interop.JavaObject` subclasses
*didn't* participate in typemap generation.  This commit *adds* that
support, introducing this unforeseen interaction.

Fix this by *removing* most "alternate bindings" for `java/lang/Object`:

	- [JniTypeSignature ("java/lang/Object", GenerateJavaPeer=false)]
	+ [JniTypeSignature (JniTypeName)]
	  partial class JavaDisposedObject : JavaObject {
	+     internal const string JniTypeName = "net/dot/jni/test/JavaDisposedObject";
	  }

This implicitly requires that we now have a Java Callable Wrapper
for this type, so update `Java.Interop-Tests.csproj` to run `jcw-gen`
as part of the build process.  This ensures that we create the
JCW for e.g. `JavaDisposedObject`.

Update `JavaVMFixture` to add the required typemap entries.

---

Aside: this project includes [T4 Text Templates][0].  To regenerate
the output files *without involving Visual Studio*, you can install
the [`dotnet-t4`][1] tool:

	$ dotnet tool install --global dotnet-t4

then run it separately for each `.tt` file:

	$HOME/.dotnet/tools/t4 -o src/Java.Interop/Java.Interop/JavaPrimitiveArrays.cs \
	  src/Java.Interop/Java.Interop/JavaPrimitiveArrays.tt

[0]: https://learn.microsoft.com/visualstudio/modeling/code-generation-and-t4-text-templates?view=vs-2022
[1]: https://www.nuget.org/packages/dotnet-t4/
@jonpryor jonpryor changed the title Try xamarin/java.interop#1181 Bump to xamarin/java.interop/main@07c73009 Feb 2, 2024
@jonpryor jonpryor merged commit 25d1f00 into main Feb 2, 2024
47 checks passed
@jonpryor jonpryor deleted the dev/jonp/try-ji-1181 branch February 2, 2024 17:03
jonpryor added a commit that referenced this pull request Feb 2, 2024
PR #8681 was merged, which fixed the Java.Interop bump issues.
grendello added a commit that referenced this pull request Feb 5, 2024
* main:
  [monodroid] typemaps may need to load assemblies (#8625)
  Bump $(AndroidNetPreviousVersion) to 34.0.79 (#8693)
  Bump to xamarin/java.interop/main@07c73009 (#8681)
  Bump to dotnet/installer@1c496970b7 9.0.100-preview.2.24078.1 (#8685)
grendello added a commit that referenced this pull request Feb 5, 2024
* main:
  [monodroid] typemaps may need to load assemblies (#8625)
  Bump $(AndroidNetPreviousVersion) to 34.0.79 (#8693)
  Bump to xamarin/java.interop/main@07c73009 (#8681)
grendello added a commit that referenced this pull request Feb 7, 2024
* main:
  Bump to dotnet/installer@fb7b9a4b9e 9.0.100-preview.2.24106.6 (#8700)
  [Mono.Android] Cache `Profiles/api-34.xml` contents (#8679)
  [monodroid] typemaps may need to load assemblies (#8625)
  Bump $(AndroidNetPreviousVersion) to 34.0.79 (#8693)
  Bump to xamarin/java.interop/main@07c73009 (#8681)
  Bump to dotnet/installer@1c496970b7 9.0.100-preview.2.24078.1 (#8685)
  [GetAndroidDependencies] Add Jdk dependency info (#8651)
  [xaprepare] Add support for newer SparkyLinux (#8684)
grendello added a commit that referenced this pull request Feb 14, 2024
* main: (116 commits)
  [tmt] Update to work with current `libxamarin-app.so` (#8694)
  [Xamarin.Android.Build.Tasks] remove `$(AndroidSupportedAbis)` from `build.props` (#8717)
  [Xamarin.Android.Build.Tasks] BannedApiAnalyzers for Resolve() (#8715)
  Bump to xamarin/Java.Interop/main@dfcbd670 (#8714)
  [monodroid] C++ tweaks and legacy code cleanup (#8638)
  Bump to xamarin/xamarin-android-tools/main@a698a33 (#8710)
  [readme] Add `d17-8` download links. (#8709)
  Bump external/Java.Interop from `07c7300` to `7f08b77` (#8702)
  Bump to xamarin/monodroid@848d1277b7 (#8691)
  [Xamarin.Android.Build.Tasks] `FixAbstractMethodsStep` performance (#8650)
  Bump to dotnet/installer@fb7b9a4b9e 9.0.100-preview.2.24106.6 (#8700)
  [Mono.Android] Cache `Profiles/api-34.xml` contents (#8679)
  [monodroid] typemaps may need to load assemblies (#8625)
  Bump $(AndroidNetPreviousVersion) to 34.0.79 (#8693)
  Bump to xamarin/java.interop/main@07c73009 (#8681)
  Bump to dotnet/installer@1c496970b7 9.0.100-preview.2.24078.1 (#8685)
  [GetAndroidDependencies] Add Jdk dependency info (#8651)
  [xaprepare] Add support for newer SparkyLinux (#8684)
  Bump to dotnet/installer@5680e93cb2 9.0.100-preview.2.24073.12 (#8666)
  $(AndroidPackVersionSuffix)=preview.2; net9 is 34.99.0.preview.2 (#8678)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants