Merge pull request #113159 from dsnopek/android-variant-to-jvalue-memory-issues

Android: Fix memory issues in `_variant_to_jvalue()`

platform/android/java/app/src/instrumented/assets/test/base_test.gd

@@ -42,3 +42,10 @@ func assert_equal(actual, expected):
 	else:
 		__assert_fail()
 		print ("    |-> Expected '%s' but got '%s'" % [expected, actual])
+
+func assert_true(value):
+	if value:
+		__assert_pass()
+	else:
+		__assert_fail()
+		print ("    |-> Expected '%s' to be truthy" % value)

platform/android/java/app/src/instrumented/assets/test/javaclasswrapper/java_class_wrapper_tests.gd

@@ -18,6 +18,8 @@ func run_tests():
 
 	__exec_test(test_big_integers)
 
+	__exec_test(test_callable)
+
 	print("JavaClassWrapper tests finished.")
 	print("Tests started: " + str(_test_started))
 	print("Tests completed: " + str(_test_completed))
@@ -142,3 +144,14 @@ func test_big_integers():
 	assert_equal(TestClass.testArgLong(4242424242), "4242424242")
 	assert_equal(TestClass.testArgLong(-4242424242), "-4242424242")
 	assert_equal(TestClass.testDictionary({a = 4242424242, b = -4242424242}), "{a=4242424242, b=-4242424242}")
+
+func test_callable():
+	var android_runtime = Engine.get_singleton("AndroidRuntime")
+	assert_true(android_runtime != null)
+
+	var cb1_data := {called = false}
+	var cb1 = func():
+		cb1_data['called'] = true
+		return null
+	android_runtime.createRunnableFromGodotCallable(cb1).run()
+	assert_equal(cb1_data['called'], true)

platform/android/java_class_wrapper.cpp

@@ -393,7 +393,7 @@ bool JavaClass::_call_method(JavaObject *p_instance, const StringName &p_method,
 			} break;
 			case ARG_TYPE_CLASS: {
 				if (p_args[i]->get_type() == Variant::DICTIONARY) {
-					argv[i].l = _variant_to_jvalue(env, Variant::DICTIONARY, p_args[i]).l;
+					argv[i].l = _variant_to_jobject(env, Variant::DICTIONARY, p_args[i]);
 				} else {
 					Ref<JavaObject> jo = *p_args[i];
 					if (jo.is_valid()) {

platform/android/java_godot_lib_jni.cpp

@@ -536,7 +536,7 @@ JNIEXPORT jobject JNICALL Java_org_godotengine_godot_GodotLib_getEditorProjectMe
 		String key = jstring_to_string(p_key, env);
 		Variant default_value = _jobject_to_variant(env, p_default_value);
 		Variant data = EditorSettings::get_singleton()->get_project_metadata(section, key, default_value);
-		result = _variant_to_jvalue(env, data.get_type(), &data, true);
+		result.l = _variant_to_jobject(env, data.get_type(), &data);
 	}
 #else
 	WARN_PRINT("Access to the Editor Settings Project Metadata is only available on Editor builds");

platform/android/jni_utils.cpp

@@ -90,61 +90,44 @@ String charsequence_to_string(JNIEnv *p_env, jobject p_charsequence) {
 	return result;
 }
 
-jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, bool force_jobject, int p_depth) {
-	jvalue value;
+jobject _variant_to_jobject(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, int p_depth) {
+	jobject ret = nullptr;
 
 	if (p_depth > Variant::MAX_RECURSION_DEPTH) {
 		ERR_PRINT("Variant is too deep! Bailing.");
-		value.i = 0;
-		return value;
+		return ret;
 	}
 
 	env->PushLocalFrame(2);
 	switch (p_type) {
 		case Variant::BOOL: {
-			if (force_jobject) {
-				jclass bclass = jni_find_class(env, "java/lang/Boolean");
-				jmethodID ctor = env->GetMethodID(bclass, "<init>", "(Z)V");
-				jvalue val;
-				val.z = (bool)(*p_arg);
-				jobject obj = env->NewObjectA(bclass, ctor, &val);
-				value.l = obj;
-				env->DeleteLocalRef(bclass);
-			} else {
-				value.z = *p_arg;
-			}
+			jclass bclass = jni_find_class(env, "java/lang/Boolean");
+			jmethodID ctor = env->GetMethodID(bclass, "<init>", "(Z)V");
+			jvalue val;
+			val.z = (bool)(*p_arg);
+			ret = env->NewObjectA(bclass, ctor, &val);
+			env->DeleteLocalRef(bclass);
 		} break;
 		case Variant::INT: {
-			if (force_jobject) {
-				jclass bclass = jni_find_class(env, "java/lang/Long");
-				jmethodID ctor = env->GetMethodID(bclass, "<init>", "(J)V");
-				jvalue val;
-				val.j = (jlong)(*p_arg);
-				jobject obj = env->NewObjectA(bclass, ctor, &val);
-				value.l = obj;
-				env->DeleteLocalRef(bclass);
-			} else {
-				value.j = (jlong)(*p_arg);
-			}
+			jclass bclass = jni_find_class(env, "java/lang/Long");
+			jmethodID ctor = env->GetMethodID(bclass, "<init>", "(J)V");
+			jvalue val;
+			val.j = (jlong)(*p_arg);
+			ret = env->NewObjectA(bclass, ctor, &val);
+			env->DeleteLocalRef(bclass);
 		} break;
 		case Variant::FLOAT: {
-			if (force_jobject) {
-				jclass bclass = jni_find_class(env, "java/lang/Double");
-				jmethodID ctor = env->GetMethodID(bclass, "<init>", "(D)V");
-				jvalue val;
-				val.d = (double)(*p_arg);
-				jobject obj = env->NewObjectA(bclass, ctor, &val);
-				value.l = obj;
-				env->DeleteLocalRef(bclass);
-
-			} else {
-				value.f = *p_arg;
-			}
+			jclass bclass = jni_find_class(env, "java/lang/Double");
+			jmethodID ctor = env->GetMethodID(bclass, "<init>", "(D)V");
+			jvalue val;
+			val.d = (double)(*p_arg);
+			ret = env->NewObjectA(bclass, ctor, &val);
+			env->DeleteLocalRef(bclass);
 		} break;
 		case Variant::STRING: {
 			String s = *p_arg;
 			jstring jStr = env->NewStringUTF(s.utf8().get_data());
-			value.l = jStr;
+			ret = jStr;
 		} break;
 		case Variant::PACKED_STRING_ARRAY: {
 			Vector<String> sarray = *p_arg;
@@ -155,13 +138,12 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
 				env->SetObjectArrayElement(arr, j, str);
 				env->DeleteLocalRef(str);
 			}
-			value.l = arr;
-
+			ret = arr;
 		} break;
 
 		case Variant::CALLABLE: {
 			jobject jcallable = callable_to_jcallable(env, *p_arg);
-			value.l = jcallable;
+			ret = jcallable;
 		} break;
 
 		case Variant::DICTIONARY: {
@@ -189,10 +171,10 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
 
 			for (int j = 0; j < keys.size(); j++) {
 				Variant var = dict[keys[j]];
-				jvalue valret = _variant_to_jvalue(env, var.get_type(), &var, true, p_depth + 1);
-				env->SetObjectArrayElement(jvalues, j, valret.l);
-				if (valret.l) {
-					env->DeleteLocalRef(valret.l);
+				jobject jvar = _variant_to_jobject(env, var.get_type(), &var, p_depth + 1);
+				env->SetObjectArrayElement(jvalues, j, jvar);
+				if (jvar) {
+					env->DeleteLocalRef(jvar);
 				}
 			}
 
@@ -202,7 +184,7 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
 			env->DeleteLocalRef(jvalues);
 			env->DeleteLocalRef(dclass);
 
-			value.l = jdict;
+			ret = jdict;
 		} break;
 
 		case Variant::ARRAY: {
@@ -211,13 +193,13 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
 
 			for (int j = 0; j < array.size(); j++) {
 				Variant var = array[j];
-				jvalue valret = _variant_to_jvalue(env, var.get_type(), &var, true, p_depth + 1);
-				env->SetObjectArrayElement(arr, j, valret.l);
-				if (valret.l) {
-					env->DeleteLocalRef(valret.l);
+				jobject jvar = _variant_to_jobject(env, var.get_type(), &var, p_depth + 1);
+				env->SetObjectArrayElement(arr, j, jvar);
+				if (jvar) {
+					env->DeleteLocalRef(jvar);
 				}
 			}
-			value.l = arr;
+			ret = arr;
 		} break;
 
 		case Variant::PACKED_INT32_ARRAY: {
@@ -225,57 +207,50 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
 			jintArray arr = env->NewIntArray(array.size());
 			const int *r = array.ptr();
 			env->SetIntArrayRegion(arr, 0, array.size(), r);
-			value.l = arr;
-
+			ret = arr;
 		} break;
 		case Variant::PACKED_INT64_ARRAY: {
 			Vector<int64_t> array = *p_arg;
 			jlongArray arr = env->NewLongArray(array.size());
 			const int64_t *r = array.ptr();
 			env->SetLongArrayRegion(arr, 0, array.size(), r);
-			value.l = arr;
-
+			ret = arr;
 		} break;
 		case Variant::PACKED_BYTE_ARRAY: {
 			Vector<uint8_t> array = *p_arg;
 			jbyteArray arr = env->NewByteArray(array.size());
 			const uint8_t *r = array.ptr();
 			env->SetByteArrayRegion(arr, 0, array.size(), reinterpret_cast<const signed char *>(r));
-			value.l = arr;
-
+			ret = arr;
 		} break;
 		case Variant::PACKED_FLOAT32_ARRAY: {
 			Vector<float> array = *p_arg;
 			jfloatArray arr = env->NewFloatArray(array.size());
 			const float *r = array.ptr();
 			env->SetFloatArrayRegion(arr, 0, array.size(), r);
-			value.l = arr;
-
+			ret = arr;
 		} break;
 		case Variant::PACKED_FLOAT64_ARRAY: {
 			Vector<double> array = *p_arg;
 			jdoubleArray arr = env->NewDoubleArray(array.size());
 			const double *r = array.ptr();
 			env->SetDoubleArrayRegion(arr, 0, array.size(), r);
-			value.l = arr;
-
+			ret = arr;
 		} break;
 		case Variant::OBJECT: {
 			Ref<JavaObject> generic_object = *p_arg;
 			if (generic_object.is_valid()) {
 				jobject obj = env->NewLocalRef(generic_object->get_instance());
-				value.l = obj;
-			} else {
-				value.i = 0;
+				ret = obj;
 			}
 		} break;
 
-		default: {
-			value.i = 0;
-		} break;
+		// Add default to prevent compiler warning about not handling all types.
+		default:
+			break;
 	}
-	value.l = env->PopLocalFrame(value.l);
-	return value;
+
+	return env->PopLocalFrame(ret);
 }
 
 String _get_class_name(JNIEnv *env, jclass cls, bool *array) {

platform/android/jni_utils.h

@@ -38,7 +38,7 @@
 
 #include <jni.h>
 
-jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, bool force_jobject = false, int p_depth = 0);
+jobject _variant_to_jobject(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, int p_depth = 0);
 
 String _get_class_name(JNIEnv *env, jclass cls, bool *array);
 

platform/android/variant/callable_jni.cpp

@@ -91,8 +91,7 @@ JNIEXPORT jobject JNICALL Java_org_godotengine_godot_variant_Callable_nativeCall
 		Callable::CallError err;
 		Variant result;
 		callable.callp(argptrs, count, result, err);
-		jvalue jresult = _variant_to_jvalue(p_env, result.get_type(), &result, true);
-		ret = jresult.l;
+		ret = _variant_to_jobject(p_env, result.get_type(), &result);
 	}
 
 	// Manually invoke the destructor to decrease the reference counts for the variant arguments.
@@ -107,8 +106,7 @@ JNIEXPORT jobject JNICALL Java_org_godotengine_godot_variant_Callable_nativeCall
 	Callable callable = _generate_callable(p_env, p_object_id, p_method_name, p_parameters);
 	if (callable.is_valid()) {
 		Variant result = callable.call();
-		jvalue jresult = _variant_to_jvalue(p_env, result.get_type(), &result, true);
-		return jresult.l;
+		return _variant_to_jobject(p_env, result.get_type(), &result);
 	} else {
 		return nullptr;
 	}