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

[edusperoni]

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 id into a per-runtime table (jsValueAddress stays a long; no Java change). 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, 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.

Cosmetic

IsolateDisposer.h's two namespace-scope definitions are inline (C++17). They were a strong symbol in each of the three including TUs — benign, since the link collapsed them and the map genuinely is shared, but an ODR violation.

---

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. 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.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 25435d64-09d3-4c25-8314-77a5d8d9cd05

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}