fix(runtime): free a JS block's wrapper only once its isolate's teardown is done

[edusperoni]

Stacked on #500. Draft.

Bug

A JS function passed as a block param becomes a malloc'd Interop::JSBlock, and a BlockWrapper owned by that block is cached in the function's slot. When the last native reference goes, the block's dispose helper frees that wrapper.

The helper used to skip the Locker and free the wrapper as soon as IsolateWrapper::IsValid() was false. ~Runtime makes IsValid() false at its very start: it erases the isolate from isolates_ and calls InvalidateIsolate() without the Locker, and only takes the Locker later. Until its locked section is done, the function's slot can still be read, and it would point at the freed wrapper. Two readers can get to it:

1. ~Runtime itself. DisposeAllRegistered calls GetValue on every registered object. A function registered through interop.FunctionReference is visited after an earlier entry has released the block's last owner. The new test reproduces this.
2. A foreign thread already running the isolate's JS under its Locker (e.g. a GCD queue running a worker block). It re-marshals the same function: GetValue returns the freed wrapper, and TryRetainJSBlock reads the freed block.
   • On Debug worker teardown this window is narrow, because DestroyInspector takes the Locker first.
   • It is wide for Release workers and for shutdownRuntime called from outside JS.

Repro

Test: BlockCacheRaceTests.js → "JS block outliving its worker / is released by the teardown that later disposes its function". The worker script is blockTeardownReleaseWorker.js.

The worker:

1. registers fn through interop.FunctionReference;
2. makes an NSBlockOperation the only owner of the block built from fn;
3. is terminated.

The registration walk goes newest first, so it releases the operation before it reaches fn.

Without the fix:

• ASan: heap-use-after-free READ in BaseDataWrapper::IsGcProtected.
  • The read comes from DisposeValue ← DisposeAllRegistered ← ~Runtime.
  • The wrapper was freed by the JSBlock dispose ← _Block_release ← -[NSBlockOperation dealloc] ← DisposeAllRegistered, on the same thread.
• Plain build: EXC_BAD_ACCESS (KERN_INVALID_ADDRESS at 0xbeadde7d0340) at the virtual wrapper->Type() call in DisposeValue.

Fix: per-isolate lifetime gate

The gate is a process-wide registry in IsolateWrapper.cpp (IsolateGates::), guarded by an UnfairMutex (os_unfair_lock). The table is a never-destroyed robin_hood::unordered_map, so it is still usable while exit() runs static destructors.

• Key: a per-isolate id assigned in Caches::Init. Caches::getGateId() returns it; unlike getIsolateId() it survives InvalidateIsolate, and ids are never reused.
• Entry: {pins, closed}. A missing entry reads as closed.
• Opened in Runtime::Init, right after the isolate's Caches is created, so no IsolateWrapper for the isolate can exist before its entry.
• IsolateWrapper keeps only {Isolate*, int}, and a static_assert keeps it trivially copyable. Its Pin() and IsTornDown() resolve through the registry.
• IsolatePin is the scoped (RAII) pin and holds only the id. Declare it before the Locker, so the Locker is released first.

Why a registry and not a refcounted token: the extended classes' synthesized +initialize/-retain/-release ObjC blocks capture IsolateWrapper by value and live as long as the process. A shared_ptr inside IsolateWrapper would keep each gate alive forever.

~Runtime

• Ordering is unchanged: the early erase/invalidate stays, so new callbacks still bail early.
• The gate is closed as the last step of the locked section, after DisposeAllRegistered, SweepAll, CloseAllPorts and Caches::Remove.
• DisposeIsolateWhenPossible(isolate, gateId) now defers while the isolate is in use or its entry is pinned, using its existing 10 ms re-post. Before Isolate::Dispose it removes the entry, which is closed with zero pins at that point, so no new pin can arrive.
• It never blocks, so a pinned thread waiting on a Locker the tearing-down thread still holds further up the stack, as with shutdownRuntime from JS, can't deadlock it. The deferred block captures only the int id.

JSBlock dispose

1. Pin.
2. Take the Locker.
3. Unless the gate has closed meanwhile, clear the function's slot if it still holds this wrapper, then reset the persistent. The test here is "gate not closed", not IsValid(): IsValid() is already false while the slot is still reachable.
4. Unlock and unpin.
5. Free.

If the pin is refused, the gate is closed and nothing can reach the slot any more, so the helper frees without the Locker.

ArgConverter::MethodCallback

• Pins before its Locker and holds the pin across it.
• Keeps both IsValid() checks: before the Locker, and again under it, so it still bails once the runtime is invalid.
• This closes the fix(runtime): re-check isolate liveness under the Locker in ObjC-to-JS callbacks #475 residual window where a callback waiting on the Locker could outlive Isolate::Dispose.

Fast path: a thread that has already entered the isolate takes no pin. This covers the dispose running inline during DisposeAllRegistered and nested callbacks. An entered isolate stays in use, which already defers DisposeIsolateWhenPossible.

Deadlock exposure: between the isolates_ erase and Close, a block dispose on another thread now waits for the Locker instead of skipping it. That widens an existing hazard: a -dealloc reached from DisposeAllRegistered that dispatch_syncs to a thread waiting on this isolate's Locker would deadlock. This deadlock class already exists while the isolate is valid.

Cache-hit path: unchanged and costs nothing new; it already runs under the Locker.

exit(): never runs ~Runtime, so nothing changes for a dying process.

Not converted

These other foreign-thread IsValid → Locker takers keep the old check-then-lock and its narrow Isolate::Dispose window. They are left for a follow-up decision:

• the ArrayAdapter, DictionaryAdapter and NSDataAdapter methods and deallocs;
• the synthesized method/property callbacks in ClassBuilder.mm;
• AnimationFrame;
• Timers.hpp.

Messaging.cpp never takes the receiving isolate's Locker.

Results

Run	Tests	Failures	Errors	Skipped
ASan (./run_tests.sh -a)	1754	0	0	16
Plain (./run_tests.sh)	1754	0	0	11

The repro test passes in both runs.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9d70c888-6175-4299-85bb-c58cb6bfc7e7

---

📥 Commits

Reviewing files that changed from the base of the PR and between a9f6fb5 and f4c0c51.

---

📒 Files selected for processing (7)

• NativeScript/runtime/ArgConverter.mm
• NativeScript/runtime/Caches.h
• NativeScript/runtime/Interop.mm
• NativeScript/runtime/IsolateWrapper.h
• NativeScript/runtime/Runtime.mm
• TestRunner/app/tests/BlockCacheRaceTests.js
• TestRunner/app/tests/blockTeardownReleaseWorker.js

---

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

---

📝 Walkthrough

Walkthrough

The runtime adds a shared isolate gate and scoped pins. Callback paths acquire pins before accessing the isolate. Teardown closes the gate and defers disposal while pins remain. A worker teardown test covers block release while the worker still holds its function for disposal.

Changes

Isolate teardown coordination

Layer / File(s)	Summary
Gate and wrapper pin contract NativeScript/runtime/Caches.h, NativeScript/runtime/IsolateWrapper.h	Caches owns a shared IsolateGate. The gate tracks active pins and closure. IsolateWrapper retains the gate and adds Pin() and IsTornDown().
Pinned callback access NativeScript/runtime/ArgConverter.mm, NativeScript/runtime/Interop.mm	MethodCallback acquires a pin before entering the V8-scope block and zeroes the return buffer when pinning or wrapper validation fails. JSBlock disposal pins the isolate and conditionally clears its wrapper from the callback slot.
Gate closure and deferred disposal NativeScript/runtime/Runtime.mm, TestRunner/app/tests/BlockCacheRaceTests.js, TestRunner/app/tests/blockTeardownReleaseWorker.js	Runtime teardown closes the gate and defers disposal while it has pins. The worker test releases a block during teardown while the worker still holds its function for disposal.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant MethodCallback
  participant IsolateWrapper
  participant IsolateGate
  participant Runtime
  participant DisposeIsolateWhenPossible
  MethodCallback->>IsolateWrapper: Pin()
  IsolateWrapper->>IsolateGate: TryPin()
  Runtime->>IsolateGate: Close() after teardown
  Runtime->>DisposeIsolateWhenPossible: pass gate for deferred disposal
  DisposeIsolateWhenPossible->>IsolateGate: check active pins

Loading

Suggested reviewers: nathanwalker

---

Merge Risk: ⚪ Minimal · up to f4c0c

No actionable issue remains from this review; the PR is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (3 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: delaying JS block wrapper release until isolate teardown is complete.

---

Full details: Docstring Coverage

Explanation

Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (3 skipped: 3 unsupported.)

---

---

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

---

_{A rabbit guards the gate with care
Pins keep the isolate safe in there
When teardown calls, the gate shuts tight
The last pin leaves; disposal waits
Then hops away into the night.}

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

[edusperoni]

@CodeRabbit review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.