fix(android): release JS errors handed to Java instead of leaking a handle per throw

NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.cpp

@@ -4,11 +4,59 @@
 #include "NativeScriptAssert.h"
 #include "Runtime.h"
 #include "ObjectManager.h"
+#include <deque>
+#include <mutex>
 #include <sstream>
+#include <unordered_map>
 
 using namespace std;
 using namespace tns;
 
+namespace {
+struct TransferredError {
+    jlong id;
+    JsValue value;
+};
+
+constexpr size_t kMaxTransferredPerRuntime = 1024;
+
+std::mutex g_transferredMutex;
+std::unordered_map<const void*, std::deque<TransferredError>> g_transferred;
+jlong g_nextTransferredId = 0;
+}  // namespace
+
+jlong NativeScriptException::TransferToJava(JsRuntime& rt, const JsValue& error) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    auto &entries = g_transferred[rt.identity()];
+    entries.push_back({++g_nextTransferredId, JsValue(rt, error)});
+    while (entries.size() > kMaxTransferredPerRuntime) {
+        entries.pop_front();
+    }
+    return entries.back().id;
+}
+
+bool NativeScriptException::TakeFromJava(JsRuntime& rt, jlong id, JsValue& out) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    auto bucket = g_transferred.find(rt.identity());
+    if (bucket == g_transferred.end()) {
+        return false;
+    }
+    auto &entries = bucket->second;
+    for (auto it = entries.rbegin(); it != entries.rend(); ++it) {
+        if (it->id == id) {
+            out = it->value;
+            entries.erase(std::next(it).base());
+            return true;
+        }
+    }
+    return false;
+}
+
+void NativeScriptException::ReleaseTransferred(JsRuntime& rt) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    g_transferred.erase(rt.identity());
+}
+
 NativeScriptException::NativeScriptException(JEnv& env)
     : m_javascriptException(nullptr) {
     jthrowable  thrw = env.ExceptionOccurred();
@@ -135,12 +183,9 @@ void NativeScriptException::ReThrowToJava(JsRuntime* rt) {
         JniLocalRef stackTrace(jEnv.NewStringUTF(m_stackTrace.c_str()));
 
         if (ex == nullptr) {
-            // The napi tree hands Java the napi_ref itself; here Java gets its own
-            // owned handle, released by WrapJavaToJsException when it comes back.
-            // Ownership is then unambiguous: this exception object keeps its
-            // shared_ptr and Java keeps the copy it was given.
-            auto* javaOwnedValue = new JsValue(*rt, errObj);
-            ex = static_cast<jthrowable>(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, reinterpret_cast<jlong>(javaOwnedValue)));
+            jlong transferId = TransferToJava(*rt, errObj);
+            ex = static_cast<jthrowable>(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, transferId));
+
         } else {
             auto excClassName = ObjectManager::GetClassName(ex);
             if (excClassName != "com/tns/NativeScriptException") {
@@ -214,10 +259,8 @@ JsValue NativeScriptException::WrapJavaToJsException(JsRuntime& rt) {
         jfieldID fieldID = jenv.GetFieldID(jenv.GetObjectClass(m_javaException), "jsValueAddress", "J");
         jlong addr = jenv.GetLongField(m_javaException, fieldID);
 
-        if (addr != 0) {
-            auto pv = reinterpret_cast<JsValue*>(addr);
-            errObj = *pv;
-            delete pv;
+        if (addr != 0 && TakeFromJava(rt, addr, errObj)) {
+            jenv.SetLongField(m_javaException, fieldID, 0);
         } else {
             errObj = GetJavaExceptionFromEnv(rt, m_javaException, jenv);
         }

NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.h

@@ -69,6 +69,18 @@ class NativeScriptException : public std::exception {
          */
         static void CallJsFuncWithErr(JsRuntime& rt, const JsValue& errObj, bool isDiscarded);
 
+        /*
+         * Java cannot free an engine handle, so a JS error it holds travels as an id into a table
+         * that this class owns, bounded per runtime: the oldest entry is released when the table
+         * is full. An entry is also released when the error comes back to JS, and with its runtime.
+         * An error that is no longer in the table when it comes back is rebuilt from its throwable.
+         */
+        static jlong TransferToJava(JsRuntime& rt, const JsValue& error);
+
+        static bool TakeFromJava(JsRuntime& rt, jlong id, JsValue& out);
+
+        static void ReleaseTransferred(JsRuntime& rt);
+
     private:
         /*
          * Try to get native exception or NativeScriptException from js object

NativeScript/ffi/jni/napi/exceptions/NativeScriptException.cpp

@@ -4,11 +4,69 @@
 #include "NativeScriptAssert.h"
 #include "Runtime.h"
 #include "ObjectManager.h"
+#include <deque>
+#include <mutex>
 #include <sstream>
+#include <unordered_map>
 
 using namespace std;
 using namespace tns;
 
+namespace {
+struct TransferredError {
+    jlong id;
+    napi_ref ref;
+};
+
+constexpr size_t kMaxTransferredPerEnv = 1024;
+
+std::mutex g_transferredMutex;
+std::unordered_map<napi_env, std::deque<TransferredError>> g_transferred;
+jlong g_nextTransferredId = 0;
+}  // namespace
+
+jlong NativeScriptException::TransferToJava(napi_env env, napi_ref ref) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    auto &entries = g_transferred[env];
+    entries.push_back({++g_nextTransferredId, ref});
+    while (entries.size() > kMaxTransferredPerEnv) {
+        napi_delete_reference(env, entries.front().ref);
+        entries.pop_front();
+    }
+    return entries.back().id;
+}
+
+napi_value NativeScriptException::TakeFromJava(napi_env env, jlong id) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    auto bucket = g_transferred.find(env);
+    if (bucket == g_transferred.end()) {
+        return nullptr;
+    }
+    auto &entries = bucket->second;
+    for (auto it = entries.rbegin(); it != entries.rend(); ++it) {
+        if (it->id == id) {
+            napi_value value = nullptr;
+            napi_get_reference_value(env, it->ref, &value);
+            napi_delete_reference(env, it->ref);
+            entries.erase(std::next(it).base());
+            return value;
+        }
+    }
+    return nullptr;
+}
+
+void NativeScriptException::ReleaseTransferred(napi_env env) {
+    std::lock_guard<std::mutex> lock(g_transferredMutex);
+    auto bucket = g_transferred.find(env);
+    if (bucket == g_transferred.end()) {
+        return;
+    }
+    for (auto &entry : bucket->second) {
+        napi_delete_reference(env, entry.ref);
+    }
+    g_transferred.erase(bucket);
+}
+
 NativeScriptException::NativeScriptException(JEnv& env)
     : m_javascriptException(nullptr), m_napiEnv(nullptr) {
     jthrowable  thrw = env.ExceptionOccurred();
@@ -141,7 +199,8 @@ void NativeScriptException::ReThrowToJava(napi_env env) {
         JniLocalRef stackTrace(jEnv.NewStringUTF(m_stackTrace.c_str()));
 
         if (ex == nullptr) {
-            ex = static_cast<jthrowable>(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, reinterpret_cast<jlong>(m_javascriptException)));
+            jlong transferId = TransferToJava(env, m_javascriptException);
+            ex = static_cast<jthrowable>(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, transferId));
             transferredJavascriptException = true;
         } else {
             auto excClassName = ObjectManager::GetClassName(ex);
@@ -233,12 +292,11 @@ napi_value NativeScriptException::WrapJavaToJsException(napi_env env) {
         jfieldID fieldID = jenv.GetFieldID(jenv.GetObjectClass(m_javaException), "jsValueAddress", "J");
         jlong addr = jenv.GetLongField(m_javaException, fieldID);
 
+        errObj = addr != 0 ? TakeFromJava(env, addr) : nullptr;
         if (addr != 0) {
-            auto pv = reinterpret_cast<napi_ref>(addr);
-            NAPI_GUARD(napi_get_reference_value(env, pv, &errObj)) {}
-            NAPI_GUARD(napi_delete_reference(env, pv)) {}
             jenv.SetLongField(m_javaException, fieldID, 0);
-        } else {
+        }
+        if (errObj == nullptr) {
             errObj = GetJavaExceptionFromEnv(env, m_javaException, jenv);
         }
     } else {

NativeScript/ffi/jni/napi/exceptions/NativeScriptException.h

@@ -52,6 +52,18 @@ class NativeScriptException {
          */
         static void CallJsFuncWithErr(napi_env env, napi_value errObj, bool isDiscarded);
 
+        /*
+         * Java cannot free a napi_ref, so a JS error it holds travels as an id into a table that
+         * this class owns, bounded per env: the oldest entry is released when the table is full.
+         * An entry is also released when the error comes back to JS, and with its env. An error
+         * that is no longer in the table when it comes back is rebuilt from its throwable.
+         */
+        static jlong TransferToJava(napi_env env, napi_ref ref);
+
+        static napi_value TakeFromJava(napi_env env, jlong id);
+
+        static void ReleaseTransferred(napi_env env);
+
     private:
         /*
          * Try to get native exception or NativeScriptException from js object

NativeScript/runtime/android/jsi/Runtime.cpp

@@ -556,6 +556,7 @@ void Runtime::DestroyRuntime() {
     }
     MetadataNode::onDisposeRuntime(rt);
     ArgConverter::onDisposeRuntime(rt);
+    NativeScriptException::ReleaseTransferred(rt);
     tns::GlobalHelpers::onDisposeRuntime(rt);
     this->js_method_cache->cleanupCache();
     delete this->js_method_cache;

NativeScript/runtime/android/napi/Runtime.cpp

@@ -513,6 +513,7 @@ void Runtime::DestroyRuntime() {
     Runtime::thread_id_to_rt_cache.Remove(this->my_thread_id);
     id_to_runtime_cache.Remove(m_id);
     env_to_runtime_cache.Remove(env);
+    NativeScriptException::ReleaseTransferred(env);
     js_free_napi_env(env);
 
 #ifndef __V8__

platforms/android/test-app/app/src/main/assets/app/marshalling-benchmark.js

@@ -103,6 +103,10 @@ function buildSuite() {
             var cb = new org.nativescript.BenchmarkCallback({ onString: function (value) {} });
             for (var i = 0; i < 1e4; i++) Benchmarks.invokeStringCallback(cb);
         } },
+        { name: "Invoke callback that throws", iterations: 5e4, action: function () {
+            var cb = new org.nativescript.BenchmarkCallback({ onCallback: function () { throw new Error("benchmark"); } });
+            Benchmarks.invokeThrowingCallback(cb, 5e4);
+        } },
     ];
 }
 

platforms/android/test-app/app/src/main/java/org/nativescript/Benchmarks.java

@@ -126,4 +126,16 @@ public static int invokeIntCallback(BenchmarkCallback callback) {
     public static void invokeStringCallback(BenchmarkCallback callback) {
         callback.onString("hello");
     }
+
+    public static int invokeThrowingCallback(BenchmarkCallback callback, int count) {
+        int failures = 0;
+        for (int i = 0; i < count; i++) {
+            try {
+                callback.onCallback();
+            } catch (Throwable t) {
+                failures++;
+            }
+        }
+        return failures;
+    }
 }