Java: remove leak of JNI weak references in JniWrapperCache

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Gluecodium project Release Notes
 
+## Unreleased
+### Bug fixes:
+ * JNI: removed leak of JNI weak references in JniWrapperCache.
+
 ## 13.10.2
 Release date 2025-01-22
 ### Bug fixes:

cmake/modules/gluecodium/gluecodium/GetTargetCompileDefinitions.cmake

@@ -68,9 +68,11 @@ function(gluecodium_get_target_compile_definitions _target)
 
     if("COMMON" IN_LIST _source_sets)
       set(_sync_cache_property "$<TARGET_PROPERTY:${_target},GLUECODIUM_SYNCHRONIZE_ACCESS_CLASS_CACHE>")
+      set(_enable_internal_debug_check "$<TARGET_PROPERTY:${_target},GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS>")
       list(APPEND _public $<$<NOT:$<STREQUAL:${_common},${_main}>>:${_common}_SHARED>)
       list(APPEND _private $<$<NOT:$<STREQUAL:${_common},${_main}>>:${_common}_INTERNAL>
-                           $<$<BOOL:${_sync_cache_property}>:GLUECODIUM_SYNCHRONIZE_ACCESS_CLASS_CACHE>)
+                           $<$<BOOL:${_sync_cache_property}>:GLUECODIUM_SYNCHRONIZE_ACCESS_CLASS_CACHE>
+                           $<$<BOOL:${_enable_internal_debug_check}>:GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS>)
     endif()
 
     if(_args_RESULT_PUBLIC)

cmake/modules/gluecodium/gluecodium/KnownOptionalProperties.cmake

@@ -271,6 +271,14 @@ _gluecodium_define_target_property(
     "Such synchronisation might be necessary when Java/JNI generated code is located in several libraries and the libraries are loaded on-demand."
 )
 
+_gluecodium_define_target_property(
+  GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS
+  BRIEF_DOCS "Option to enable internal debug checks."
+  FULL_DOCS
+    "Enables additional debug checks in C++ code. For instance validation of JNI references handling."
+    "This property is initialized by the value of the GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS_DEFAULT variable if it is set when the function gluecodium_add_generate_command is called."
+)
+
 # TODO: Add read-only properties
 
 function(_gluecodium_get_default_value_for_variable result _property)

functional-tests/functional/android/src/test/java/com/here/android/RobolectricApplication.java

@@ -50,6 +50,7 @@ public void onCreate() {
     if (isFirstTime) {
       isFirstTime = false; // Only load libraries once
       loadNativeLibraries();
+      NativeBase.propagateCleanupException = true;
       Runtime.getRuntime()
           .addShutdownHook(
               new Thread() {

functional-tests/functional/android/src/test/java/com/here/android/test/InterfacesTest.java

@@ -28,6 +28,8 @@
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
+import java.util.*;
+
 @RunWith(RobolectricTestRunner.class)
 @Config(sdk = Build.VERSION_CODES.M, application = RobolectricApplication.class)
 public class InterfacesTest {
@@ -36,6 +38,36 @@ public class InterfacesTest {
   private static final String INSTANCE_TWO_STRING = "simpleInterfaceTwo";
   private static final String INSTANCE_OTHER_STRING = "simpleInstanceOther";
 
+  @Test
+  public void stressTestJniWeakReferenceCache() {
+    List<NestedInterfaceOne> nestedInterfaceOne = new ArrayList(1000);
+    for (int i = 0; i < 1000; ++i) {
+      nestedInterfaceOne.add(InterfacesFactory.createNestedInterfaceOne());
+    }
+
+    for (int i = 0; i < 1000; ++i) {
+      SimpleInterfaceOne instanceOne = InterfacesFactory.createSimpleInterfaceOne();
+      SimpleInterfaceOne instanceTwo = InterfacesFactory.createSimpleInterfaceOne();
+      instanceOne.setStringValue(INSTANCE_ONE_STRING);
+      instanceTwo.setStringValue(INSTANCE_TWO_STRING);
+
+      nestedInterfaceOne.get(i).setSameTypeInterfaces(instanceOne, instanceTwo);
+    }
+
+    System.gc();
+
+    for (int j = 0; j < 1000; ++j) {
+      for (int i = 0; i < 1000; ++i) {
+        SimpleInterfaceOne resultOne = nestedInterfaceOne.get(i).getInterfaceOne();
+        SimpleInterfaceOne resultTwo = nestedInterfaceOne.get(i).getInterfaceTwo();
+        assertEquals(INSTANCE_ONE_STRING, resultOne.getStringValue());
+        assertEquals(INSTANCE_TWO_STRING, resultTwo.getStringValue());
+      }
+
+      System.gc();
+    }
+  }
+
   @Test
   public void setSameTypeInterfaces() {
     SimpleInterfaceOne instanceOne = InterfacesFactory.createSimpleInterfaceOne();

functional-tests/functional/build.gradle

@@ -25,7 +25,8 @@ def getCMakeCommonParameters() {
     return ['-Wno-dev',
         '-DGLUECODIUM_GENERATORS_DEFAULT=android;cpp',
         "-DGLUECODIUM_BASE_OUTPUT_DIR_DEFAULT=${rootProject.generatedDir}",
-        "-DGLUECODIUM_VERSION_DEFAULT=${rootProject.useGluecodiumVersion()}"]
+        "-DGLUECODIUM_VERSION_DEFAULT=${rootProject.useGluecodiumVersion()}",
+        '-DGLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS_DEFAULT=ON']
 }
 
 android {

gluecodium/src/main/resources/templates/java/NativeBase.mustache

@@ -76,6 +76,8 @@ public abstract class NativeBase {
   private static final Set<Reference<?>> REFERENCES =
       Collections.newSetFromMap(new ConcurrentHashMap<Reference<?>, Boolean>());
 
+  public static boolean propagateCleanupException = false;
+
   private static final ReferenceQueue<NativeBase> REFERENCE_QUEUE = new ReferenceQueue<>();
   private final long nativeHandle;
 
@@ -126,6 +128,9 @@ public abstract class NativeBase {
         ((DisposableReference) reference).dispose();
       } catch (Throwable t) {
         LOGGER.log(Level.SEVERE, "Error cleaning up after reference.", t);
+        if (propagateCleanupException) {
+            throw t;
+        }
       }
     }
   }

gluecodium/src/main/resources/templates/jni/utils/JniWrapperCacheImplementation.mustache

@@ -21,6 +21,9 @@
 {{>java/CopyrightHeader}}
 
 #include "JniWrapperCache.h"
+#include "JniThrowNewException.h"
+
+#include <jni.h>
 
 #include <mutex>
 #include <unordered_map>
@@ -32,12 +35,26 @@ namespace {{.}}
 namespace jni
 {
 
+struct CachedObject
+{
+    jobject obj = nullptr;
+    unsigned int scheduled_removals = 0;
+};
+
 static std::mutex s_mutex;
-static std::unordered_map<const void*, jobject> s_wrapper_cache;
+static std::unordered_map<const void*, CachedObject> s_wrapper_cache;
 
 void JniWrapperCache::cache_wrapper_impl(JNIEnv* jenv, const void* obj_ptr, const JniReference<jobject>& jobj) {
     std::lock_guard<std::mutex> lock(s_mutex);
-    s_wrapper_cache[obj_ptr] = jenv->NewWeakGlobalRef(jobj.get());
+
+#ifdef GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS
+    auto iter = s_wrapper_cache.find(obj_ptr);
+    if (iter != s_wrapper_cache.end() && iter->second.obj != nullptr) {
+        throw_new_runtime_exception(jenv, "Weak reference leaked!");
+    }
+#endif
+
+    s_wrapper_cache[obj_ptr].obj = jenv->NewWeakGlobalRef(jobj.get());
 }
 
 JniReference<jobject> JniWrapperCache::get_cached_wrapper_impl(JNIEnv* jenv, const void* obj_ptr) {
@@ -47,9 +64,25 @@ JniReference<jobject> JniWrapperCache::get_cached_wrapper_impl(JNIEnv* jenv, con
     if (iter == s_wrapper_cache.end())
         return {};
 
-    auto jobj = jenv->NewLocalRef(iter->second);
+    if (iter->second.obj == nullptr) {
+        return {};
+    }
+
+    auto jobj = jenv->NewLocalRef(iter->second.obj);
     if (jenv->IsSameObject(jobj, NULL)) {
         jenv->DeleteLocalRef(jobj);
+
+        // Object reference has been cleared by garbage collector.
+        // To avoid leaks we need to delete the weak ref object here.
+        jenv->DeleteWeakGlobalRef(iter->second.obj);
+        iter->second.obj = nullptr;
+
+        // However, because we "remove" the entry from cache here we need to ensure that
+        // the cleanup, which will be scheduled for already cleared object (which has not
+        // been performed yet) does not touch any cache entry added later (an entry for new object
+        // will be added by 'convert_to_jni()').
+        iter->second.scheduled_removals++;
+
         return {};
     } else {
         return make_local_ref(jenv, jobj);
@@ -60,8 +93,27 @@ void JniWrapperCache::remove_cached_wrapper_impl(JNIEnv* jenv, const void* obj_p
     std::lock_guard<std::mutex> lock(s_mutex);
 
     auto iter = s_wrapper_cache.find(obj_ptr);
+
+#ifdef GLUECODIUM_ENABLE_INTERNAL_DEBUG_CHECKS
+    if (iter == s_wrapper_cache.end()) {
+        throw_new_runtime_exception(jenv, "Invalid removal of cache entry");
+    }
+#endif
+
     if (iter != s_wrapper_cache.end()) {
-        jenv->DeleteWeakGlobalRef(iter->second);
+        // Firstly, verify if there were multiple cleanups scheduled for the given entry.
+        // This situation may happen if the exceptional removal of entry happened in
+        // JniWrapperCache::get_cached_wrapper_impl() when the weak reference was cleared by
+        // garbage collector. In such a case does not touch the entry - the last scheduled
+        // cleanup will remove it.
+        if (iter->second.scheduled_removals > 0) {
+            iter->second.scheduled_removals--;
+            return;
+        }
+
+        if (iter->second.obj != nullptr) {
+            jenv->DeleteWeakGlobalRef(iter->second.obj);
+        }
         s_wrapper_cache.erase(iter);
     }
 }

gluecodium/src/test/resources/smoke/namespace_basic/output/android/com/example/foo/bar/NativeBase.java

@@ -56,6 +56,8 @@ public abstract class NativeBase {
   private static final Set<Reference<?>> REFERENCES =
       Collections.newSetFromMap(new ConcurrentHashMap<Reference<?>, Boolean>());
 
+  public static boolean propagateCleanupException = false;
+
   private static final ReferenceQueue<NativeBase> REFERENCE_QUEUE = new ReferenceQueue<>();
   private final long nativeHandle;
 
@@ -106,6 +108,9 @@ private static void cleanUpQueue() {
         ((DisposableReference) reference).dispose();
       } catch (Throwable t) {
         LOGGER.log(Level.SEVERE, "Error cleaning up after reference.", t);
+        if (propagateCleanupException) {
+            throw t;
+        }
       }
     }
   }