Audit JNIEnv::GetObjectClass() invocations

Context: dotnet/java-interop@6b3637d

The story so far is that some of our unit tests are crashing, and
have been in one form or another since 4332ea0
(the bump to dotnet/java-interop@def5bc0).

The current crash, from the PR build for 7b46391:

	D monodroid-assembly: typemap: assembly 'Java.Interop-Tests' hasn't been loaded yet, attempting a full load
	W monodroid-assembly: typemap: failed to load managed assembly 'Java.Interop-Tests.dll'. No such file or directory
	E monodroid-assembly: typemap: unable to load assembly 'Java.Interop-Tests' when looking up managed type corresponding to Java type 'java/lang/Object'
	I monodroid-timing: [1/5] Typemap.java_to_managed: end, total time; elapsed: 0:0::260000
	W monodroid-assembly: typemap: called from
	W monodroid-assembly: at Java.Interop.TypeManager.GetJavaToManagedType(String )
	W monodroid-assembly:    at Java.Interop.TypeManager.CreateInstance(IntPtr , JniHandleOwnership , Type )
	W monodroid-assembly:    at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
	W monodroid-assembly:    at Java.Lang.Object._GetObject[IIterator](IntPtr , JniHandleOwnership )
	W monodroid-assembly:    at Java.Lang.Object.GetObject[IIterator](IntPtr handle, JniHandleOwnership transfer)
	W monodroid-assembly:    at Android.Runtime.JavaSet.Iterator()
	W monodroid-assembly:    at Android.Runtime.JavaSet`1[[System.String, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetEnumerator()
	W monodroid-assembly:    at Xamarin.Android.UnitTests.TestInstrumentation`1[[Xamarin.Android.UnitTests.NUnit.NUnitTestRunner, TestRunner.NUnit.NET, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].ProcessArguments()
	W monodroid-assembly:    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)
	W monodroid-assembly:    at Android.App.Instrumentation.n_OnCreate_Landroid_os_Bundle_(IntPtr jnienv, IntPtr native__this, IntPtr native_arguments)
	W monodroid-assembly:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PPL_V(_JniMarshal_PPL_V callback, IntPtr jnienv, IntPtr klazz, IntPtr p0)
	E droid.NET_Test: JNI ERROR (app bug): accessed stale Local 0x75  (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 0x75

There are *three* "concerning" items here:

 1. typemaps are trying to load `Java.Interop-Tests`, and failing:

        typemap: failed to load managed assembly 'Java.Interop-Tests.dll'. No such file or directory

    @grendello is looking into this.

 2. The binding for `java/lang/Object` is coming from
    Java.Interop-Tests, not Mono.Android (?!)

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

    dotnet/java-interop#1181 has a fix for this, and we're not
    applying the fix yet because we believe that it will hide (1).

 3. The JNI error, which crashes the process:

        F droid.NET_Test: java_vm_ext.cc:570] JNI DETECTED ERROR IN APPLICATION: use of deleted local reference 0x75
        F droid.NET_Test: java_vm_ext.cc:570]     from void crc643df67da7b13bb6b1.TestInstrumentation_1.n_onCreate(android.os.Bundle)
        F droid.NET_Test: runtime.cc:630] Runtime aborting...
        F droid.NET_Test: runtime.cc:630] Dumping all threads without mutator lock held
        F droid.NET_Test: runtime.cc:630] All threads:
        F droid.NET_Test: runtime.cc:630] DALVIK THREADS (14):
        F droid.NET_Test: runtime.cc:630] "main" prio=5 tid=1 Runnable
        F droid.NET_Test: runtime.cc:630]   | group="" sCount=0 dsCount=0 flags=0 obj=0x729e9d98 self=0x7567e0f51000
        F droid.NET_Test: runtime.cc:630]   | sysTid=9143 nice=0 cgrp=default sched=0/0 handle=0x7567e14daed8
        F droid.NET_Test: runtime.cc:630]   | state=R schedstat=( 1270418000 334229000 139 ) utm=16 stm=110 core=0 HZ=100
        F droid.NET_Test: runtime.cc:630]   | stack=0x7ffcbb3e4000-0x7ffcbb3e6000 stackSize=8192KB
        F droid.NET_Test: runtime.cc:630]   | held mutexes= "abort lock" "mutator lock"(shared held)
        F droid.NET_Test: runtime.cc:630]   native: #00 pc 000000000048df4e  /apex/com.android.runtime/lib64/libart.so (art::DumpNativeStack(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, int, BacktraceMap*, char const*, art::ArtMethod*, void*, bool)+126)
        F droid.NET_Test: runtime.cc:630]   native: #1 pc 00000000005a77c3  /apex/com.android.runtime/lib64/libart.so (art::Thread::DumpStack(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, bool, BacktraceMap*, bool) const+675)
        F droid.NET_Test: runtime.cc:630]   native: #2 pc 00000000005c49cb  /apex/com.android.runtime/lib64/libart.so (art::DumpCheckpoint::Run(art::Thread*)+859)
        F droid.NET_Test: runtime.cc:630]   native: #3 pc 00000000005bcf28  /apex/com.android.runtime/lib64/libart.so (art::ThreadList::RunCheckpoint(art::Closure*, art::Closure*)+456)
        F droid.NET_Test: runtime.cc:630]   native: #4 pc 00000000005bc2e1  /apex/com.android.runtime/lib64/libart.so (art::ThreadList::Dump(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, bool)+1601)
        F droid.NET_Test: runtime.cc:630]   native: #5 pc 0000000000552eb9  /apex/com.android.runtime/lib64/libart.so (art::Runtime::Abort(char const*)+1529)
        F droid.NET_Test: runtime.cc:630]   native: #6 pc 000000000000c873  /system/lib64/libbase.so (android::base::LogMessage::~LogMessage()+611)
        F droid.NET_Test: runtime.cc:630]   native: #7 pc 00000000003ede84  /apex/com.android.runtime/lib64/libart.so (art::JavaVMExt::JniAbort(char const*, char const*)+1604)
        F droid.NET_Test: runtime.cc:630]   native: #8 pc 00000000003ee18a  /apex/com.android.runtime/lib64/libart.so (art::JavaVMExt::JniAbortF(char const*, char const*, ...)+234)
        F droid.NET_Test: runtime.cc:630]   native: #9 pc 00000000005adf31  /apex/com.android.runtime/lib64/libart.so (art::Thread::DecodeJObject(_jobject*) const+785)
        F droid.NET_Test: runtime.cc:630]   native: #10 pc 00000000003def9b  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::CheckInstance(art::ScopedObjectAccess&, art::(anonymous namespace)::ScopedCheck::InstanceKind, _jobject*, bool)+91)
        F droid.NET_Test: runtime.cc:630]   native: #11 pc 00000000003de205  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::CheckPossibleHeapValue(art::ScopedObjectAccess&, char, art::(anonymous namespace)::JniValueType)+485)
        F droid.NET_Test: runtime.cc:630]   native: #12 pc 00000000003dd732  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::(anonymous namespace)::JniValueType*)+690)
        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)
        F droid.NET_Test: runtime.cc:630]   native: #14 pc 0000000000017196  /data/app/Mono.Android.NET_Tests-LUUW792fOvX745oAS4jeRQ==/split_config.x86_64.apk (offset 331000) (???)
        F droid.NET_Test: runtime.cc:630]   at crc643df67da7b13bb6b1.TestInstrumentation_1.n_onCreate(Native method)

As per `native #13`, we're (somehow) passing an invalid JNI reference
to `JNIEnv::GetObjectClass()`.

***HOW*** are we passing an invalid JNI reference to
`JNIEnv::GetObjectClass()`?

Attempt to investigate (3) further, by:

 1. Reviewing all calls to `JNIEnv::GetObjectClass()` within this
    repo to see if we could potentially be passing an invalid value.
    The "most obvious" candidate is `TypeManager.CreateInstance()`,
    which calls `JNIEnv::GetObjectClass()` in a loop.

    I'm still not sure if that could possibly be the cause, but
    Just In Case™…

    "Cleanup" some C++ code so that calls to
    `JNIEnv::DeleteLocalReference()` are closer to the
    `JNIEnv::GetObjectClass()` calls.

 2. Update
    `tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj`
    to add an `@(AndroidEnvironment)` item, which sets
    `debug.mono.log=gref+,lref+`.  This will enable LREF and GREF
    logging within `adb logcat`, which *may* allow us to track down
    where "local reference 0x75" came from.

Author: jonpryor

src/Mono.Android/Java.Interop/TypeManager.cs

@@ -265,9 +265,14 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership
 		[UnconditionalSuppressMessage ("Trimming", "IL2072", Justification = "TypeManager.CreateProxy() does not statically know the value of the 'type' local variable.")]
 		internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership transfer, Type? targetType)
 		{
+			if (handle == IntPtr.Zero) {
+				throw new NotSupportedException (
+						FormattableString.Invariant ($"Internal error: `handle` is NULL!"),
+						CreateJavaLocationException ());
+			}
 			Type? type = null;
 			IntPtr class_ptr = JNIEnv.GetObjectClass (handle);
-			string class_name = GetClassName (class_ptr);
+			string? class_name = GetClassName (class_ptr);
 			lock (TypeManagerMapDictionaries.AccessLock) {
 				while (class_ptr != IntPtr.Zero && !TypeManagerMapDictionaries.JniToManaged.TryGetValue (class_name, out type)) {
 
@@ -279,12 +284,18 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership
 
 					IntPtr super_class_ptr = JNIEnv.GetSuperclass (class_ptr);
 					JNIEnv.DeleteLocalRef (class_ptr);
+					class_name = null;
 					class_ptr = super_class_ptr;
-					class_name = GetClassName (class_ptr);
+					if (class_ptr != IntPtr.Zero) {
+						class_name = GetClassName (class_ptr);
+					}
 				}
 			}
 
-			JNIEnv.DeleteLocalRef (class_ptr);
+			if (class_ptr != IntPtr.Zero) {
+				JNIEnv.DeleteLocalRef (class_ptr);
+				class_ptr = IntPtr.Zero;
+			}
 
 			if (type == null) {
 				JNIEnv.DeleteRef (handle, transfer);

src/monodroid/jni/osbridge.cc

@@ -657,15 +657,13 @@ OSBridge::add_reference_jobject (JNIEnv *env, jobject handle, jobject reffed_han
 
 	java_class = env->GetObjectClass (handle);
 	add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V");
+	env->DeleteLocalRef (java_class);
 	if (add_method_id) {
 		env->CallVoidMethod (handle, add_method_id, reffed_handle);
-		env->DeleteLocalRef (java_class);
-
 		return 1;
 	}
 
 	env->ExceptionClear ();
-	env->DeleteLocalRef (java_class);
 	return 0;
 }
 
@@ -910,7 +908,6 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri
 	MonoClass *klass;
 #endif
 	MonoObject *obj;
-	jclass java_class;
 	jobject jref;
 	jmethodID clear_method_id;
 	int i, j, total, alive, refs_added;
@@ -942,8 +939,9 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri
 				sccs [i]->is_alive = 1;
 				mono_field_get_value (obj, bridge_info->refs_added, &refs_added);
 				if (refs_added) {
-					java_class = env->GetObjectClass (jref);
+					jclass java_class = env->GetObjectClass (jref);
 					clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V");
+					env->DeleteLocalRef (java_class);
 					if (clear_method_id) {
 						env->CallVoidMethod (jref, clear_method_id);
 					} else {
@@ -957,7 +955,6 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri
 						}
 #endif
 					}
-					env->DeleteLocalRef (java_class);
 				}
 			} else {
 				abort_unless (!sccs [i]->is_alive, "Bridge SCC at index %d must NOT be alive", i);

tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Environment.txt

@@ -0,0 +1 @@
+debug.mono.log=lref+,gref+

tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj

@@ -50,6 +50,10 @@
     <Compile Remove="..\Mono.Data.Sqlite\SqliteTests.cs" />
   </ItemGroup>
 
+  <ItemGroup>
+    <AndroidEnvironment Include="Environment.txt" />
+  </ItemGroup>
+
   <ItemGroup Condition=" '$(AndroidPackageFormat)' != 'aab' ">
     <TestApk Include="$(OutputPath)$(_MonoAndroidTestPackage)-Signed.apk">
       <Package>$(_MonoAndroidTestPackage)</Package>