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..d900371a1 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, @@ -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; @@ -460,6 +463,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 +520,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(); @@ -604,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 483307697..23194133a 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); @@ -98,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 { @@ -201,7 +222,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.