From 069e1b2948218f0c807391ed8349c2e474e551e2 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Fri, 17 Jan 2025 21:04:12 +0530 Subject: [PATCH 1/2] Fixed the native crash happening on Android. * The issue on the Android side was a `null pointer dereference`, meaning that when accessing the getEntryByPath method, the archive object had already been disposed of, which caused the crash. * We are correctly handling and disposing of the previous archive and web views for the previously set archive. However, web views load content on their own thread. When we change the ZIM file, we dispose of the previous archive and stop the ongoing web view processes. In certain cases, the web view internally tries to get content via the getEntryByPath method because it takes some time to cancel all the previous calls on the background thread. As a result, the web view attempts to call the method on a disposed archive, which causes the native crash. * To fix this, I refactored the getPtr method to return nullptr if the archive has already been disposed of. I also handled this in the utils.h class by checking if the object is null (meaning it has already been disposed). If so, an error is thrown (which should be handled by the caller, in this case, the Kiwix app), instead of crashing the program. * Handling this scenario on the Android side is not feasible since the WebView operates on a separate thread over which we have no control. However, addressing this issue on the java-libkiwix side is a better approach. If an object is already disposed, it should throw an error that can be caught by the caller, instead of crashing the entire program. Handling such errors is the responsibility of the user of `java-libkiwix` (e.g., Kiwix Android). We are already handling all errors thrown by `java-libkiwix` in the Kiwix Android app. * Currently, I have created the `SAFE_THIS` macro to gather feedback on this approach. Changing the existing `THIS` macro to incorporate the new behavior would require updating all the places where it is being used. To validate this fix before making those changes, I implemented the new function separately. I explicitly disposed of the archive and attempted to access its content via the getEntryByPath method. Now, instead of crashing the entire program, it correctly throws an error, which is handled by Kiwix. --- lib/src/main/cpp/libzim/archive.cpp | 2 +- lib/src/main/cpp/macros.h | 13 +++++++++++++ lib/src/main/cpp/utils.h | 3 +++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/src/main/cpp/libzim/archive.cpp b/lib/src/main/cpp/libzim/archive.cpp index c3b2fbc..381f947 100644 --- a/lib/src/main/cpp/libzim/archive.cpp +++ b/lib/src/main/cpp/libzim/archive.cpp @@ -183,7 +183,7 @@ METHOD(jboolean, hasIllustration, jint size) { GETTER(jlongArray, getIllustrationSizes) METHOD(jobject, getEntryByPath__Ljava_lang_String_2, jstring path) { - return BUILD_WRAPPER("org/kiwix/libzim/Entry", THIS->getEntryByPath(TO_C(path))); + return BUILD_WRAPPER("org/kiwix/libzim/Entry", SAFE_THIS->getEntryByPath(TO_C(path))); } CATCH_EXCEPTION(nullptr) METHOD(jobject, getEntryByPath__I, jint index) { diff --git a/lib/src/main/cpp/macros.h b/lib/src/main/cpp/macros.h index 46201d5..f610898 100644 --- a/lib/src/main/cpp/macros.h +++ b/lib/src/main/cpp/macros.h @@ -27,6 +27,16 @@ #define THIS getPtr(env, thisObj) +#define SAFE_THIS \ + ({ \ + auto ptr = getPtr(env, thisObj); \ + if (!ptr) { \ + throwException(env, "java/lang/IllegalStateException", "The native object has already been disposed."); \ + return nullptr; \ + } \ + ptr; \ + }) + #define METHOD0(retType, name) \ JNIEXPORT retType JNICALL BUILD_METHOD(TYPENAME, name) ( \ JNIEnv* env, jobject thisObj) try @@ -59,4 +69,7 @@ catch(const zim::ZimFileFormatError& e) { \ } catch(const std::exception& e) { \ throwException(env, "java/lang/Exception", e.what()); \ return RET; \ +} catch(...) { \ + throwException(env, "java/lang/Error", "Unknown native exception occurred."); \ + return RET; \ } diff --git a/lib/src/main/cpp/utils.h b/lib/src/main/cpp/utils.h index 9324adf..ea13d54 100644 --- a/lib/src/main/cpp/utils.h +++ b/lib/src/main/cpp/utils.h @@ -104,6 +104,9 @@ shared_ptr getPtr(JNIEnv* env, jobject thisObj, const char* handleName = "nat jclass thisClass = env->GetObjectClass(thisObj); jfieldID fidNumber = env->GetFieldID(thisClass, handleName, "J"); auto handle = reinterpret_cast*>(env->GetLongField(thisObj, fidNumber)); + if (handle == nullptr || *handle == nullptr) { + return nullptr; + } return *handle; } #define GET_PTR(NATIVE_TYPE) getPtr(env, thisObj) From 5fd942d731f16ad8305354259ae42c2122d5cb54 Mon Sep 17 00:00:00 2001 From: MohitMaliFtechiz Date: Mon, 20 Jan 2025 12:29:53 +0530 Subject: [PATCH 2/2] Removed the redundant check to check whether the native object is null or not. --- lib/src/main/cpp/utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/main/cpp/utils.h b/lib/src/main/cpp/utils.h index ea13d54..f1086a7 100644 --- a/lib/src/main/cpp/utils.h +++ b/lib/src/main/cpp/utils.h @@ -104,7 +104,7 @@ shared_ptr getPtr(JNIEnv* env, jobject thisObj, const char* handleName = "nat jclass thisClass = env->GetObjectClass(thisObj); jfieldID fidNumber = env->GetFieldID(thisClass, handleName, "J"); auto handle = reinterpret_cast*>(env->GetLongField(thisObj, fidNumber)); - if (handle == nullptr || *handle == nullptr) { + if (handle == nullptr) { return nullptr; } return *handle;