From 4ac3ce773e56d6c7db2e27de834599f01d33a862 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 14 Aug 2026 19:40:33 -0300 Subject: [PATCH 1/2] fix: release ObjectManager's JS handles and JNI weak refs at teardown ObjectManager had no destructor at all. `delete m_objectManager` ran the implicit one, which destroyed the containers and abandoned everything they pointed at. The expensive part is the JNI weak global refs. m_cache holds one per entry, up to its capacity of 1000, and LRUCache only ever ran its evict callback under capacity pressure or explicit invalidation -- never at destruction, since it had no destructor either. So a worker that touched Java objects abandoned its whole cache when it died. ART's weak-global table is bounded, so this is not merely a leak: enough worker cycles exhaust it and ART aborts. The JS side leaked too. Every linked object owns a Persistent, a JSInstanceInfo and an ObjectWeakCallbackState, freed only from the GC finalizer -- and V8 does not run weak callbacks when an isolate is disposed. In the default `none` marking mode the finalizer additionally re-arms SetWeak while the Java counterpart is alive, so those wrappers are deliberately retained and are therefore all still live at teardown. Split across the two windows teardown actually has: - ReleaseAllRegistered(), called from DestroyRuntime while the isolate is alive and locked: clears each wrapper's JsInfo internal field before freeing the JSInstanceInfo it points at, resets and deletes the Persistent, and releases m_poJsWrapperFunc. - ~ObjectManager, reached from ~Runtime once the isolate is gone and while the thread is still attached to the JVM: clears the LRU cache, which now evicts through the callback. It touches no v8 handle. That ordering is not incidental: Persistent::Reset() after Isolate::Dispose writes into a freed handle table, and the JNI eviction has to happen before ~Runtime drops the com.tns.Runtime global ref, which ObjectManager calls through. m_idToObject now maps to ObjectWeakCallbackState* rather than the bare Persistent*. The state was created and handed to SetWeak but stored nowhere, so teardown had no way to reach it or the JSInstanceInfo. That also lets ReleaseJSInstance free the state, which it never did. --- test-app/runtime/src/main/cpp/LRUCache.h | 17 ++++++ .../runtime/src/main/cpp/ObjectManager.cpp | 52 +++++++++++++++++-- test-app/runtime/src/main/cpp/ObjectManager.h | 17 +++++- test-app/runtime/src/main/cpp/Runtime.cpp | 8 +++ 4 files changed, 90 insertions(+), 4 deletions(-) diff --git a/test-app/runtime/src/main/cpp/LRUCache.h b/test-app/runtime/src/main/cpp/LRUCache.h index d2e4c95d0..624120fee 100644 --- a/test-app/runtime/src/main/cpp/LRUCache.h +++ b/test-app/runtime/src/main/cpp/LRUCache.h @@ -105,6 +105,23 @@ class LRUCache { insert(key, ref); } + + /* + * Evicts every entry, running the evict callback for each. Needed at + * teardown: the callback owns the resource behind each value (JNI weak + * global refs, here), and nothing else releases them -- eviction + * otherwise only happens on capacity pressure or invalidation. + */ + void clear() { + if (m_evictCallback != nullptr) { + for (auto& entry : m_key_to_value) { + m_evictCallback(entry.second.first, m_state); + } + } + m_key_to_value.clear(); + m_key_tracker.clear(); + } + private: void evictKey(const key_type& key) { diff --git a/test-app/runtime/src/main/cpp/ObjectManager.cpp b/test-app/runtime/src/main/cpp/ObjectManager.cpp index 5951d5e51..d4d470910 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.cpp +++ b/test-app/runtime/src/main/cpp/ObjectManager.cpp @@ -220,7 +220,7 @@ Local ObjectManager::GetJsObjectByJavaObject(int javaObjectID) { return handleScope.Escape(Local()); } - Persistent* jsObject = it->second; + Persistent* jsObject = it->second->target; auto localObject = Local::New(isolate, *jsObject); return handleScope.Escape(localObject); @@ -296,7 +296,7 @@ void ObjectManager::Link(const Local& object, uint32_t javaObjectID, // link object->SetInternalField(jsInfoIdx, jsInfo); - m_idToObject.emplace(javaObjectID, objectHandle); + m_idToObject.emplace(javaObjectID, state); } bool ObjectManager::CloneLink(const Local& src, @@ -460,6 +460,50 @@ int ObjectManager::GenerateNewObjectID() { return oldValue; } +void ObjectManager::ReleaseAllRegistered() { + HandleScope handleScope(m_isolate); + + auto jsInfoIdx = static_cast(MetadataNodeKeys::JsInfo); + + // Detached first: nothing below should observe a half-emptied map, and the + // finalizers these handles were armed with will never run now anyway. + auto survivors = std::move(m_idToObject); + m_idToObject.clear(); + + for (auto& entry : survivors) { + ObjectWeakCallbackState* state = entry.second; + Persistent* po = state->target; + + if (!po->IsEmpty()) { + auto local = po->Get(m_isolate); + if (!local.IsEmpty()) { + // Drop the back-pointer before the JSInstanceInfo goes away. + local->SetInternalField(jsInfoIdx, Undefined(m_isolate)); + } + po->Reset(); + } + + delete po; + delete state->jsInfo; + delete state; + } + + if (m_poJsWrapperFunc != nullptr) { + m_poJsWrapperFunc->Reset(); + delete m_poJsWrapperFunc; + m_poJsWrapperFunc = nullptr; + } +} + +ObjectManager::~ObjectManager() { + // JNI only -- the isolate is already disposed by the time this runs. The LRU + // cache holds a JNI weak global ref per entry (up to its capacity) and only + // ever evicted them under capacity pressure, so a worker that touched Java + // objects abandoned the whole cache when it died. ART's weak-global table is + // bounded, so enough worker cycles turned that leak into an abort. + m_cache.clear(); +} + void ObjectManager::ReleaseJSInstance(Persistent* po, JSInstanceInfo* jsInstanceInfo) { int javaObjectID = jsInstanceInfo->JavaObjectID; @@ -473,9 +517,11 @@ void ObjectManager::ReleaseJSInstance(Persistent* po, throw NativeScriptException(ss.str()); } - assert(po == it->second); + assert(po == it->second->target); + ObjectWeakCallbackState* callbackState = it->second; m_idToObject.erase(it); + delete callbackState; m_released.insert(po, javaObjectID); po->Reset(); diff --git a/test-app/runtime/src/main/cpp/ObjectManager.h b/test-app/runtime/src/main/cpp/ObjectManager.h index 483307697..89493e974 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.h +++ b/test-app/runtime/src/main/cpp/ObjectManager.h @@ -19,6 +19,21 @@ class ObjectManager { public: ObjectManager(jobject javaRuntimeObject); + /* + * Frees the JS side of every still-linked object. Must run on the runtime's + * own thread while the isolate is alive -- it resets v8::Persistents and + * clears internal fields. V8 does not run weak callbacks at isolate + * disposal, so without this every linked wrapper's handle, JSInstanceInfo + * and callback state is abandoned. + */ + void ReleaseAllRegistered(); + + /* + * JNI-only teardown; runs from ~Runtime, after the isolate is disposed and + * while the thread is still attached. Must not touch any v8 handle. + */ + ~ObjectManager(); + void Init(v8::Isolate* isolate); JniLocalRef GetJavaObjectByJsObject(const v8::Local& object); @@ -201,7 +216,7 @@ class ObjectManager { std::stack m_markedForGC; - std::unordered_map*> m_idToObject; + std::unordered_map m_idToObject; PersistentObjectIdSet m_released; diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index e2a450527..907d47068 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -983,6 +983,14 @@ void Runtime::DestroyRuntime() { // is alive and its destructors can still touch v8::Global handles. IsolateTracked::SweepAll(m_isolate); + // Same reason: every still-linked Java<->JS wrapper holds a Persistent, a + // JSInstanceInfo and its weak-callback state, and those finalizers will + // never run now. The JNI half of ObjectManager is released later, in its + // destructor, once the isolate is gone. + if (m_objectManager != nullptr) { + m_objectManager->ReleaseAllRegistered(); + } + // Everything below still needs the isolate alive -- the caller disposes it // only after this returns -- but runs after the hooks above so nothing they // touch is pulled out from under them. From a62f4df8d029cab8cecf747e5b9d6ff4649082e0 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 14 Aug 2026 20:29:36 -0300 Subject: [PATCH 2/2] fix: keep the ObjectManager registration consistent when the link is dropped ReleaseNativeCounterpart frees the JSInstanceInfo and clears the JsInfo internal field but leaves the m_idToObject entry in place. The finalizer that later collects the wrapper then takes its "no JSInstanceInfo" branch, which freed the callback state without unregistering it, so the map was left pointing at freed memory -- and the teardown sweep added here would free it a second time. The finalizer now unregisters via an id carried on the callback state, and ReleaseNativeCounterpart clears the state's back-pointer to the JSInstanceInfo it frees. --- test-app/runtime/src/main/cpp/ObjectManager.cpp | 11 +++++++++++ test-app/runtime/src/main/cpp/ObjectManager.h | 8 +++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/test-app/runtime/src/main/cpp/ObjectManager.cpp b/test-app/runtime/src/main/cpp/ObjectManager.cpp index d4d470910..d900371a1 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.cpp +++ b/test-app/runtime/src/main/cpp/ObjectManager.cpp @@ -364,6 +364,9 @@ void ObjectManager::JSObjectFinalizer(Isolate* isolate, auto jsInstanceInfo = GetJSInstanceInfoFromRuntimeObject(po->Get(m_isolate)); if (jsInstanceInfo == nullptr) { + // The link was already torn down (ReleaseNativeCounterpart); nothing but + // the registration is left to drop. + m_idToObject.erase(callbackState->javaObjectID); po->Reset(); delete po; delete callbackState; @@ -650,6 +653,14 @@ void ObjectManager::ReleaseNativeCounterpart(v8::Local& object) { env.CallVoidMethod(m_javaRuntimeObject, RELEASE_NATIVE_INSTANCE_METHOD_ID, jsInstanceInfo->JavaObjectID); + // The registration outlives the link: it is dropped by the finalizer once + // the wrapper is collected. Until then the entry must not point at the + // JSInstanceInfo being freed here. + auto it = m_idToObject.find(jsInstanceInfo->JavaObjectID); + if (it != m_idToObject.end()) { + it->second->jsInfo = nullptr; + } + delete jsInstanceInfo; auto jsInfoIdx = static_cast(MetadataNodeKeys::JsInfo); object->SetInternalField(jsInfoIdx, Undefined(m_isolate)); diff --git a/test-app/runtime/src/main/cpp/ObjectManager.h b/test-app/runtime/src/main/cpp/ObjectManager.h index 89493e974..23194133a 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.h +++ b/test-app/runtime/src/main/cpp/ObjectManager.h @@ -113,11 +113,17 @@ class ObjectManager { struct ObjectWeakCallbackState { ObjectWeakCallbackState(ObjectManager* _thisPtr, JSInstanceInfo* _jsInfo, v8::Persistent* _target) - : thisPtr(_thisPtr), jsInfo(_jsInfo), target(_target) {} + : thisPtr(_thisPtr), + jsInfo(_jsInfo), + target(_target), + javaObjectID(_jsInfo->JavaObjectID) {} ObjectManager* thisPtr; JSInstanceInfo* jsInfo; v8::Persistent* target; + // Duplicated from jsInfo: the finalizer has to unregister itself even when + // the JsInfo field was already cleared and jsInfo freed. + uint32_t javaObjectID; }; struct GarbageCollectionInfo {