diff --git a/lib/src/main/cpp/libzim/archive.cpp b/lib/src/main/cpp/libzim/archive.cpp index 065393d..c6a4763 100644 --- a/lib/src/main/cpp/libzim/archive.cpp +++ b/lib/src/main/cpp/libzim/archive.cpp @@ -31,20 +31,19 @@ #include /* Kiwix Reader JNI functions */ -JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchive( - JNIEnv* env, jobject obj, jstring filename) +JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchive( + JNIEnv* env, jobject thisObj, jstring filename) { std::string cPath = TO_C(filename); LOG("Attempting to create reader with: %s", cPath.c_str()); Lock l; try { - zim::Archive* reader = new zim::Archive(cPath); - return reinterpret_cast(new Handle(reader)); + auto archive = std::make_shared(cPath); + SET_PTR(archive); } catch (std::exception& e) { LOG("Error opening ZIM file"); LOG("%s", e.what()); - return 0; } } @@ -68,8 +67,8 @@ int jni2fd(const jobject& fdObj, JNIEnv* env) } // unnamed namespace -JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveByFD( - JNIEnv* env, jobject obj, jobject fdObj) +JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchiveByFD( + JNIEnv* env, jobject thisObj, jobject fdObj) { #ifndef _WIN32 int fd = jni2fd(fdObj, env); @@ -77,22 +76,20 @@ JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveByFD( LOG("Attempting to create reader with fd: %d", fd); Lock l; try { - zim::Archive* reader = new zim::Archive(fd); - return reinterpret_cast(new Handle(reader)); + auto archive = std::make_shared(fd); + SET_PTR(archive); } catch (std::exception& e) { LOG("Error opening ZIM file"); LOG("%s", e.what()); - return 0; } #else jclass exception = env->FindClass("java/lang/UnsupportedOperationException"); - env->ThrowNew(exception, "org.kiwix.libzim.Archive.getNativeArchiveByFD() is not supported under Windows"); - return 0; + env->ThrowNew(exception, "org.kiwix.libzim.Archive.setNativeArchiveByFD() is not supported under Windows"); #endif } -JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveEmbedded( - JNIEnv* env, jobject obj, jobject fdObj, jlong offset, jlong size) +JNIEXPORT void JNICALL Java_org_kiwix_libzim_Archive_setNativeArchiveEmbedded( + JNIEnv* env, jobject thisObj, jobject fdObj, jlong offset, jlong size) { #ifndef _WIN32 int fd = jni2fd(fdObj, env); @@ -100,17 +97,15 @@ JNIEXPORT jlong JNICALL Java_org_kiwix_libzim_Archive_getNativeArchiveEmbedded( LOG("Attempting to create reader with fd: %d", fd); Lock l; try { - zim::Archive* reader = new zim::Archive(fd, offset, size); - return reinterpret_cast(new Handle(reader)); + auto archive = std::make_shared(fd, offset, size); + SET_PTR(archive); } catch (std::exception& e) { LOG("Error opening ZIM file"); LOG("%s", e.what()); - return 0; } #else jclass exception = env->FindClass("java/lang/UnsupportedOperationException"); - env->ThrowNew(exception, "org.kiwix.libzim.Archive.getNativeArchiveEmbedded() is not supported under Windows"); - return 0; + env->ThrowNew(exception, "org.kiwix.libzim.Archive.setNativeArchiveEmbedded() is not supported under Windows"); #endif } @@ -120,7 +115,7 @@ Java_org_kiwix_libzim_Archive_dispose(JNIEnv* env, jobject thisObj) dispose(env, thisObj); } -#define THIS (Handle::getHandle(env, thisObj)) +#define THIS GET_PTR(zim::Archive) #define GETTER(retType, name) JNIEXPORT retType JNICALL \ Java_org_kiwix_libzim_Archive_##name (JNIEnv* env, jobject thisObj) \ { \ @@ -144,16 +139,16 @@ METHOD(jstring, Archive, getMetadata, jstring name) { } METHOD(jobject, Archive, getMetadataItem, jstring name) { - auto item = THIS->getMetadataItem(TO_C(name)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item); + auto obj = NEW_OBJECT("org/kiwix/libzim/Item"); + SET_HANDLE(zim::Item, obj, THIS->getMetadataItem(TO_C(name))); return obj; } GETTER(jobjectArray, getMetadataKeys) METHOD(jobject, Archive, getIllustrationItem, jint size) { - auto item = THIS->getIllustrationItem(TO_C(size)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item); + auto obj = NEW_OBJECT("org/kiwix/libzim/Item"); + SET_HANDLE(zim::Item, obj, THIS->getIllustrationItem(TO_C(size))); return obj; } @@ -164,44 +159,44 @@ METHOD(jboolean, Archive, hasIllustration, jint size) { GETTER(jlongArray, getIllustrationSizes) METHOD(jobject, Archive, getEntryByPath, jlong index) { - auto entry = THIS->getEntryByPath(TO_C(index)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getEntryByPath(TO_C(index))); return obj; } METHOD(jobject, Archive, getEntryByPath, jstring path) { - auto entry = THIS->getEntryByPath(TO_C(path)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getEntryByPath(TO_C(path))); return obj; } METHOD(jobject, Archive, getEntryByTitle, jlong index) { - auto entry = THIS->getEntryByTitle(TO_C(index)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getEntryByTitle(TO_C(index))); return obj; } METHOD(jobject, Archive, getEntryByTitle, jstring title) { - auto entry = THIS->getEntryByTitle(TO_C(title)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getEntryByTitle(TO_C(title))); return obj; } METHOD(jobject, Archive, getEntryByClusterOrder, jlong index) { - auto entry = THIS->getEntryByClusterOrder(TO_C(index)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getEntryByClusterOrder(TO_C(index))); return obj; } METHOD0(jobject, Archive, getMainEntry) { - auto entry = THIS->getMainEntry(); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getMainEntry()); return obj; } METHOD0(jobject, Archive, getRandomEntry) { - auto entry = THIS->getRandomEntry(); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getRandomEntry()); return obj; } diff --git a/lib/src/main/cpp/libzim/blob.cpp b/lib/src/main/cpp/libzim/blob.cpp index 27f5ee6..39bed81 100644 --- a/lib/src/main/cpp/libzim/blob.cpp +++ b/lib/src/main/cpp/libzim/blob.cpp @@ -28,13 +28,14 @@ #include +#define NATIVE_TYPE zim::Blob + JNIEXPORT void JNICALL Java_org_kiwix_kiwixlib_libzim_Blob_dispose(JNIEnv* env, jobject thisObj) { - dispose(env, thisObj); + dispose(env, thisObj); } - -#define THIS (Handle::getHandle(env, thisObj)) +#define THIS GET_PTR(NATIVE_TYPE) #define GETTER(retType, name) JNIEXPORT retType JNICALL \ Java_org_kiwix_libzim_Blob__##name (JNIEnv* env, jobject thisObj) \ { \ @@ -42,6 +43,6 @@ Java_org_kiwix_libzim_Blob__##name (JNIEnv* env, jobject thisObj) \ } METHOD0(jstring, Blob, getData) { - return TO_JNI(std::string(**THIS)); + return TO_JNI(std::string(*THIS)); } GETTER(jlong, size) diff --git a/lib/src/main/cpp/libzim/entry.cpp b/lib/src/main/cpp/libzim/entry.cpp index d71f0d5..6e5ce73 100644 --- a/lib/src/main/cpp/libzim/entry.cpp +++ b/lib/src/main/cpp/libzim/entry.cpp @@ -34,10 +34,10 @@ JNIEXPORT void JNICALL Java_org_kiwix_kiwixlib_libzim_Entry_dispose(JNIEnv* env, jobject thisObj) { - dispose(env, thisObj); + dispose(env, thisObj); } -#define THIS (Handle::getHandle(env, thisObj)) +#define THIS GET_PTR(NATIVE_TYPE) #define GETTER(retType, name) JNIEXPORT retType JNICALL \ Java_org_kiwix_libzim_Entry__##name (JNIEnv* env, jobject thisObj) \ { \ @@ -49,19 +49,19 @@ GETTER(jboolean, isRedirect) GETTER(jstring, getTitle) GETTER(jstring, getPath) METHOD(jobject, Entry, getItem, jboolean follow) { - auto item = THIS->getItem(TO_C(follow)); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item); + auto obj = NEW_OBJECT("org/kiwix/libzim/Item"); + SET_HANDLE(zim::Item, obj, THIS->getItem(TO_C(follow))); return obj; } METHOD0(jobject, Entry, getRedirect) { - auto item = THIS->getRedirect(); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Item", item); + auto obj = NEW_OBJECT("org/kiwix/libzim/Item"); + SET_HANDLE(zim::Item, obj, THIS->getRedirect()); return obj; } METHOD0(jobject, Entry, getRedirectEntry) { - auto entry = THIS->getRedirectEntry(); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Entry", entry); + auto obj = NEW_OBJECT("org/kiwix/libzim/Entry"); + SET_HANDLE(zim::Entry, obj, THIS->getRedirectEntry()); return obj; } diff --git a/lib/src/main/cpp/libzim/item.cpp b/lib/src/main/cpp/libzim/item.cpp index 1168a2d..81d7aaf 100644 --- a/lib/src/main/cpp/libzim/item.cpp +++ b/lib/src/main/cpp/libzim/item.cpp @@ -28,13 +28,15 @@ #include +#define NATIVE_TYPE zim::Item + JNIEXPORT void JNICALL Java_org_kiwix_kiwixlib_libzim_Item_dispose(JNIEnv* env, jobject thisObj) { - dispose(env, thisObj); + dispose(env, thisObj); } -#define THIS (Handle::getHandle(env, thisObj)) +#define THIS GET_PTR(NATIVE_TYPE) #define GETTER(retType, name) JNIEXPORT retType JNICALL \ Java_org_kiwix_libzim_Item__##name (JNIEnv* env, jobject thisObj) \ { \ @@ -46,8 +48,8 @@ GETTER(jstring, getPath) GETTER(jstring, getMimetype) METHOD0(jobject, Item, getData) { - auto blob = THIS->getData(); - auto obj = CREATE_WRAPPER("org/kiwix/libzim/Blob", blob); + auto obj = NEW_OBJECT("org/kiwix/libzim/Blob"); + SET_HANDLE(zim::Blob, obj, THIS->getData()); return obj; } diff --git a/lib/src/main/cpp/utils.h b/lib/src/main/cpp/utils.h index e5475e8..7557637 100644 --- a/lib/src/main/cpp/utils.h +++ b/lib/src/main/cpp/utils.h @@ -38,34 +38,74 @@ #endif extern std::mutex globalLock; +using std::shared_ptr; +// Here is the wrapping structure. +// All java class wrapping a `Native` instance must contain a pointer (cast as a long (J)) called "nativeHandle". +// They also need a constructor taking no argument (or they can, but helper here cannot be used.) +// The "nativeHandle" pointer points to a heap allocated `Handle`. The handle itself IS a shared_ptr. +// !!! This is a shared_ptr heap allocated instance which contains a `Native` heap allocated. +// So if the `Native` instance is own by a smart pointer (shared_ptr), the shared_ptr itself is "own" by a raw pointer. + +// From the jobject and the pointer field ("nativeHandle"), we can access a shared_ptr (not a pointer to a shared_ptr). +// To avoid a memory leak, we must at java wrapper destruction (dispose), delete the heap shared_ptr. + +// Create a java object using a default constructor. No field is set. +inline jobject newObject(const char* className, JNIEnv* env) { + jclass objClass = env->FindClass(className); + jmethodID initMethod = env->GetMethodID(objClass, "", "()V"); + jobject wrapper = env->NewObject(objClass, initMethod); + return wrapper; +} +#define NEW_OBJECT(CLASSNAME) newObject(CLASSNAME, env) + + +// Set the pointer to the wrapped object. template -void setPtr(JNIEnv* env, jobject thisObj, T* ptr) +inline void setPtr(JNIEnv* env, jobject thisObj, shared_ptr&& ptr, const char* handleName = "nativeHandle") { jclass thisClass = env->GetObjectClass(thisObj); - jfieldID fieldId = env->GetFieldID(thisClass, "nativeHandle", "J"); - env->SetLongField(thisObj, fieldId, reinterpret_cast(ptr)); + jfieldID fieldId = env->GetFieldID(thisClass, handleName, "J"); + // if field id is not null, we are leaking the currently stored handle + assert(env->GetLongField(thisObj, fieldId) == 0); + // allocate a shared_ptr on the head + shared_ptr* handle = new shared_ptr(ptr); + env->SetLongField(thisObj, fieldId, reinterpret_cast(handle)); } +#define SET_PTR(SHARED_PTR) setPtr(env, thisObj, std::move(SHARED_PTR)) +// This create a object and set it in the wrapper template -void allocate(JNIEnv* env, jobject thisObj, Args && ...args) +inline void setHandle(JNIEnv* env, jobject thisObj, Args && ...args) { - T* ptr = new T(std::forward(args)...); - setPtr(env, thisObj, ptr); + auto ptr = std::make_shared(std::forward(args)...); + setPtr(env, thisObj, std::move(ptr)); } +#define SET_HANDLE(NATIVE_TYPE, OBJ, VALUE) setHandle(env, OBJ, VALUE) + +// Return a shared_ptr for the handle template -T* getPtr(JNIEnv* env, jobject thisObj) +shared_ptr getPtr(JNIEnv* env, jobject thisObj, const char* handleName = "nativeHandle") { jclass thisClass = env->GetObjectClass(thisObj); - jfieldID fidNumber = env->GetFieldID(thisClass, "nativeHandle", "J"); - return reinterpret_cast(env->GetLongField(thisObj, fidNumber)); + jfieldID fidNumber = env->GetFieldID(thisClass, handleName, "J"); + auto handle = reinterpret_cast*>(env->GetLongField(thisObj, fidNumber)); + return *handle; } +#define GET_PTR(NATIVE_TYPE) getPtr(env, thisObj) +// Delete the heap allocated handle template -void dispose(JNIEnv* env, jobject thisObj) +void dispose(JNIEnv* env, jobject thisObj, const char* handleName = "nativeHandle") { - delete getPtr(env, thisObj); + jclass thisClass = env->GetObjectClass(thisObj); + jfieldID fieldId = env->GetFieldID(thisClass, handleName, "J"); + auto handle = reinterpret_cast*>(env->GetLongField(thisObj, fieldId)); + if (handle != 0) { + delete handle; + } + env->SetLongField(thisObj, fieldId, 0); } #define METHOD0(retType, class, name) \ @@ -76,11 +116,11 @@ JNIEXPORT retType JNICALL Java_org_kiwix_libzim_##class##_##name( \ JNIEXPORT retType JNICALL Java_org_kiwix_libzim_##class##_##name( \ JNIEnv* env, jobject thisObj, __VA_ARGS__) -inline jfieldID getHandleField(JNIEnv* env, jobject obj) +inline jfieldID getHandleField(JNIEnv* env, jobject obj, const char* handleName) { jclass c = env->GetObjectClass(obj); // J is the type signature for long: - return env->GetFieldID(c, "nativeHandle", "J"); + return env->GetFieldID(c, handleName, "J"); } inline jobjectArray createArray(JNIEnv* env, size_t length, const std::string& type_sig) @@ -98,55 +138,6 @@ class Lock : public std::unique_lock Lock() : std::unique_lock(globalLock) { } }; -template -class LockedHandle; - - -/* - * A handle to a shared_ptr - * Not usable by itself, you must get a LockedHandle to access the value. - */ -template -class Handle -{ - protected: - std::shared_ptr value; - - public: - Handle(T* v) : value(v){}; - - // No destructor. This must and will be handled by dispose method. - - static LockedHandle getHandle(JNIEnv* env, jobject obj) - { - jlong handle = env->GetLongField(obj, getHandleField(env, obj)); - return LockedHandle(reinterpret_cast*>(handle)); - } - - static void dispose(JNIEnv* env, jobject obj) - { - auto lHandle = getHandle(env, obj); - auto handle = lHandle.h; - delete handle; - } - friend class LockedHandle; -}; - -/* - * A locked handle. - * Only one LockedHandle can exist at the same time. - * As LockedHandle is the - */ -template -struct LockedHandle : public Lock { - Handle* h; - LockedHandle(Handle* h) : h(h) {} - T* operator->() { return h->value.get(); } - T* operator*() { return h->value.get(); } - operator bool() const { return (h->value); } - operator T*() const { return h->value.get(); } -}; - // --------------------------------------------------------------------------- // Convert things to JAVA // --------------------------------------------------------------------------- @@ -160,6 +151,8 @@ template<> struct JType{ typedef jlong type_t; }; template<> struct JType { typedef jlong type_t; }; template<> struct JType { typedef jlong type_t; }; template<> struct JType{ typedef jstring type_t; }; +template<> struct JType { typedef jfloat type_t;}; +template<> struct JType { typedef jdouble type_t;}; template @@ -282,6 +275,8 @@ template<> struct CType{ typedef bool type_t; }; template<> struct CType{ typedef int type_t; }; template<> struct CType{ typedef long type_t; }; template<> struct CType{ typedef std::string type_t; }; +template<> struct CType { typedef float type_t; }; +template<> struct CType { typedef double type_t; }; template struct CType{ typedef std::vector::type_t> type_t; }; @@ -362,15 +357,4 @@ inline void setDaiObjValue(const std::string& filename, const long offset, env->SetLongField(obj, offsetFid, offset); } -template -inline jobject createWrapper(const char* className, T && nativeObj, JNIEnv* env) { - jclass objClass = env->FindClass(className); - jmethodID initMethod = env->GetMethodID(objClass, "", "(J)V"); - jlong nativeHandle = reinterpret_cast(new Handle(new T(std::forward(nativeObj)))); - jobject object = env->NewObject(objClass, initMethod, nativeHandle); - return object; -} - -#define CREATE_WRAPPER(CLASSNAME, OBJ) createWrapper(CLASSNAME, std::move(OBJ), env) - #endif // _ANDROID_JNI_UTILS_H diff --git a/lib/src/main/java/org/kiwix/libzim/Archive.java b/lib/src/main/java/org/kiwix/libzim/Archive.java index b6d8104..a663c65 100644 --- a/lib/src/main/java/org/kiwix/libzim/Archive.java +++ b/lib/src/main/java/org/kiwix/libzim/Archive.java @@ -29,7 +29,7 @@ public class Archive public Archive(String filename) throws ZimFileFormatException { - nativeHandle = getNativeArchive(filename); + setNativeArchive(filename); if (nativeHandle == 0) { throw new ZimFileFormatException("Cannot open zimfile "+filename); } @@ -37,7 +37,7 @@ public class Archive public Archive(FileDescriptor fd) throws ZimFileFormatException { - nativeHandle = getNativeArchiveByFD(fd); + setNativeArchiveByFD(fd); if (nativeHandle == 0) { throw new ZimFileFormatException("Cannot open zimfile by fd "+fd.toString()); } @@ -46,7 +46,7 @@ public class Archive public Archive(FileDescriptor fd, long offset, long size) throws ZimFileFormatException { - nativeHandle = getNativeArchiveEmbedded(fd, offset, size); + setNativeArchiveEmbedded(fd, offset, size); if (nativeHandle == 0) { throw new ZimFileFormatException(String.format("Cannot open embedded zimfile (fd=%s, offset=%d, size=%d)", fd, offset, size)); } @@ -93,9 +93,9 @@ public class Archive public native boolean hasNewNamespaceScheme(); - private native long getNativeArchive(String filename); - private native long getNativeArchiveByFD(FileDescriptor fd); - private native long getNativeArchiveEmbedded(FileDescriptor fd, long offset, long size); + private native void setNativeArchive(String filename); + private native void setNativeArchiveByFD(FileDescriptor fd); + private native void setNativeArchiveEmbedded(FileDescriptor fd, long offset, long size); ///--------- The wrapper thing diff --git a/lib/src/main/java/org/kiwix/libzim/Blob.java b/lib/src/main/java/org/kiwix/libzim/Blob.java index c6ccf6c..fc8390a 100644 --- a/lib/src/main/java/org/kiwix/libzim/Blob.java +++ b/lib/src/main/java/org/kiwix/libzim/Blob.java @@ -23,8 +23,6 @@ import org.kiwix.libzim.Blob; public class Blob { - private Blob(long handle) {nativeHandle = handle;} - public native String getData(); public native long size(); diff --git a/lib/src/main/java/org/kiwix/libzim/Entry.java b/lib/src/main/java/org/kiwix/libzim/Entry.java index 713691b..67b47f6 100644 --- a/lib/src/main/java/org/kiwix/libzim/Entry.java +++ b/lib/src/main/java/org/kiwix/libzim/Entry.java @@ -23,8 +23,6 @@ import org.kiwix.libzim.Item; public class Entry { - private Entry(long handle) {nativeHandle = handle;} - public native boolean isRedirect(); public native String getTitle(); public native String getPath(); diff --git a/lib/src/main/java/org/kiwix/libzim/Item.java b/lib/src/main/java/org/kiwix/libzim/Item.java index e96b2a7..1c198fc 100644 --- a/lib/src/main/java/org/kiwix/libzim/Item.java +++ b/lib/src/main/java/org/kiwix/libzim/Item.java @@ -23,8 +23,6 @@ import org.kiwix.libzim.Blob; public class Item { - private Item(long handle) {nativeHandle = handle;} - public native String getTitle(); public native String getPath(); public native String getMimeType();