From a391ba162234a3a0a5a17201eb3b16253310d593 Mon Sep 17 00:00:00 2001 From: Connor McLaughlin Date: Fri, 11 Dec 2020 00:10:35 +1000 Subject: [PATCH] Android: Clean up local references on emu thread and loops Should hopefully fix the runtime killing our app on some devices. --- .../app/src/cpp/android_host_interface.cpp | 64 +++++++--- android/app/src/cpp/android_host_interface.h | 45 ++++++- .../src/cpp/android_settings_interface.cpp | 115 ++++++++++++------ .../app/src/cpp/android_settings_interface.h | 2 + 4 files changed, 168 insertions(+), 58 deletions(-) diff --git a/android/app/src/cpp/android_host_interface.cpp b/android/app/src/cpp/android_host_interface.cpp index defb8f514..6fd0796c5 100644 --- a/android/app/src/cpp/android_host_interface.cpp +++ b/android/app/src/cpp/android_host_interface.cpp @@ -36,6 +36,7 @@ static jfieldID s_AndroidHostInterface_field_mNativePointer; static jmethodID s_AndroidHostInterface_method_reportError; static jmethodID s_AndroidHostInterface_method_reportMessage; static jmethodID s_AndroidHostInterface_method_openAssetStream; +static jclass s_EmulationActivity_class; static jmethodID s_EmulationActivity_method_reportError; static jmethodID s_EmulationActivity_method_reportMessage; static jmethodID s_EmulationActivity_method_onEmulationStarted; @@ -107,6 +108,8 @@ std::unique_ptr ReadInputStreamToMemory(JNIEnv* env, j } bs->Resize(position); + env->DeleteLocalRef(temp); + env->DeleteLocalRef(cls); return bs; } } // namespace AndroidHelpers @@ -156,6 +159,7 @@ void AndroidHostInterface::ReportError(const char* message) env->CallVoidMethod(m_emulation_activity_object, s_EmulationActivity_method_reportError, message_jstr); else env->CallVoidMethod(m_java_object, s_AndroidHostInterface_method_reportError, message_jstr); + env->DeleteLocalRef(message_jstr); } void AndroidHostInterface::ReportMessage(const char* message) @@ -168,6 +172,7 @@ void AndroidHostInterface::ReportMessage(const char* message) env->CallVoidMethod(m_emulation_activity_object, s_EmulationActivity_method_reportMessage, message_jstr); else env->CallVoidMethod(m_java_object, s_AndroidHostInterface_method_reportMessage, message_jstr); + env->DeleteLocalRef(message_jstr); } std::string AndroidHostInterface::GetStringSettingValue(const char* section, const char* key, const char* default_value) @@ -205,7 +210,9 @@ std::unique_ptr AndroidHostInterface::OpenPackageFile(const char* pa return {}; } - return AndroidHelpers::ReadInputStreamToMemory(env, stream, 65536); + std::unique_ptr ret(AndroidHelpers::ReadInputStreamToMemory(env, stream, 65536)); + env->DeleteLocalRef(stream); + return ret; } void AndroidHostInterface::SetUserDirectory() @@ -517,6 +524,7 @@ void AndroidHostInterface::OnRunningGameChanged() JNIEnv* env = AndroidHelpers::GetJNIEnv(); jstring title_string = env->NewStringUTF(System::GetRunningTitle().c_str()); env->CallVoidMethod(m_emulation_activity_object, s_EmulationActivity_method_onGameTitleChanged, title_string); + env->DeleteLocalRef(title_string); } } @@ -736,19 +744,22 @@ extern "C" jint JNI_OnLoad(JavaVM* vm, void* reserved) // Create global reference so it doesn't get cleaned up. JNIEnv* env = AndroidHelpers::GetJNIEnv(); - if ((s_String_class = env->FindClass("java/lang/String")) == nullptr || - (s_String_class = static_cast(env->NewGlobalRef(s_String_class))) == nullptr || - (s_AndroidHostInterface_class = env->FindClass("com/github/stenzek/duckstation/AndroidHostInterface")) == - nullptr || - (s_AndroidHostInterface_class = static_cast(env->NewGlobalRef(s_AndroidHostInterface_class))) == - nullptr || - (s_PatchCode_class = env->FindClass("com/github/stenzek/duckstation/PatchCode")) == nullptr || - (s_PatchCode_class = static_cast(env->NewGlobalRef(s_PatchCode_class))) == nullptr) + jclass string_class, host_interface_class, patch_code_class; + if ((string_class = env->FindClass("java/lang/String")) == nullptr || + (s_String_class = static_cast(env->NewGlobalRef(string_class))) == nullptr || + (host_interface_class = env->FindClass("com/github/stenzek/duckstation/AndroidHostInterface")) == nullptr || + (s_AndroidHostInterface_class = static_cast(env->NewGlobalRef(host_interface_class))) == nullptr || + (patch_code_class = env->FindClass("com/github/stenzek/duckstation/PatchCode")) == nullptr || + (s_PatchCode_class = static_cast(env->NewGlobalRef(patch_code_class))) == nullptr) { Log_ErrorPrint("AndroidHostInterface class lookup failed"); return -1; } + env->DeleteLocalRef(string_class); + env->DeleteLocalRef(host_interface_class); + env->DeleteLocalRef(patch_code_class); + jclass emulation_activity_class; if ((s_AndroidHostInterface_constructor = env->GetMethodID(s_AndroidHostInterface_class, "", "(Landroid/content/Context;)V")) == nullptr || @@ -761,16 +772,17 @@ extern "C" jint JNI_OnLoad(JavaVM* vm, void* reserved) (s_AndroidHostInterface_method_openAssetStream = env->GetMethodID( s_AndroidHostInterface_class, "openAssetStream", "(Ljava/lang/String;)Ljava/io/InputStream;")) == nullptr || (emulation_activity_class = env->FindClass("com/github/stenzek/duckstation/EmulationActivity")) == nullptr || + (s_EmulationActivity_class = static_cast(env->NewGlobalRef(emulation_activity_class))) == nullptr || (s_EmulationActivity_method_reportError = - env->GetMethodID(emulation_activity_class, "reportError", "(Ljava/lang/String;)V")) == nullptr || + env->GetMethodID(s_EmulationActivity_class, "reportError", "(Ljava/lang/String;)V")) == nullptr || (s_EmulationActivity_method_reportMessage = - env->GetMethodID(emulation_activity_class, "reportMessage", "(Ljava/lang/String;)V")) == nullptr || + env->GetMethodID(s_EmulationActivity_class, "reportMessage", "(Ljava/lang/String;)V")) == nullptr || (s_EmulationActivity_method_onEmulationStarted = - env->GetMethodID(emulation_activity_class, "onEmulationStarted", "()V")) == nullptr || + env->GetMethodID(s_EmulationActivity_class, "onEmulationStarted", "()V")) == nullptr || (s_EmulationActivity_method_onEmulationStopped = - env->GetMethodID(emulation_activity_class, "onEmulationStopped", "()V")) == nullptr || + env->GetMethodID(s_EmulationActivity_class, "onEmulationStopped", "()V")) == nullptr || (s_EmulationActivity_method_onGameTitleChanged = - env->GetMethodID(emulation_activity_class, "onGameTitleChanged", "(Ljava/lang/String;)V")) == nullptr || + env->GetMethodID(s_EmulationActivity_class, "onGameTitleChanged", "(Ljava/lang/String;)V")) == nullptr || (s_EmulationActivity_method_setVibration = env->GetMethodID(emulation_activity_class, "setVibration", "(Z)V")) == nullptr || (s_PatchCode_constructor = env->GetMethodID(s_PatchCode_class, "", "(ILjava/lang/String;Z)V")) == nullptr) @@ -779,6 +791,8 @@ extern "C" jint JNI_OnLoad(JavaVM* vm, void* reserved) return -1; } + env->DeleteLocalRef(emulation_activity_class); + return JNI_VERSION_1_6; } @@ -795,7 +809,8 @@ DEFINE_JNI_ARGS_METHOD(jstring, AndroidHostInterface_getScmVersion, jobject unus DEFINE_JNI_ARGS_METHOD(jstring, AndroidHostInterface_getFullScmVersion, jobject unused) { - return env->NewStringUTF(SmallString::FromFormat("DuckStation for Android %s (%s)\nBuilt %s %s", g_scm_tag_str, g_scm_branch_str, __DATE__, __TIME__)); + return env->NewStringUTF(SmallString::FromFormat("DuckStation for Android %s (%s)\nBuilt %s %s", g_scm_tag_str, + g_scm_branch_str, __DATE__, __TIME__)); } DEFINE_JNI_ARGS_METHOD(jobject, AndroidHostInterface_create, jobject unused, jobject context_object, @@ -969,6 +984,17 @@ DEFINE_JNI_ARGS_METHOD(jarray, AndroidHostInterface_getGameListEntries, jobject modified_time, region, type, compatibility_rating, cover_path); env->SetObjectArrayElement(entry_array, counter++, entry_jobject); + env->DeleteLocalRef(entry_jobject); + env->DeleteLocalRef(modified_time); + if (cover_path) + env->DeleteLocalRef(cover_path); + env->DeleteLocalRef(compatibility_rating); + env->DeleteLocalRef(type); + env->DeleteLocalRef(region); + env->DeleteLocalRef(file_title); + env->DeleteLocalRef(title); + env->DeleteLocalRef(code); + env->DeleteLocalRef(path); } return entry_array; @@ -1068,9 +1094,12 @@ DEFINE_JNI_ARGS_METHOD(jobject, AndroidHostInterface_getPatchCodeList, jobject o { const CheatCode& cc = cl->GetCode(i); - jobject java_cc = env->NewObject(s_PatchCode_class, s_PatchCode_constructor, static_cast(i), - env->NewStringUTF(cc.description.c_str()), cc.enabled); + jstring desc_str = env->NewStringUTF(cc.description.c_str()); + jobject java_cc = + env->NewObject(s_PatchCode_class, s_PatchCode_constructor, static_cast(i), desc_str, cc.enabled); env->SetObjectArrayElement(arr, i, java_cc); + env->DeleteLocalRef(java_cc); + env->DeleteLocalRef(desc_str); } return arr; @@ -1162,6 +1191,7 @@ DEFINE_JNI_ARGS_METHOD(jobjectArray, AndroidHostInterface_getMediaPlaylistPaths, { jstring str = env->NewStringUTF(System::GetMediaPlaylistPath(i).c_str()); env->SetObjectArrayElement(arr, static_cast(i), str); + env->DeleteLocalRef(str); } return arr; diff --git a/android/app/src/cpp/android_host_interface.h b/android/app/src/cpp/android_host_interface.h index 4bedd9b41..58e572e64 100644 --- a/android/app/src/cpp/android_host_interface.h +++ b/android/app/src/cpp/android_host_interface.h @@ -1,8 +1,8 @@ #pragma once #include "android_settings_interface.h" +#include "common/byte_stream.h" #include "common/event.h" #include "common/progress_callback.h" -#include "common/byte_stream.h" #include "frontend-common/common_host_interface.h" #include #include @@ -109,3 +109,46 @@ AndroidHostInterface* GetNativeClass(JNIEnv* env, jobject obj); std::string JStringToString(JNIEnv* env, jstring str); std::unique_ptr ReadInputStreamToMemory(JNIEnv* env, jobject obj, u32 chunk_size = 65536); } // namespace AndroidHelpers + +template +class LocalRefHolder +{ +public: + LocalRefHolder() : m_env(nullptr), m_object(nullptr) {} + + LocalRefHolder(JNIEnv* env, T object) : m_env(env), m_object(object) {} + + LocalRefHolder(const LocalRefHolder&) = delete; + LocalRefHolder(LocalRefHolder&& move) : m_env(move.m_env), m_object(move.m_object) + { + move.m_env = nullptr; + move.m_object = {}; + } + + ~LocalRefHolder() + { + if (m_object) + m_env->DeleteLocalRef(m_object); + } + + operator T() const { return m_object; } + T operator*() const { return m_object; } + + LocalRefHolder& operator=(const LocalRefHolder&) = delete; + LocalRefHolder& operator=(LocalRefHolder&& move) + { + if (m_object) + m_env->DeleteLocalRef(m_object); + m_env = move.m_env; + m_object = move.m_object; + move.m_env = nullptr; + move.m_object = {}; + return *this; + } + + T Get() const { return m_object; } + +private: + JNIEnv* m_env; + T m_object; +}; diff --git a/android/app/src/cpp/android_settings_interface.cpp b/android/app/src/cpp/android_settings_interface.cpp index 016d40f01..94ebf2d9d 100644 --- a/android/app/src/cpp/android_settings_interface.cpp +++ b/android/app/src/cpp/android_settings_interface.cpp @@ -21,28 +21,43 @@ AndroidSettingsInterface::AndroidSettingsInterface(jobject java_context) env->GetStaticMethodID(c_preference_manager, "getDefaultSharedPreferences", "(Landroid/content/Context;)Landroid/content/SharedPreferences;"); Assert(c_preference_manager && c_set && m_get_default_shared_preferences); + m_set_class = reinterpret_cast(env->NewGlobalRef(c_set)); + Assert(m_set_class); + env->DeleteLocalRef(c_set); - m_java_shared_preferences = + jobject shared_preferences = env->CallStaticObjectMethod(c_preference_manager, m_get_default_shared_preferences, java_context); + Assert(shared_preferences); + m_java_shared_preferences = env->NewGlobalRef(shared_preferences); Assert(m_java_shared_preferences); - m_java_shared_preferences = env->NewGlobalRef(m_java_shared_preferences); - jclass c_shared_preferences = env->GetObjectClass(m_java_shared_preferences); + env->DeleteLocalRef(c_preference_manager); + env->DeleteLocalRef(shared_preferences); - m_get_boolean = env->GetMethodID(c_shared_preferences, "getBoolean", "(Ljava/lang/String;Z)Z"); - m_get_int = env->GetMethodID(c_shared_preferences, "getInt", "(Ljava/lang/String;I)I"); - m_get_float = env->GetMethodID(c_shared_preferences, "getFloat", "(Ljava/lang/String;F)F"); - m_get_string = - env->GetMethodID(c_shared_preferences, "getString", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"); + jclass c_shared_preferences = env->GetObjectClass(m_java_shared_preferences); + m_shared_preferences_class = reinterpret_cast(env->NewGlobalRef(c_shared_preferences)); + Assert(m_shared_preferences_class); + env->DeleteLocalRef(c_shared_preferences); + + m_get_boolean = env->GetMethodID(m_shared_preferences_class, "getBoolean", "(Ljava/lang/String;Z)Z"); + m_get_int = env->GetMethodID(m_shared_preferences_class, "getInt", "(Ljava/lang/String;I)I"); + m_get_float = env->GetMethodID(m_shared_preferences_class, "getFloat", "(Ljava/lang/String;F)F"); + m_get_string = env->GetMethodID(m_shared_preferences_class, "getString", + "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"); m_get_string_set = - env->GetMethodID(c_shared_preferences, "getStringSet", "(Ljava/lang/String;Ljava/util/Set;)Ljava/util/Set;"); - m_set_to_array = env->GetMethodID(c_set, "toArray", "()[Ljava/lang/Object;"); + env->GetMethodID(m_shared_preferences_class, "getStringSet", "(Ljava/lang/String;Ljava/util/Set;)Ljava/util/Set;"); + m_set_to_array = env->GetMethodID(m_set_class, "toArray", "()[Ljava/lang/Object;"); Assert(m_get_boolean && m_get_int && m_get_float && m_get_string && m_get_string_set && m_set_to_array); } AndroidSettingsInterface::~AndroidSettingsInterface() { + JNIEnv* env = AndroidHelpers::GetJNIEnv(); if (m_java_shared_preferences) - AndroidHelpers::GetJNIEnv()->DeleteGlobalRef(m_java_shared_preferences); + env->DeleteGlobalRef(m_java_shared_preferences); + if (m_shared_preferences_class) + env->DeleteGlobalRef(m_shared_preferences_class); + if (m_set_class) + env->DeleteGlobalRef(m_set_class); } void AndroidSettingsInterface::Clear() @@ -52,25 +67,28 @@ void AndroidSettingsInterface::Clear() int AndroidSettingsInterface::GetIntValue(const char* section, const char* key, int default_value /*= 0*/) { - JNIEnv* env = AndroidHelpers::GetJNIEnv(); - // Some of these settings are string lists... - jstring string_object = reinterpret_cast( - env->CallObjectMethod(m_java_shared_preferences, m_get_string, env->NewStringUTF(GetSettingKey(section, key)), - env->NewStringUTF(TinyString::FromFormat("%d", default_value)))); + JNIEnv* env = AndroidHelpers::GetJNIEnv(); + LocalRefHolder key_string(env, env->NewStringUTF(GetSettingKey(section, key))); + LocalRefHolder default_value_string(env, env->NewStringUTF(TinyString::FromFormat("%d", default_value))); + LocalRefHolder string_object( + env, reinterpret_cast(env->CallObjectMethod(m_java_shared_preferences, m_get_string, key_string.Get(), + default_value_string.Get()))); if (env->ExceptionCheck()) { env->ExceptionClear(); // it might actually be an int (e.g. seek bar preference) - const int int_value = static_cast(env->CallIntMethod(m_java_shared_preferences, m_get_int, - env->NewStringUTF(GetSettingKey(section, key)), default_value)); + const int int_value = + static_cast(env->CallIntMethod(m_java_shared_preferences, m_get_int, key_string.Get(), default_value)); if (env->ExceptionCheck()) { env->ExceptionClear(); + Log_DevPrintf("GetIntValue(%s, %s) -> %d (exception)", section, key, default_value); return default_value; } + Log_DevPrintf("GetIntValue(%s, %s) -> %d (int)", section, key, int_value); return int_value; } @@ -79,6 +97,7 @@ int AndroidSettingsInterface::GetIntValue(const char* section, const char* key, const char* data = env->GetStringUTFChars(string_object, nullptr); Assert(data != nullptr); + Log_DevPrintf("GetIntValue(%s, %s) -> %s", section, key, data); std::optional value = StringUtil::FromChars(data); env->ReleaseStringUTFChars(string_object, data); @@ -88,44 +107,47 @@ int AndroidSettingsInterface::GetIntValue(const char* section, const char* key, float AndroidSettingsInterface::GetFloatValue(const char* section, const char* key, float default_value /*= 0.0f*/) { JNIEnv* env = AndroidHelpers::GetJNIEnv(); -#if 0 - return static_cast(env->CallFloatMethod(m_java_shared_preferences, m_get_float, - env->NewStringUTF(GetSettingKey(section, key)), default_value)); -#else - // Some of these settings are string lists... - jstring string_object = reinterpret_cast( - env->CallObjectMethod(m_java_shared_preferences, m_get_string, env->NewStringUTF(GetSettingKey(section, key)), - env->NewStringUTF(TinyString::FromFormat("%f", default_value)))); - + LocalRefHolder key_string(env, env->NewStringUTF(GetSettingKey(section, key))); + LocalRefHolder default_value_string(env, env->NewStringUTF(TinyString::FromFormat("%f", default_value))); + LocalRefHolder string_object( + env, reinterpret_cast(env->CallObjectMethod(m_java_shared_preferences, m_get_string, key_string.Get(), + default_value_string.Get()))); if (env->ExceptionCheck()) { env->ExceptionClear(); + Log_DevPrintf("GetFloatValue(%s, %s) -> %f (exception)", section, key, default_value); return default_value; } if (!string_object) + { + Log_DevPrintf("GetFloatValue(%s, %s) -> %f (null)", section, key, default_value); return default_value; + } const char* data = env->GetStringUTFChars(string_object, nullptr); Assert(data != nullptr); + Log_DevPrintf("GetFloatValue(%s, %s) -> %s", section, key, data); std::optional value = StringUtil::FromChars(data); env->ReleaseStringUTFChars(string_object, data); return value.value_or(default_value); -#endif } bool AndroidSettingsInterface::GetBoolValue(const char* section, const char* key, bool default_value /*= false*/) { JNIEnv* env = AndroidHelpers::GetJNIEnv(); - jboolean bool_value = static_cast(env->CallBooleanMethod(m_java_shared_preferences, m_get_boolean, - env->NewStringUTF(GetSettingKey(section, key)), default_value)); + LocalRefHolder key_string(env, env->NewStringUTF(GetSettingKey(section, key))); + jboolean bool_value = static_cast( + env->CallBooleanMethod(m_java_shared_preferences, m_get_boolean, key_string.Get(), default_value)); if (env->ExceptionCheck()) { + Log_DevPrintf("GetBoolValue(%s, %s) -> %u (exception)", section, key, static_cast(default_value)); env->ExceptionClear(); return default_value; } + Log_DevPrintf("GetBoolValue(%s, %s) -> %u", section, key, static_cast(bool_value)); return bool_value; } @@ -133,20 +155,28 @@ std::string AndroidSettingsInterface::GetStringValue(const char* section, const const char* default_value /*= ""*/) { JNIEnv* env = AndroidHelpers::GetJNIEnv(); - jobject string_object = - env->CallObjectMethod(m_java_shared_preferences, m_get_string, env->NewStringUTF(GetSettingKey(section, key)), - env->NewStringUTF(default_value)); + LocalRefHolder key_string(env, env->NewStringUTF(GetSettingKey(section, key))); + LocalRefHolder default_value_string(env, env->NewStringUTF(default_value)); + LocalRefHolder string_object( + env, reinterpret_cast(env->CallObjectMethod(m_java_shared_preferences, m_get_string, key_string.Get(), + default_value_string.Get()))); if (env->ExceptionCheck()) { env->ExceptionClear(); + Log_DevPrintf("GetStringValue(%s, %s) -> %s (exception)", section, key, default_value); return default_value; } if (!string_object) + { + Log_DevPrintf("GetStringValue(%s, %s) -> %s (null)", section, key, default_value); return default_value; + } - return AndroidHelpers::JStringToString(env, reinterpret_cast(string_object)); + const std::string ret(AndroidHelpers::JStringToString(env, string_object)); + Log_DevPrintf("GetStringValue(%s, %s) -> %s", section, key, ret.c_str()); + return ret; } void AndroidSettingsInterface::SetIntValue(const char* section, const char* key, int value) @@ -177,8 +207,9 @@ void AndroidSettingsInterface::DeleteValue(const char* section, const char* key) std::vector AndroidSettingsInterface::GetStringList(const char* section, const char* key) { JNIEnv* env = AndroidHelpers::GetJNIEnv(); - jobject values_set = env->CallObjectMethod(m_java_shared_preferences, m_get_string_set, - env->NewStringUTF(GetSettingKey(section, key)), nullptr); + LocalRefHolder key_string(env, env->NewStringUTF(GetSettingKey(section, key))); + LocalRefHolder values_set( + env, env->CallObjectMethod(m_java_shared_preferences, m_get_string_set, key_string.Get(), nullptr)); if (env->ExceptionCheck()) { env->ExceptionClear(); @@ -188,13 +219,14 @@ std::vector AndroidSettingsInterface::GetStringList(const char* sec if (!values_set) return {}; - jobjectArray values_array = reinterpret_cast(env->CallObjectMethod(values_set, m_set_to_array)); + LocalRefHolder values_array( + env, reinterpret_cast(env->CallObjectMethod(values_set, m_set_to_array))); if (env->ExceptionCheck()) { env->ExceptionClear(); return {}; } - + if (!values_array) return {}; @@ -202,8 +234,11 @@ std::vector AndroidSettingsInterface::GetStringList(const char* sec std::vector values; values.reserve(size); for (jsize i = 0; i < size; i++) - values.push_back( - AndroidHelpers::JStringToString(env, reinterpret_cast(env->GetObjectArrayElement(values_array, i)))); + { + jstring str = reinterpret_cast(env->GetObjectArrayElement(values_array, i)); + values.push_back(AndroidHelpers::JStringToString(env, str)); + env->DeleteLocalRef(str); + } return values; } diff --git a/android/app/src/cpp/android_settings_interface.h b/android/app/src/cpp/android_settings_interface.h index 1da353590..ca42ed569 100644 --- a/android/app/src/cpp/android_settings_interface.h +++ b/android/app/src/cpp/android_settings_interface.h @@ -27,6 +27,8 @@ public: bool AddToStringList(const char* section, const char* key, const char* item) override; private: + jclass m_set_class{}; + jclass m_shared_preferences_class{}; jobject m_java_shared_preferences{}; jmethodID m_get_boolean{}; jmethodID m_get_int{};