Skip to content

Commit b22d153

Browse files
akien-mgaakien-mga
committed
Merge pull request #113159 from dsnopek/android-variant-to-jvalue-memory-issues
Android: Fix memory issues in `_variant_to_jvalue()`
2 parents 30f1ab1 + c2f8d1a commit b22d153

File tree

7 files changed

+70
-77
lines changed

7 files changed

+70
-77
lines changed

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,10 @@ func assert_equal(actual, expected):
4242
else:
4343
__assert_fail()
4444
print (" |-> Expected '%s' but got '%s'" % [expected, actual])
45+
46+
func assert_true(value):
47+
if value:
48+
__assert_pass()
49+
else:
50+
__assert_fail()
51+
print (" |-> Expected '%s' to be truthy" % value)

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ func run_tests():
1818

1919
__exec_test(test_big_integers)
2020

21+
__exec_test(test_callable)
22+
2123
print("JavaClassWrapper tests finished.")
2224
print("Tests started: " + str(_test_started))
2325
print("Tests completed: " + str(_test_completed))
@@ -142,3 +144,14 @@ func test_big_integers():
142144
assert_equal(TestClass.testArgLong(4242424242), "4242424242")
143145
assert_equal(TestClass.testArgLong(-4242424242), "-4242424242")
144146
assert_equal(TestClass.testDictionary({a = 4242424242, b = -4242424242}), "{a=4242424242, b=-4242424242}")
147+
148+
func test_callable():
149+
var android_runtime = Engine.get_singleton("AndroidRuntime")
150+
assert_true(android_runtime != null)
151+
152+
var cb1_data := {called = false}
153+
var cb1 = func():
154+
cb1_data['called'] = true
155+
return null
156+
android_runtime.createRunnableFromGodotCallable(cb1).run()
157+
assert_equal(cb1_data['called'], true)

‎platform/android/java_class_wrapper.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ bool JavaClass::_call_method(JavaObject *p_instance, const StringName &p_method,
393393
} break;
394394
case ARG_TYPE_CLASS: {
395395
if (p_args[i]->get_type() == Variant::DICTIONARY) {
396-
argv[i].l = _variant_to_jvalue(env, Variant::DICTIONARY, p_args[i]).l;
396+
argv[i].l = _variant_to_jobject(env, Variant::DICTIONARY, p_args[i]);
397397
} else {
398398
Ref<JavaObject> jo = *p_args[i];
399399
if (jo.is_valid()) {

‎platform/android/java_godot_lib_jni.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -536,7 +536,7 @@ JNIEXPORT jobject JNICALL Java_org_godotengine_godot_GodotLib_getEditorProjectMe
536536
String key = jstring_to_string(p_key, env);
537537
Variant default_value = _jobject_to_variant(env, p_default_value);
538538
Variant data = EditorSettings::get_singleton()->get_project_metadata(section, key, default_value);
539-
result = _variant_to_jvalue(env, data.get_type(), &data, true);
539+
result.l = _variant_to_jobject(env, data.get_type(), &data);
540540
}
541541
#else
542542
WARN_PRINT("Access to the Editor Settings Project Metadata is only available on Editor builds");

‎platform/android/jni_utils.cpp‎

Lines changed: 45 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -90,61 +90,44 @@ String charsequence_to_string(JNIEnv *p_env, jobject p_charsequence) {
9090
return result;
9191
}
9292

93-
jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, bool force_jobject, int p_depth) {
94-
jvalue value;
93+
jobject _variant_to_jobject(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, int p_depth) {
94+
jobject ret = nullptr;
9595

9696
if (p_depth > Variant::MAX_RECURSION_DEPTH) {
9797
ERR_PRINT("Variant is too deep! Bailing.");
98-
value.i = 0;
99-
return value;
98+
return ret;
10099
}
101100

102101
env->PushLocalFrame(2);
103102
switch (p_type) {
104103
case Variant::BOOL: {
105-
if (force_jobject) {
106-
jclass bclass = jni_find_class(env, "java/lang/Boolean");
107-
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(Z)V");
108-
jvalue val;
109-
val.z = (bool)(*p_arg);
110-
jobject obj = env->NewObjectA(bclass, ctor, &val);
111-
value.l = obj;
112-
env->DeleteLocalRef(bclass);
113-
} else {
114-
value.z = *p_arg;
115-
}
104+
jclass bclass = jni_find_class(env, "java/lang/Boolean");
105+
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(Z)V");
106+
jvalue val;
107+
val.z = (bool)(*p_arg);
108+
ret = env->NewObjectA(bclass, ctor, &val);
109+
env->DeleteLocalRef(bclass);
116110
} break;
117111
case Variant::INT: {
118-
if (force_jobject) {
119-
jclass bclass = jni_find_class(env, "java/lang/Long");
120-
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(J)V");
121-
jvalue val;
122-
val.j = (jlong)(*p_arg);
123-
jobject obj = env->NewObjectA(bclass, ctor, &val);
124-
value.l = obj;
125-
env->DeleteLocalRef(bclass);
126-
} else {
127-
value.j = (jlong)(*p_arg);
128-
}
112+
jclass bclass = jni_find_class(env, "java/lang/Long");
113+
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(J)V");
114+
jvalue val;
115+
val.j = (jlong)(*p_arg);
116+
ret = env->NewObjectA(bclass, ctor, &val);
117+
env->DeleteLocalRef(bclass);
129118
} break;
130119
case Variant::FLOAT: {
131-
if (force_jobject) {
132-
jclass bclass = jni_find_class(env, "java/lang/Double");
133-
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(D)V");
134-
jvalue val;
135-
val.d = (double)(*p_arg);
136-
jobject obj = env->NewObjectA(bclass, ctor, &val);
137-
value.l = obj;
138-
env->DeleteLocalRef(bclass);
139-
140-
} else {
141-
value.f = *p_arg;
142-
}
120+
jclass bclass = jni_find_class(env, "java/lang/Double");
121+
jmethodID ctor = env->GetMethodID(bclass, "<init>", "(D)V");
122+
jvalue val;
123+
val.d = (double)(*p_arg);
124+
ret = env->NewObjectA(bclass, ctor, &val);
125+
env->DeleteLocalRef(bclass);
143126
} break;
144127
case Variant::STRING: {
145128
String s = *p_arg;
146129
jstring jStr = env->NewStringUTF(s.utf8().get_data());
147-
value.l = jStr;
130+
ret = jStr;
148131
} break;
149132
case Variant::PACKED_STRING_ARRAY: {
150133
Vector<String> sarray = *p_arg;
@@ -155,13 +138,12 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
155138
env->SetObjectArrayElement(arr, j, str);
156139
env->DeleteLocalRef(str);
157140
}
158-
value.l = arr;
159-
141+
ret = arr;
160142
} break;
161143

162144
case Variant::CALLABLE: {
163145
jobject jcallable = callable_to_jcallable(env, *p_arg);
164-
value.l = jcallable;
146+
ret = jcallable;
165147
} break;
166148

167149
case Variant::DICTIONARY: {
@@ -189,10 +171,10 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
189171

190172
for (int j = 0; j < keys.size(); j++) {
191173
Variant var = dict[keys[j]];
192-
jvalue valret = _variant_to_jvalue(env, var.get_type(), &var, true, p_depth + 1);
193-
env->SetObjectArrayElement(jvalues, j, valret.l);
194-
if (valret.l) {
195-
env->DeleteLocalRef(valret.l);
174+
jobject jvar = _variant_to_jobject(env, var.get_type(), &var, p_depth + 1);
175+
env->SetObjectArrayElement(jvalues, j, jvar);
176+
if (jvar) {
177+
env->DeleteLocalRef(jvar);
196178
}
197179
}
198180

@@ -202,7 +184,7 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
202184
env->DeleteLocalRef(jvalues);
203185
env->DeleteLocalRef(dclass);
204186

205-
value.l = jdict;
187+
ret = jdict;
206188
} break;
207189

208190
case Variant::ARRAY: {
@@ -211,71 +193,64 @@ jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_ar
211193

212194
for (int j = 0; j < array.size(); j++) {
213195
Variant var = array[j];
214-
jvalue valret = _variant_to_jvalue(env, var.get_type(), &var, true, p_depth + 1);
215-
env->SetObjectArrayElement(arr, j, valret.l);
216-
if (valret.l) {
217-
env->DeleteLocalRef(valret.l);
196+
jobject jvar = _variant_to_jobject(env, var.get_type(), &var, p_depth + 1);
197+
env->SetObjectArrayElement(arr, j, jvar);
198+
if (jvar) {
199+
env->DeleteLocalRef(jvar);
218200
}
219201
}
220-
value.l = arr;
202+
ret = arr;
221203
} break;
222204

223205
case Variant::PACKED_INT32_ARRAY: {
224206
Vector<int> array = *p_arg;
225207
jintArray arr = env->NewIntArray(array.size());
226208
const int *r = array.ptr();
227209
env->SetIntArrayRegion(arr, 0, array.size(), r);
228-
value.l = arr;
229-
210+
ret = arr;
230211
} break;
231212
case Variant::PACKED_INT64_ARRAY: {
232213
Vector<int64_t> array = *p_arg;
233214
jlongArray arr = env->NewLongArray(array.size());
234215
const int64_t *r = array.ptr();
235216
env->SetLongArrayRegion(arr, 0, array.size(), r);
236-
value.l = arr;
237-
217+
ret = arr;
238218
} break;
239219
case Variant::PACKED_BYTE_ARRAY: {
240220
Vector<uint8_t> array = *p_arg;
241221
jbyteArray arr = env->NewByteArray(array.size());
242222
const uint8_t *r = array.ptr();
243223
env->SetByteArrayRegion(arr, 0, array.size(), reinterpret_cast<const signed char *>(r));
244-
value.l = arr;
245-
224+
ret = arr;
246225
} break;
247226
case Variant::PACKED_FLOAT32_ARRAY: {
248227
Vector<float> array = *p_arg;
249228
jfloatArray arr = env->NewFloatArray(array.size());
250229
const float *r = array.ptr();
251230
env->SetFloatArrayRegion(arr, 0, array.size(), r);
252-
value.l = arr;
253-
231+
ret = arr;
254232
} break;
255233
case Variant::PACKED_FLOAT64_ARRAY: {
256234
Vector<double> array = *p_arg;
257235
jdoubleArray arr = env->NewDoubleArray(array.size());
258236
const double *r = array.ptr();
259237
env->SetDoubleArrayRegion(arr, 0, array.size(), r);
260-
value.l = arr;
261-
238+
ret = arr;
262239
} break;
263240
case Variant::OBJECT: {
264241
Ref<JavaObject> generic_object = *p_arg;
265242
if (generic_object.is_valid()) {
266243
jobject obj = env->NewLocalRef(generic_object->get_instance());
267-
value.l = obj;
268-
} else {
269-
value.i = 0;
244+
ret = obj;
270245
}
271246
} break;
272247

273-
default: {
274-
value.i = 0;
275-
} break;
248+
// Add default to prevent compiler warning about not handling all types.
249+
default:
250+
break;
276251
}
277-
value.l = env->PopLocalFrame(value.l);
278-
return value;
252+
253+
return env->PopLocalFrame(ret);
279254
}
280255

281256
String _get_class_name(JNIEnv *env, jclass cls, bool *array) {

‎platform/android/jni_utils.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
#include <jni.h>
4040

41-
jvalue _variant_to_jvalue(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, bool force_jobject = false, int p_depth = 0);
41+
jobject _variant_to_jobject(JNIEnv *env, Variant::Type p_type, const Variant *p_arg, int p_depth = 0);
4242

4343
String _get_class_name(JNIEnv *env, jclass cls, bool *array);
4444

‎platform/android/variant/callable_jni.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,7 @@ JNIEXPORT jobject JNICALL Java_org_godotengine_godot_variant_Callable_nativeCall
9191
Callable::CallError err;
9292
Variant result;
9393
callable.callp(argptrs, count, result, err);
94-
jvalue jresult = _variant_to_jvalue(p_env, result.get_type(), &result, true);
95-
ret = jresult.l;
94+
ret = _variant_to_jobject(p_env, result.get_type(), &result);
9695
}
9796

9897
// 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
107106
Callable callable = _generate_callable(p_env, p_object_id, p_method_name, p_parameters);
108107
if (callable.is_valid()) {
109108
Variant result = callable.call();
110-
jvalue jresult = _variant_to_jvalue(p_env, result.get_type(), &result, true);
111-
return jresult.l;
109+
return _variant_to_jobject(p_env, result.get_type(), &result);
112110
} else {
113111
return nullptr;
114112
}

0 commit comments

Comments
 (0)