Skip to content

Commit dd87a19

Browse files
committed
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<Object>, 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.
1 parent d87221d commit dd87a19

4 files changed

Lines changed: 90 additions & 4 deletions

File tree

test-app/runtime/src/main/cpp/LRUCache.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,23 @@ class LRUCache {
105105
insert(key, ref);
106106
}
107107

108+
109+
/*
110+
* Evicts every entry, running the evict callback for each. Needed at
111+
* teardown: the callback owns the resource behind each value (JNI weak
112+
* global refs, here), and nothing else releases them -- eviction
113+
* otherwise only happens on capacity pressure or invalidation.
114+
*/
115+
void clear() {
116+
if (m_evictCallback != nullptr) {
117+
for (auto& entry : m_key_to_value) {
118+
m_evictCallback(entry.second.first, m_state);
119+
}
120+
}
121+
m_key_to_value.clear();
122+
m_key_tracker.clear();
123+
}
124+
108125
private:
109126

110127
void evictKey(const key_type& key) {

test-app/runtime/src/main/cpp/ObjectManager.cpp

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ Local<Object> ObjectManager::GetJsObjectByJavaObject(int javaObjectID) {
220220
return handleScope.Escape(Local<Object>());
221221
}
222222

223-
Persistent<Object>* jsObject = it->second;
223+
Persistent<Object>* jsObject = it->second->target;
224224

225225
auto localObject = Local<Object>::New(isolate, *jsObject);
226226
return handleScope.Escape(localObject);
@@ -296,7 +296,7 @@ void ObjectManager::Link(const Local<Object>& object, uint32_t javaObjectID,
296296
// link
297297
object->SetInternalField(jsInfoIdx, jsInfo);
298298

299-
m_idToObject.emplace(javaObjectID, objectHandle);
299+
m_idToObject.emplace(javaObjectID, state);
300300
}
301301

302302
bool ObjectManager::CloneLink(const Local<Object>& src,
@@ -460,6 +460,50 @@ int ObjectManager::GenerateNewObjectID() {
460460
return oldValue;
461461
}
462462

463+
void ObjectManager::ReleaseAllRegistered() {
464+
HandleScope handleScope(m_isolate);
465+
466+
auto jsInfoIdx = static_cast<int>(MetadataNodeKeys::JsInfo);
467+
468+
// Detached first: nothing below should observe a half-emptied map, and the
469+
// finalizers these handles were armed with will never run now anyway.
470+
auto survivors = std::move(m_idToObject);
471+
m_idToObject.clear();
472+
473+
for (auto& entry : survivors) {
474+
ObjectWeakCallbackState* state = entry.second;
475+
Persistent<Object>* po = state->target;
476+
477+
if (!po->IsEmpty()) {
478+
auto local = po->Get(m_isolate);
479+
if (!local.IsEmpty()) {
480+
// Drop the back-pointer before the JSInstanceInfo goes away.
481+
local->SetInternalField(jsInfoIdx, Undefined(m_isolate));
482+
}
483+
po->Reset();
484+
}
485+
486+
delete po;
487+
delete state->jsInfo;
488+
delete state;
489+
}
490+
491+
if (m_poJsWrapperFunc != nullptr) {
492+
m_poJsWrapperFunc->Reset();
493+
delete m_poJsWrapperFunc;
494+
m_poJsWrapperFunc = nullptr;
495+
}
496+
}
497+
498+
ObjectManager::~ObjectManager() {
499+
// JNI only -- the isolate is already disposed by the time this runs. The LRU
500+
// cache holds a JNI weak global ref per entry (up to its capacity) and only
501+
// ever evicted them under capacity pressure, so a worker that touched Java
502+
// objects abandoned the whole cache when it died. ART's weak-global table is
503+
// bounded, so enough worker cycles turned that leak into an abort.
504+
m_cache.clear();
505+
}
506+
463507
void ObjectManager::ReleaseJSInstance(Persistent<Object>* po,
464508
JSInstanceInfo* jsInstanceInfo) {
465509
int javaObjectID = jsInstanceInfo->JavaObjectID;
@@ -473,9 +517,11 @@ void ObjectManager::ReleaseJSInstance(Persistent<Object>* po,
473517
throw NativeScriptException(ss.str());
474518
}
475519

476-
assert(po == it->second);
520+
assert(po == it->second->target);
477521

522+
ObjectWeakCallbackState* callbackState = it->second;
478523
m_idToObject.erase(it);
524+
delete callbackState;
479525
m_released.insert(po, javaObjectID);
480526
po->Reset();
481527

test-app/runtime/src/main/cpp/ObjectManager.h

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,21 @@ class ObjectManager {
1919
public:
2020
ObjectManager(jobject javaRuntimeObject);
2121

22+
/*
23+
* Frees the JS side of every still-linked object. Must run on the runtime's
24+
* own thread while the isolate is alive -- it resets v8::Persistents and
25+
* clears internal fields. V8 does not run weak callbacks at isolate
26+
* disposal, so without this every linked wrapper's handle, JSInstanceInfo
27+
* and callback state is abandoned.
28+
*/
29+
void ReleaseAllRegistered();
30+
31+
/*
32+
* JNI-only teardown; runs from ~Runtime, after the isolate is disposed and
33+
* while the thread is still attached. Must not touch any v8 handle.
34+
*/
35+
~ObjectManager();
36+
2237
void Init(v8::Isolate* isolate);
2338

2439
JniLocalRef GetJavaObjectByJsObject(const v8::Local<v8::Object>& object);
@@ -201,7 +216,7 @@ class ObjectManager {
201216

202217
std::stack<GarbageCollectionInfo> m_markedForGC;
203218

204-
std::unordered_map<int, v8::Persistent<v8::Object>*> m_idToObject;
219+
std::unordered_map<int, ObjectWeakCallbackState*> m_idToObject;
205220

206221
PersistentObjectIdSet m_released;
207222

test-app/runtime/src/main/cpp/Runtime.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,14 @@ void Runtime::DestroyRuntime() {
983983
// is alive and its destructors can still touch v8::Global handles.
984984
IsolateTracked::SweepAll(m_isolate);
985985

986+
// Same reason: every still-linked Java<->JS wrapper holds a Persistent, a
987+
// JSInstanceInfo and its weak-callback state, and those finalizers will
988+
// never run now. The JNI half of ObjectManager is released later, in its
989+
// destructor, once the isolate is gone.
990+
if (m_objectManager != nullptr) {
991+
m_objectManager->ReleaseAllRegistered();
992+
}
993+
986994
// Everything below still needs the isolate alive -- the caller disposes it
987995
// only after this returns -- but runs after the hooks above so nothing they
988996
// touch is pulled out from under them.

0 commit comments

Comments
 (0)