fix(jsruntime): #255 re-entrancy — Perry callbacks can read JS object props#259
Merged
Merged
Conversation
… props (v0.5.370) When a Perry closure passed to a JS-imported function (via JsCreateCallback, landed in #248 Phase 2B / v0.5.369) reads a property from a JS object passed in as a callback argument, perry-jsruntime panicked twice in a row: first with "RefCell already borrowed" (the inner with_runtime tried to borrow_mut the JsRuntimeState that the outer was still holding), then with "active scope can't be dropped" (the inner FFI created a new V8 HandleScope that conflicted with the trampoline's existing active scope under deno_core's scope tracking). Two cascading fixes in perry-jsruntime: 1. with_runtime + ensure_runtime_initialized are now re-entrancy-safe via thread-local REENTRY_PTR<Cell<*mut JsRuntimeState>>. Outer stashes its &mut on entry; inner re-entrant calls reuse the stashed pointer instead of acquiring a second RefCell::borrow_mut. Drop guard clears the pointer on normal return AND panic-unwind. 2. native_callback_trampoline now stashes its V8 &mut HandleScope in thread-local REENTRY_SCOPE_PTR (via stash_trampoline_scope() which returns a TrampolineScopeGuard for LIFO restore). js_handle_object_get_property checks for the stashed scope and reuses it (via unsafe try_trampoline_scope() helper) instead of calling state.runtime.handle_scope() — the latter conflicts with V8's internal scope-stack tracking when called nested inside a trampoline. js_handle_object_get_property body factored into get_property_with_scope() so both paths (normal: new scope; re-entrant: trampoline's scope) share the same logic. Other re-entrant FFIs (js_set_property, js_call_method nested, js_handle_array_get, js_handle_array_length) still use the state.runtime.handle_scope() pattern and remain panic-prone from inside callbacks. Refactoring them to use try_trampoline_scope is mechanical and tracked as a follow-up. Safety: the raw-pointer escape hatch is the standard callback-driven re-entrancy pattern. The outer &mut reference is paused on the call stack while V8 → trampoline → Perry callback → inner FFI runs; they never alias in time. Strictly UB by Rust's aliasing rules but standard across callback-driven libraries; documented as `unsafe fn`. New regression test test_issue_255_jsruntime_reentrancy.ts + fixture exercises single property read (user's #248 repro), multi-property + nested object property read (ev.target.id is 2-level re-entrancy), same callback fired twice (REENTRY_SCOPE_PTR stash/restore), captured mutation + property read combined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #255. After PR #254 (Phase 2B) wired Perry closures to fire from JS, the user's exact
#248repro still aborted because the callback's body read a property from the JS object arg:```ts
gp.use((ctx: any) => {
console.log("deltaTime =", ctx.deltaTime); // panic
});
```
Two panics in a row, both pre-existing tangles in perry-jsruntime that the new callback path surfaced for the first time:
The fix — two cascading thread-local escape hatches
1. State re-entrancy (`REENTRY_PTR`): outer `with_runtime` stashes its `&mut JsRuntimeState` as a raw pointer on entry. Inner re-entrant calls detect the stash and reuse the pointer instead of re-borrowing. Drop guard clears on normal return AND panic-unwind. `ensure_runtime_initialized` short-circuits when the stash is non-null.
2. V8 scope re-entrancy (`REENTRY_SCOPE_PTR`): `native_callback_trampoline` stashes its `&mut HandleScope` as it enters the Perry callback. `js_handle_object_get_property` checks the stash and reuses the trampoline's scope instead of creating a conflicting one. Public helpers `stash_trampoline_scope()` (returns a `TrampolineScopeGuard`) and `try_trampoline_scope()` (`unsafe fn` — caller responsible for not outliving the trampoline frame).
The body of `js_handle_object_get_property` is factored into `get_property_with_scope(scope, ...)` so both paths (normal: create a scope; re-entrant: reuse trampoline's scope) share the same logic.
Safety analysis
The raw-pointer escape hatch is the standard callback-driven re-entrancy pattern. The outer `&mut state` reference is paused on the call stack while V8 → trampoline → Perry callback → inner FFI runs; the inner reference is only live during that window; they never alias in time. Strictly UB by Rust's aliasing rules, but it's what every callback-driven library that holds state ends up doing in some form (deno_core uses an OpState handoff at await points instead, but our synchronous re-entry case can't suspend). Documented as `unsafe fn try_trampoline_scope`.
Scope of the fix
`js_handle_object_get_property` is the single FFI most commonly hit from inside callbacks (any `obj.field` read on a JS-supplied arg). Other re-entrant FFIs (`js_set_property`, `js_call_method` nested, `js_handle_array_get`, `js_handle_array_length`) use the same `state.runtime.handle_scope()` pattern and remain panic-prone if called from inside callbacks. Refactoring them to use `try_trampoline_scope()` is mechanical (extract body into `_with_scope` helper, add the trampoline-stash branch) — tracked as a follow-up.
Test plan
Known follow-ups
The other re-entrant FFIs (`js_set_property`, nested `js_call_method`, etc.) still panic when called from inside a callback. Each is a mechanical refactor using the same pattern documented here. Worth a sweep PR but out of scope for the immediate user-blocker.