[monodroid] typemaps may need to load assemblies (#8625)

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

When attempting to bump to dotnet/java-interop@005c9141, multiple
unit tests would fail, e.g.

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

This happened because dotnet/java-interop@005c9141 implicitly
required that typemaps exist for `Java.Interop.JavaObject` subclasses.

Fair enough; enter xamasrin/java.interop#1181, which added support to
`Java.Interop.Tools.JavaCallableWrappers` to emit typemaps for
`Java.Interop.JavaObject` subclasses.

That caused *crashes* in `tests/Mono.Android-Tests`:

	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
	…
	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 cause of the crash was a "use after free" bug within
`TypeManager.CreateInstance()` in a never-hit-before error path; the
"use after free" bug was fixed in 25d1f00.

However, the cause of the hitting a never-hit-before error path is
because `EmbeddedAssemblies::typemap_java_to_managed()` would only
map Java types to `System.Type` instances for assemblies that have
*already been loaded*.  If the assembly had not yet been loaded, then
`EmbeddedAssemblies::typemap_java_to_managed()` would return `null`,
and if the binding it couldn't find happens to be for
`java.lang.Object`, we hit the (buggy!) "Where is the Java.Lang.Object
wrapper" error condition.

Commit 25d1f00 fixes that and a great many other related issues.

What's left is `EmbeddedAssemblies::typemap_java_to_managed()`:
it should *never* return null *unless* there is no typemap at all.
Whether the target assembly has been loaded or not should be
irrelevant.

Update `EmbeddedAssemblies::typemap_java_to_managed()` so that it
will load the target assembly if necessary.

Additionally, before we figured out that we had a "use after free"
bug, all we had to go on was that *something* related to
`JNIEnv::GetObjectClass()` was involved.  Review JNI usage around
`JNIEnv::GetObjectClass()` and related invocations, and cleanup:

  * Simplify logic related to `JNIEnv::DeleteLocalRef()`.
  * Decrease scope of local variables.
  * Clear variables passed to `JNIEnv.DeleteLocalRef()`.

Co-authored-by: Jonathan Pryor <[email protected]>
Co-authored-by: Marek Habersack <[email protected]>

Author: 3 people

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

@@ -267,7 +267,7 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership
 		{
 			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 +279,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 (targetType != null &&
 					(type == null ||

src/monodroid/jni/embedded-assemblies.cc

@@ -695,6 +695,30 @@ EmbeddedAssemblies::typemap_java_to_managed (hash_t hash, const MonoString *java
 
 	if (module->image == nullptr) {
 		module->image = mono_image_loaded (module->assembly_name);
+
+		if (module->image == nullptr) {
+			log_debug (LOG_ASSEMBLY, "typemap: assembly '%s' hasn't been loaded yet, attempting a full load", module->assembly_name);
+
+			// Fake a request from MonoVM to load the assembly.
+			MonoAssemblyName *assembly_name = mono_assembly_name_new (module->assembly_name);
+			MonoAssembly *assm;
+
+			if (assembly_name == nullptr) {
+				log_error (LOG_ASSEMBLY, "typemap: failed to create Mono assembly name for '%s'", module->assembly_name);
+				assm = nullptr;
+			} else {
+				MonoAssemblyLoadContextGCHandle alc_gchandle = mono_alc_get_default_gchandle ();
+				MonoError mono_error;
+				assm = embeddedAssemblies.open_from_bundles (assembly_name, alc_gchandle, &mono_error, false /* ref_only */);
+			}
+
+			if (assm == nullptr) {
+				log_warn (LOG_ASSEMBLY, "typemap: failed to load managed assembly '%s'", module->assembly_name);
+			} else {
+				module->image = mono_assembly_get_image (assm);
+			}
+		}
+
 		if (module->image == nullptr) {
 			log_error (LOG_ASSEMBLY, "typemap: unable to load assembly '%s' when looking up managed type corresponding to Java type '%s'", module->assembly_name, to_utf8 (java_type_name).get ());
 			return nullptr;

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