From 1d551f9884a346ec966f86f0567815d0902af087 Mon Sep 17 00:00:00 2001 From: Ammar Ahmed Date: Mon, 9 Nov 2026 12:16:44 +0500 Subject: [PATCH 1/2] test(android): add a callback that throws to the marshalling benchmark --- .../app/src/main/assets/app/marshalling-benchmark.js | 4 ++++ .../src/main/java/org/nativescript/Benchmarks.java | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/platforms/android/test-app/app/src/main/assets/app/marshalling-benchmark.js b/platforms/android/test-app/app/src/main/assets/app/marshalling-benchmark.js index cdf840ea..5799c149 100644 --- a/platforms/android/test-app/app/src/main/assets/app/marshalling-benchmark.js +++ b/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); + } }, ]; } diff --git a/platforms/android/test-app/app/src/main/java/org/nativescript/Benchmarks.java b/platforms/android/test-app/app/src/main/java/org/nativescript/Benchmarks.java index 83db4014..d08537fa 100644 --- a/platforms/android/test-app/app/src/main/java/org/nativescript/Benchmarks.java +++ b/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; + } } From b42a5a21114b2bcbb56a93237af1a09d040a61b6 Mon Sep 17 00:00:00 2001 From: Ammar Ahmed Date: Mon, 9 Nov 2026 12:58:09 +0500 Subject: [PATCH 2/2] fix(android): release JS errors handed to Java instead of leaking a handle per throw --- .../jsi/exceptions/NativeScriptException.cpp | 63 ++++++++++++++--- .../jsi/exceptions/NativeScriptException.h | 12 ++++ .../napi/exceptions/NativeScriptException.cpp | 68 +++++++++++++++++-- .../napi/exceptions/NativeScriptException.h | 12 ++++ NativeScript/runtime/android/jsi/Runtime.cpp | 1 + NativeScript/runtime/android/napi/Runtime.cpp | 1 + 6 files changed, 142 insertions(+), 15 deletions(-) diff --git a/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.cpp b/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.cpp index a9b87784..08e592e8 100644 --- a/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.cpp +++ b/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.cpp @@ -4,11 +4,59 @@ #include "NativeScriptAssert.h" #include "Runtime.h" #include "ObjectManager.h" +#include +#include #include +#include 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> g_transferred; +jlong g_nextTransferredId = 0; +} // namespace + +jlong NativeScriptException::TransferToJava(JsRuntime& rt, const JsValue& error) { + std::lock_guard 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 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 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(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, reinterpret_cast(javaOwnedValue))); + jlong transferId = TransferToJava(*rt, errObj); + ex = static_cast(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(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); } diff --git a/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.h b/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.h index 9df16db8..e4a8d7e8 100644 --- a/NativeScript/ffi/jni/jsi/exceptions/NativeScriptException.h +++ b/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 diff --git a/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.cpp b/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.cpp index f445f230..94543c8b 100644 --- a/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.cpp +++ b/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.cpp @@ -4,11 +4,69 @@ #include "NativeScriptAssert.h" #include "Runtime.h" #include "ObjectManager.h" +#include +#include #include +#include 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> g_transferred; +jlong g_nextTransferredId = 0; +} // namespace + +jlong NativeScriptException::TransferToJava(napi_env env, napi_ref ref) { + std::lock_guard 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 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 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(jEnv.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring)msg, (jstring)stackTrace, reinterpret_cast(m_javascriptException))); + jlong transferId = TransferToJava(env, m_javascriptException); + ex = static_cast(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(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 { diff --git a/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.h b/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.h index 4ee567f9..73d890d5 100644 --- a/NativeScript/ffi/jni/napi/exceptions/NativeScriptException.h +++ b/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 diff --git a/NativeScript/runtime/android/jsi/Runtime.cpp b/NativeScript/runtime/android/jsi/Runtime.cpp index 72feeacb..9453db71 100644 --- a/NativeScript/runtime/android/jsi/Runtime.cpp +++ b/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; diff --git a/NativeScript/runtime/android/napi/Runtime.cpp b/NativeScript/runtime/android/napi/Runtime.cpp index 48edffc7..fdfe29cd 100644 --- a/NativeScript/runtime/android/napi/Runtime.cpp +++ b/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__