Merge pull request #90710 from m4gr3d/fix_jstring_leaks

Fix leakage of JNI object references
godotengine · Apr 22, 2024 · 8c474dd · 8c474dd
2 parents 4b66299 + f291a4e
commit 8c474dd
Show file tree

Hide file tree

Showing 20 changed files with 139 additions and 32 deletions.
diff --git a/modules/openxr/extensions/platform/openxr_android_extension.cpp b/modules/openxr/extensions/platform/openxr_android_extension.cpp
@@ -36,7 +36,6 @@
 #include "os_android.h"
 #include "thread_jandroid.h"
 
-#include <jni.h>
 #include <openxr/openxr.h>
 #include <openxr/openxr_platform.h>
 
@@ -48,6 +47,12 @@ OpenXRAndroidExtension *OpenXRAndroidExtension::get_singleton() {
 
 OpenXRAndroidExtension::OpenXRAndroidExtension() {
 	singleton = this;
+
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->GetJavaVM(&vm);
+	activity_object = env->NewGlobalRef(static_cast<OS_Android *>(OS::get_singleton())->get_godot_java()->get_activity());
 }
 
 HashMap<String, bool *> OpenXRAndroidExtension::get_requested_extensions() {
@@ -66,11 +71,6 @@ void OpenXRAndroidExtension::on_before_instance_created() {
 	}
 	loader_init_extension_available = true;
 
-	JNIEnv *env = get_jni_env();
-	JavaVM *vm;
-	env->GetJavaVM(&vm);
-	jobject activity_object = env->NewGlobalRef(static_cast<OS_Android *>(OS::get_singleton())->get_godot_java()->get_activity());
-
 	XrLoaderInitInfoAndroidKHR loader_init_info_android = {
 		.type = XR_TYPE_LOADER_INIT_INFO_ANDROID_KHR,
 		.next = nullptr,
@@ -93,11 +93,6 @@ void *OpenXRAndroidExtension::set_instance_create_info_and_get_next_pointer(void
 		return nullptr;
 	}
 
-	JNIEnv *env = get_jni_env();
-	JavaVM *vm;
-	env->GetJavaVM(&vm);
-	jobject activity_object = env->NewGlobalRef(static_cast<OS_Android *>(OS::get_singleton())->get_godot_java()->get_activity());
-
 	instance_create_info = {
 		.type = XR_TYPE_INSTANCE_CREATE_INFO_ANDROID_KHR,
 		.next = p_next_pointer,
@@ -109,4 +104,9 @@ void *OpenXRAndroidExtension::set_instance_create_info_and_get_next_pointer(void
 
 OpenXRAndroidExtension::~OpenXRAndroidExtension() {
 	singleton = nullptr;
+
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->DeleteGlobalRef(activity_object);
 }
diff --git a/modules/openxr/extensions/platform/openxr_android_extension.h b/modules/openxr/extensions/platform/openxr_android_extension.h
@@ -34,6 +34,8 @@
 #include "../../util.h"
 #include "../openxr_extension_wrapper.h"
 
+#include <jni.h>
+
 class OpenXRAndroidExtension : public OpenXRExtensionWrapper {
 public:
 	static OpenXRAndroidExtension *get_singleton();
@@ -49,6 +51,8 @@ class OpenXRAndroidExtension : public OpenXRExtensionWrapper {
 private:
 	static OpenXRAndroidExtension *singleton;
 
+	JavaVM *vm;
+	jobject activity_object;
 	bool loader_init_extension_available = false;
 	bool create_instance_extension_available = false;
 

diff --git a/platform/android/api/java_class_wrapper.h b/platform/android/api/java_class_wrapper.h
@@ -209,8 +209,6 @@ class JavaClassWrapper : public Object {
 #ifdef ANDROID_ENABLED
 	RBMap<String, Ref<JavaClass>> class_cache;
 	friend class JavaClass;
-	jclass activityClass;
-	jmethodID findClass;
 	jmethodID getDeclaredMethods;
 	jmethodID getFields;
 	jmethodID getParameterTypes;
@@ -229,7 +227,6 @@ class JavaClassWrapper : public Object {
 	jmethodID Long_longValue;
 	jmethodID Float_floatValue;
 	jmethodID Double_doubleValue;
-	jobject classLoader;
 
 	bool _get_type_sig(JNIEnv *env, jobject obj, uint32_t &sig, String &strsig);
 #endif

diff --git a/platform/android/api/jni_singleton.h b/platform/android/api/jni_singleton.h
@@ -239,6 +239,17 @@ class JNISingleton : public Object {
 	JNISingleton() {
 #ifdef ANDROID_ENABLED
 		instance = nullptr;
+#endif
+	}
+
+	~JNISingleton() {
+#ifdef ANDROID_ENABLED
+		if (instance) {
+			JNIEnv *env = get_jni_env();
+			ERR_FAIL_NULL(env);
+
+			env->DeleteGlobalRef(instance);
+		}
 #endif
 	}
 };

diff --git a/platform/android/dir_access_jandroid.cpp b/platform/android/dir_access_jandroid.cpp
@@ -321,6 +321,14 @@ void DirAccessJAndroid::setup(jobject p_dir_access_handler) {
 	_current_is_hidden = env->GetMethodID(cls, "isCurrentHidden", "(II)Z");
 }
 
+void DirAccessJAndroid::terminate() {
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->DeleteGlobalRef(cls);
+	env->DeleteGlobalRef(dir_access_handler);
+}
+
 DirAccessJAndroid::DirAccessJAndroid() {
 }
 

diff --git a/platform/android/dir_access_jandroid.h b/platform/android/dir_access_jandroid.h
@@ -89,6 +89,7 @@ class DirAccessJAndroid : public DirAccessUnix {
 	virtual uint64_t get_space_left() override;
 
 	static void setup(jobject p_dir_access_handler);
+	static void terminate();
 
 	DirAccessJAndroid();
 	~DirAccessJAndroid();

diff --git a/platform/android/file_access_android.cpp b/platform/android/file_access_android.cpp
@@ -31,8 +31,12 @@
 #include "file_access_android.h"
 
 #include "core/string/print_string.h"
+#include "thread_jandroid.h"
+
+#include <android/asset_manager_jni.h>
 
 AAssetManager *FileAccessAndroid::asset_manager = nullptr;
+jobject FileAccessAndroid::j_asset_manager = nullptr;
 
 String FileAccessAndroid::get_path() const {
 	return path_src;
@@ -257,3 +261,16 @@ void FileAccessAndroid::close() {
 FileAccessAndroid::~FileAccessAndroid() {
 	_close();
 }
+
+void FileAccessAndroid::setup(jobject p_asset_manager) {
+	JNIEnv *env = get_jni_env();
+	j_asset_manager = env->NewGlobalRef(p_asset_manager);
+	asset_manager = AAssetManager_fromJava(env, j_asset_manager);
+}
+
+void FileAccessAndroid::terminate() {
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->DeleteGlobalRef(j_asset_manager);
+}
diff --git a/platform/android/file_access_android.h b/platform/android/file_access_android.h
@@ -35,9 +35,13 @@
 
 #include <android/asset_manager.h>
 #include <android/log.h>
+#include <jni.h>
 #include <stdio.h>
 
 class FileAccessAndroid : public FileAccess {
+	static AAssetManager *asset_manager;
+	static jobject j_asset_manager;
+
 	mutable AAsset *asset = nullptr;
 	mutable uint64_t len = 0;
 	mutable uint64_t pos = 0;
@@ -48,8 +52,6 @@ class FileAccessAndroid : public FileAccess {
 	void _close();
 
 public:
-	static AAssetManager *asset_manager;
-
 	virtual Error open_internal(const String &p_path, int p_mode_flags) override; // open a file
 	virtual bool is_open() const override; // true when file is open
 
@@ -93,6 +95,10 @@ class FileAccessAndroid : public FileAccess {
 
 	virtual void close() override;
 
+	static void setup(jobject p_asset_manager);
+
+	static void terminate();
+
 	~FileAccessAndroid();
 };
 

diff --git a/platform/android/file_access_filesystem_jandroid.cpp b/platform/android/file_access_filesystem_jandroid.cpp
@@ -408,6 +408,14 @@ void FileAccessFilesystemJAndroid::setup(jobject p_file_access_handler) {
 	_file_resize = env->GetMethodID(cls, "fileResize", "(IJ)I");
 }
 
+void FileAccessFilesystemJAndroid::terminate() {
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->DeleteGlobalRef(cls);
+	env->DeleteGlobalRef(file_access_handler);
+}
+
 void FileAccessFilesystemJAndroid::close() {
 	if (is_open()) {
 		_close();

diff --git a/platform/android/file_access_filesystem_jandroid.h b/platform/android/file_access_filesystem_jandroid.h
@@ -97,6 +97,7 @@ class FileAccessFilesystemJAndroid : public FileAccess {
 	virtual bool file_exists(const String &p_path) override; ///< return true if a file exists
 
 	static void setup(jobject p_file_access_handler);
+	static void terminate();
 
 	virtual uint64_t _get_modified_time(const String &p_file) override;
 	virtual BitField<FileAccess::UnixPermissionFlags> _get_unix_permissions(const String &p_file) override { return 0; }

diff --git a/platform/android/java_class_wrapper.cpp b/platform/android/java_class_wrapper.cpp
@@ -1157,50 +1157,54 @@ JavaClassWrapper::JavaClassWrapper(jobject p_activity) {
 	JNIEnv *env = get_jni_env();
 	ERR_FAIL_NULL(env);
 
-	jclass activity = env->FindClass("android/app/Activity");
-	jmethodID getClassLoader = env->GetMethodID(activity, "getClassLoader", "()Ljava/lang/ClassLoader;");
-	classLoader = env->CallObjectMethod(p_activity, getClassLoader);
-	classLoader = (jclass)env->NewGlobalRef(classLoader);
-	jclass classLoaderClass = env->FindClass("java/lang/ClassLoader");
-	findClass = env->GetMethodID(classLoaderClass, "loadClass", "(Ljava/lang/String;)Ljava/lang/Class;");
-
 	jclass bclass = env->FindClass("java/lang/Class");
 	getDeclaredMethods = env->GetMethodID(bclass, "getDeclaredMethods", "()[Ljava/lang/reflect/Method;");
 	getFields = env->GetMethodID(bclass, "getFields", "()[Ljava/lang/reflect/Field;");
 	Class_getName = env->GetMethodID(bclass, "getName", "()Ljava/lang/String;");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/reflect/Method");
 	getParameterTypes = env->GetMethodID(bclass, "getParameterTypes", "()[Ljava/lang/Class;");
 	getReturnType = env->GetMethodID(bclass, "getReturnType", "()Ljava/lang/Class;");
 	getName = env->GetMethodID(bclass, "getName", "()Ljava/lang/String;");
 	getModifiers = env->GetMethodID(bclass, "getModifiers", "()I");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/reflect/Field");
 	Field_getName = env->GetMethodID(bclass, "getName", "()Ljava/lang/String;");
 	Field_getModifiers = env->GetMethodID(bclass, "getModifiers", "()I");
 	Field_get = env->GetMethodID(bclass, "get", "(Ljava/lang/Object;)Ljava/lang/Object;");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Boolean");
 	Boolean_booleanValue = env->GetMethodID(bclass, "booleanValue", "()Z");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Byte");
 	Byte_byteValue = env->GetMethodID(bclass, "byteValue", "()B");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Character");
 	Character_characterValue = env->GetMethodID(bclass, "charValue", "()C");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Short");
 	Short_shortValue = env->GetMethodID(bclass, "shortValue", "()S");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Integer");
 	Integer_integerValue = env->GetMethodID(bclass, "intValue", "()I");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Long");
 	Long_longValue = env->GetMethodID(bclass, "longValue", "()J");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Float");
 	Float_floatValue = env->GetMethodID(bclass, "floatValue", "()F");
+	env->DeleteLocalRef(bclass);
 
 	bclass = env->FindClass("java/lang/Double");
 	Double_doubleValue = env->GetMethodID(bclass, "doubleValue", "()D");
+	env->DeleteLocalRef(bclass);
 }
diff --git a/platform/android/java_godot_io_wrapper.cpp b/platform/android/java_godot_io_wrapper.cpp
@@ -70,7 +70,11 @@ GodotIOJavaWrapper::GodotIOJavaWrapper(JNIEnv *p_env, jobject p_godot_io_instanc
 }
 
 GodotIOJavaWrapper::~GodotIOJavaWrapper() {
-	// nothing to do here for now
+	JNIEnv *env = get_jni_env();
+	ERR_FAIL_NULL(env);
+
+	env->DeleteGlobalRef(cls);
+	env->DeleteGlobalRef(godot_io_instance);
 }
 
 jobject GodotIOJavaWrapper::get_instance() {
@@ -82,7 +86,9 @@ Error GodotIOJavaWrapper::open_uri(const String &p_uri) {
 		JNIEnv *env = get_jni_env();
 		ERR_FAIL_NULL_V(env, ERR_UNAVAILABLE);
 		jstring jStr = env->NewStringUTF(p_uri.utf8().get_data());
-		return env->CallIntMethod(godot_io_instance, _open_URI, jStr) ? ERR_CANT_OPEN : OK;
+		Error result = env->CallIntMethod(godot_io_instance, _open_URI, jStr) ? ERR_CANT_OPEN : OK;
+		env->DeleteLocalRef(jStr);
+		return result;
 	} else {
 		return ERR_UNAVAILABLE;
 	}
@@ -220,6 +226,7 @@ void GodotIOJavaWrapper::show_vk(const String &p_existing, int p_type, int p_max
 		ERR_FAIL_NULL(env);
 		jstring jStr = env->NewStringUTF(p_existing.utf8().get_data());
 		env->CallVoidMethod(godot_io_instance, _show_keyboard, jStr, p_type, p_max_input_length, p_cursor_start, p_cursor_end);
+		env->DeleteLocalRef(jStr);
 	}
 }
 

diff --git a/platform/android/java_godot_lib_jni.cpp b/platform/android/java_godot_lib_jni.cpp
@@ -95,6 +95,13 @@ static void _terminate(JNIEnv *env, bool p_restart = false) {
 	if (godot_io_java) {
 		delete godot_io_java;
 	}
+
+	TTS_Android::terminate();
+	FileAccessAndroid::terminate();
+	DirAccessJAndroid::terminate();
+	FileAccessFilesystemJAndroid::terminate();
+	NetSocketAndroid::terminate();
+
 	if (godot_java) {
 		if (!restart_on_cleanup) {
 			if (p_restart) {
@@ -125,10 +132,7 @@ JNIEXPORT jboolean JNICALL Java_org_godotengine_godot_GodotLib_initialize(JNIEnv
 
 	init_thread_jandroid(jvm, env);
 
-	jobject amgr = env->NewGlobalRef(p_asset_manager);
-
-	FileAccessAndroid::asset_manager = AAssetManager_fromJava(env, amgr);
-
+	FileAccessAndroid::setup(p_asset_manager);
 	DirAccessJAndroid::setup(p_directory_access_handler);
 	FileAccessFilesystemJAndroid::setup(p_file_access_handler);
 	NetSocketAndroid::setup(p_net_utils);

diff --git a/platform/android/java_godot_view_wrapper.cpp b/platform/android/java_godot_view_wrapper.cpp
@@ -95,6 +95,7 @@ void GodotJavaViewWrapper::configure_pointer_icon(int pointer_type, const String
 
 		jstring jImagePath = env->NewStringUTF(image_path.utf8().get_data());
 		env->CallVoidMethod(_godot_view, _configure_pointer_icon, pointer_type, jImagePath, p_hotspot.x, p_hotspot.y);
+		env->DeleteLocalRef(jImagePath);
 	}
 }