fix: runtime lifetime — deferred leaks, cross-isolate sharing, startup robustness - #2013

Description

Works through the items tracked in #2010 — the lifetime problems found while fixing the intermittent worker SIGSEGV (#2006) and the ObjectManager teardown (#2008), and deliberately deferred there. The HMRSupport item is excluded; it is handled in a separate PR.

Two facts from the tracking issue underpin most of this, and every fix below is placed accordingly:

• V8 does not run weak callbacks when an isolate is disposed, so anything freed only from a GC finalizer leaks every instance alive when a runtime goes away.
• v8::Persistent does not reset in its destructor and v8::Global does, which decides whether a fix belongs in DestroyRuntime (isolate alive) or ~Runtime (isolate disposed).

Startup robustness

Main-runtime initialization and election are now serialized. initRuntime calls the synchronized constructor and then runtime.init() outside that block, so the s_mainThreadInitialized check-then-act was unprotected: two concurrent bootstraps could both run InitializeV8(), both elect a main runtime, and overwrite Runtime::platform / s_mainEventLoop. ElectMainRuntime() now decides the winner and performs the once-per-process V8 initialization in one critical section.

Election is kept separate from readiness, because the elected runtime is not usable by others until it has built the metadata tree they all read. A runtime that loses the election blocks until the main runtime signals ready (or fails) — today that wait returns immediately, since workers are only ever created from an initialized main runtime, but it no longer depends on that ordering. The four s_mainThreadInitialized reads inside PrepareV8Runtime were all really "am I the main runtime?" and now read the decided m_isMainThread.

Partial native initialization is now unwound. If PrepareV8Runtime throws after Isolate::New(), the isolate was already in s_isolate2RuntimesCache while the Java-side rollback only unwound Java state. UnwindFailedInit() reuses the two existing teardown windows rather than adding a third cleanup path, and the Runtime itself is freed. An in-flight NativeScriptException may hold a handle into the isolate about to be disposed, so it drops that handle first and reports from the message and stack it already extracted.

Leaks

MetadataNodeCache now owns the callback payloads. TypeMetadata, FieldCallbackData, PropertyCallbackData and ExtendedClassCallbackData (which also held a strong Persistent<Object> pinning the whole JS implementation object) had no finalizer at all and leaked on every GC; MetadataNode.cpp contained no delete. They are now owned by the per-runtime cache and freed with it, while the isolate is still alive.

This also resolves CtorCacheData::instanceMethodCallbacks, which #2008 left pending an ownership analysis. An arena is the answer to that analysis: the same MethodCallbackData is reachable from a prototype method, from CtorCacheData, and from the instanceMethodsCallbackData a derived class copies out of the cache, so a single owner sidesteps the sharing entirely.

ModuleInternal::m_loadedModules is released in ~ModuleInternal, deduplicated by pointer — TempModule inserts the same Persistent under both m_modulePath and m_cacheKey, so a naive loop would double-free. A failed module load also no longer leaks its module handle.

NativeScriptException::m_javascriptException — the only one of these that grew inside a live runtime, including the long-lived main one. The raw Persistent<Value>* was handed to Java as a jlong and Java had no way to free it, so every JS error reaching Java pinned its Error and captured stack for the life of the process. Java now receives an opaque, process-wide-unique id into a per-runtime table (jsValueAddress stays a long; no Java change). Ids are unique across runtimes, not per runtime, so a throwable converted back on a different runtime than the one that created it misses the table and falls back to rebuilding the error from the throwable, rather than consuming an unrelated entry that happened to share an id. An entry is dropped when the error is converted back to JS, when the throwable carrying the id is collected (tracked by a JNI weak ref), and at the latest with the runtime. The handle held by the exception object itself is now shared_ptr-owned, so an in-flight copy cannot double-free it.

Cross-isolate sharing

MetadataNode's three static node caches (s_name2NodeCache, s_name2TreeNodeCache, s_treeNode2NodeCache) were mutated from any runtime's thread on every JS-wrapper creation. They are now guarded; the lock covers map access only and is dropped around the metadata reader, so losing a race is possible and resolved at the insert — the entry already in the map wins.

The metadata tree and MetadataReader's buffers. m_v.push_back reallocates a vector that GetNodeById indexed with no bounds check (it is now bounds-checked, logs the offending id, and its callers surface a named error instead of dereferencing null), and m_valueData/m_valueLength is a bump allocator. As the tracking issue notes, this one cannot take a coarse lock: GetOrCreateTreeNodeByName mutates that state while calling back into Java, and a function-scope mutex would be held across ART class loading and, on the .extend() path, dex generation — inverting against the monitor com.tns.Runtime's constructor takes and against the cross-thread callJSMethod wait.

It therefore uses a reentrant lock that can be released to zero mid-section. std::recursive_mutex cannot express that (unlocking it once drops a single level, so a nested caller still excludes everyone), and GetOrCreateTreeNodeByName recurses into itself. The lock is dropped entirely around the Java callback and the child re-checked on reacquire. Ordering rule, as specified: the only permitted successor is Runtime::s_runtimeCacheMutex. GetNodeById is bounds-checked.

isolateBoundObjects_ → RuntimeState

The tracking issue lists IsolateDisposer.h's two namespace-scope definitions only as an ODR nit, but the map itself is the exact shape RuntimeState was introduced to remove: a process-wide Isolate*-keyed container behind a mutex, mutated from any runtime's thread. It existed to hold exactly one object per runtime — Timers.

Timers now lives in RuntimeState like every other per-runtime subsystem, so the map, its mutex and the unique_void_ptr machinery are deleted rather than made inline — which resolves the ODR item by removing the definitions. disposeIsolate() stays for the two builtin-layer hooks.

Timers is consequently destroyed at m_state->Clear() instead of inside disposeIsolate. Checked: ~Timers → Destroy() touches only its own task map, the event loop and the tasks' Java token peers — nothing torn down in between — and both the isolate and JNI are still alive at Clear(), which is what resetting the task handles and deleting the token global refs require.

Known limitation

While the reader's lock is released around the Java callback, a concurrently resolved type can produce a duplicate tree node (the new node is not published to m_v until after the callback returns). Duplicates are wasteful but not corrupting, and the shape predates this change; removing it would mean restructuring the resolution loop.

Does your commit message include the wording below to reference a specific issue in this repo?

Fixes #2010 (all items except HMRSupport's global maps, handled separately).

Related Pull Requests

Follows up #2006, #2007, #2008.

Does your pull request have unit tests?

No new specs — every item is a lifetime/ownership fix with no reachable JS-observable behaviour change, and the concurrency items need two runtimes bootstrapping at once, which the runtime does not currently allow.

Verified with the existing suite on an arm64 emulator (API 35): 878 specs, 0 failures, 0 errors, 4 pre-existing xit( skips — re-run in full after each of the three commits. The run exercises the paths these changes touch — .extend() and runtime dex generation (the reader's lock release), worker create/terminate cycles (the teardown windows), and the JNI reference-leak specs.

The builtin layer's isolate state, and the end of disposeIsolate

The other two disposeIsolate hooks backed three more maps of the same shape: BuiltinLoader's isolateToPrimordials and isolateToBuiltinRequire, and NsBuiltinModules' isolateToRealm, each behind its own mutex. The primordials lookup runs on every builtin call, so that one took a lock on a hot path to reach state that was never actually shared.

All three are now per-runtime: a BuiltinRealm holding the two handles as v8::Globals, and RealmState, which was already a per-runtime struct with a destructor. Reaching either is an isolate data-slot read plus a vector index. GetRealm can now return null (runtime tearing down), so its four callers degrade instead of resurrecting released state, and Instantiate keeps its contract of leaving an exception pending.

With nothing left to release per isolate, disposeIsolate and IsolateDisposer.{h,cpp} are deleted along with the DestroyRuntime call site. RealmState and the BuiltinLoader handles are consequently destroyed at m_state->Clear(); neither destructor runs JS or touches anything torn down in between, and ~RealmState only deletes v8::Persistents, which never call into V8 at all.

Summary by CodeRabbit

• Bug Fixes

  • Improved runtime startup reliability, including safer recovery when initialization fails.
  • Improved cleanup of modules, timers, and JavaScript exceptions during runtime shutdown.
  • Prevented potential metadata and type-resolution conflicts during concurrent access.
  • Improved handling of JavaScript exceptions passed between JavaScript and Java.
  • Added clearer errors for invalid metadata and type references.

• Documentation

  • Updated runtime caching documentation to reflect current behavior.