fix: Reset Kotlin C++ state in dispose() (#1326)

* fix: Reset C++ state in `dispose()`

* fix: Must call super

* fix: Run memory leak 51200 times to test JNI as well

* test 55k separately

Author: mrousavy

example/src/getTests.ts

@@ -1532,54 +1532,76 @@ export function getTests(
         .didNotThrow()
         .equals(10_000)
     ),
-    createTest('HybridObjects dont leak memory', () =>
-      it(() => {
-        const baselineAllocations =
-          NitroModules.debug_getTotalAllocatedHybridObjects()
+    createTest(
+      'HybridObjects dont leak memory with automatic YoungGen GC',
+      () =>
+        it(() => {
+          const baselineAllocations =
+            NitroModules.debug_getTotalAllocatedHybridObjects()
+          const TOTAL_ALLOCATIONS = 10_000
+          const BATCH_SIZE = 1000
+
+          const objects: Array<TestObjectCpp | TestObjectSwiftKotlin> = []
+          for (let i = 0; i < TOTAL_ALLOCATIONS; i++) {
+            const object = testObject.newTestObject()
+            object.numberValue = i
+            objects.push(object)
+
+            if (objects.length >= BATCH_SIZE) {
+              objects.length = 0
+              gc()
+            }
+          }
+
+          objects.length = 0
+          gc()
+          gc()
+          gc()
 
+          const currentAllocations =
+            NitroModules.debug_getTotalAllocatedHybridObjects()
+          const remainingAllocations = currentAllocations - baselineAllocations
+          // make sure that less than 10% of the total allocations are remaining, indicating GC ran for most of it.
+          const didDeleteMostObjects =
+            remainingAllocations < TOTAL_ALLOCATIONS * 0.1
+          const result: {
+            baselineAllocations: number
+            currentAllocations: number
+            isEqual?: boolean
+          } = {
+            baselineAllocations: baselineAllocations,
+            currentAllocations: currentAllocations,
+            isEqual: didDeleteMostObjects,
+          }
+          if (!didDeleteMostObjects) {
+            delete result.isEqual
+          }
+          return result
+        })
+          .didNotThrow()
+          .toContain('isEqual')
+    ),
+    createTest('HybridObjects dont leak memory with manual dispose()', () =>
+      it(() => {
+        // We test 55_000 allocations, because JNI's max global_ref count
+        // is 51200, so if this test goes green, it properly deletes
+        // jni::global_refs. If there would be a memory leak, this test
+        // will likely crash in C++.
+        const TOTAL_ALLOCATIONS = 55_000
         const BATCH_SIZE = 1000
-        const LOOP_COUNT = 10
-        const TOTAL_ALLOCATIONS = BATCH_SIZE * LOOP_COUNT
 
-        const objects: Array<TestObjectCpp | TestObjectSwiftKotlin> = []
         for (let i = 0; i < TOTAL_ALLOCATIONS; i++) {
           const object = testObject.newTestObject()
-          object.numberValue = i
-          objects.push(object)
+          object.dispose()
 
-          if (objects.length >= BATCH_SIZE) {
-            objects.length = 0
+          if ((i + 1) % BATCH_SIZE === 0) {
             gc()
           }
         }
 
-        objects.length = 0
         gc()
-        gc()
-        gc()
-
-        const currentAllocations =
-          NitroModules.debug_getTotalAllocatedHybridObjects()
-        const remainingAllocations = currentAllocations - baselineAllocations
-        // make sure that less than 10% of the total allocations are remaining, indicating GC ran for most of it.
-        const didDeleteMostObjects =
-          remainingAllocations < TOTAL_ALLOCATIONS * 0.1
-        const result: {
-          baselineAllocations: number
-          currentAllocations: number
-          isEqual?: boolean
-        } = {
-          baselineAllocations: baselineAllocations,
-          currentAllocations: currentAllocations,
-          isEqual: didDeleteMostObjects,
-        }
-        if (!didDeleteMostObjects) {
-          delete result.isEqual
-        }
-        return result
-      })
-        .didNotThrow()
-        .toContain('isEqual')
+        return TOTAL_ALLOCATIONS
+      }).didNotThrow()
     ),
     createTest('callWithOptional(undefined)', async () =>
       (

packages/react-native-nitro-modules/android/src/main/java/com/margelo/nitro/core/HybridObject.kt

@@ -1,5 +1,6 @@
 package com.margelo.nitro.core
 
+import androidx.annotation.CallSuper
 import androidx.annotation.Keep
 import com.facebook.jni.HybridData
 import com.facebook.proguard.annotations.DoNotStrip
@@ -39,13 +40,17 @@ abstract class HybridObject {
    * If this method is never manually called, a `HybridObject` is expected to disposes its
    * resources as usual via the object's destructor (`~HybridObject()`, `deinit` or `finalize()`).
    *
-   * By default, this method does nothing. It can be overridden to perform actual disposing/cleanup
-   * if required.
+   * By default, this method only destroys the connected C++ part.
+   * It can be overridden to perform actual disposing/cleanup if required.
    * This method must not throw.
    */
   @DoNotStrip
   @Keep
-  open fun dispose() { }
+  @CallSuper
+  open fun dispose() {
+    cxxPartCache?.get()?.destroy()
+    cxxPartCache = null
+  }
 
   /**
    * Get a string representation of this `HybridObject` - useful for logging or debugging.
@@ -96,6 +101,10 @@ abstract class HybridObject {
       mHybridData = initHybrid()
     }
 
+    fun destroy() {
+      mHybridData.resetNative()
+    }
+
     protected open external fun initHybrid(): HybridData
   }
 }

packages/react-native-nitro-test/android/src/main/java/com/margelo/nitro/test/HybridTestObjectKotlin.kt

@@ -628,6 +628,7 @@ class HybridTestObjectKotlin : HybridTestObjectSwiftKotlinSpec() {
   }
 
   override fun dispose() {
+    super.dispose()
     this.optionalCallback?.let { callback ->
       callback(13.0)
     }