diff --git a/CLAUDE.md b/CLAUDE.md index 3c9a7fcfa..6713862ca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Perry is a native TypeScript compiler written in Rust that compiles TypeScript source code directly to native executables. It uses SWC for TypeScript parsing and LLVM for code generation. -**Current Version:** 0.5.369 +**Current Version:** 0.5.370 ## TypeScript Parity Status @@ -149,6 +149,7 @@ First-resolved directory cached in `compile_package_dirs`; subsequent imports re Keep entries to 1-2 lines max. Full details in CHANGELOG.md. +- **v0.5.370** — Closes #255: perry-jsruntime no longer panics with `RefCell already borrowed` (then `active scope can't be dropped`) 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. The user's exact `gp.use((ctx) => log(ctx.deltaTime))` shape from issue #248 — which had been working around `gp.use(() => counter++)` only — now runs end-to-end. **Two cascading fixes**: (1) `with_runtime` is now re-entrancy-safe via a thread-local `REENTRY_PTR: Cell<*mut JsRuntimeState>` that the outer `with_runtime` body stashes on entry. Inner re-entrant calls (V8 trampoline → Perry callback → `js_get_property` → `with_runtime`) detect the stash and reuse the outer's `&mut JsRuntimeState` instead of trying to acquire a second `RefCell::borrow_mut`. A Drop guard clears the pointer on normal return AND on panic-unwind. `ensure_runtime_initialized` short-circuits when the stash is non-null (re-entrant code can't initialize a runtime that's already initialized). (2) But that alone surfaced a SECOND panic from V8's scope tracker: `state.runtime.handle_scope()` inside the re-entrant FFI conflicts with the trampoline's own active scope (V8 internally has a scope stack and rejects out-of-order Drops). **Fix**: the trampoline now stashes its `&mut HandleScope` in a second thread-local `REENTRY_SCOPE_PTR`, and `js_handle_object_get_property` checks for it and reuses the trampoline's existing scope instead of calling `state.runtime.handle_scope()`. Public helpers `stash_trampoline_scope()` (returns a `TrampolineScopeGuard` for LIFO Drop) and `try_trampoline_scope()` (unsafe — returns `Option<&'a mut HandleScope<'a>>`, lifetime is the caller's responsibility, valid only while the trampoline frame is on the stack). The body of `js_handle_object_get_property` factored into `get_property_with_scope(scope, ...)` so both the normal path (creates a new scope) and the trampoline-reuse path share the same logic. **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) and tracked as a follow-up. **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`. New regression test `test-files/test_issue_255_jsruntime_reentrancy.ts` + fixture `test-files/fixtures/issue_255_jsmod.js` exercises: single property read inside callback (the user's #248 repro), multi-property + nested object property read (`ev.target.id` exercises 2-level re-entrancy), same callback fired twice (verifies REENTRY_SCOPE_PTR stash/restore guard), captured-variable mutation + property read combined. All print correctly; pre-fix the first case panicked. **Verified**: cargo build --release -p perry-runtime -p perry-stdlib -p perry-jsruntime -p perry clean; cargo test --release -p perry-jsruntime --lib 4/0; gap tests 25/28 = baseline; #248 Phase 2B regression test (test_issue_248_phase2b_js_callback.ts) still passes (`call0 counter: 1`, `call1 received: 42`, `call2 sum: 30`, `call3 sum: 6`, `callTwice count: 2`). - **v0.5.369** — Closes #248 (Phase 2B): wire `JsCreateCallback` end-to-end so Perry closures passed to JS-imported functions (`arr.forEach(cb)`, `gameLoop.use(handler)`, etc.) actually fire. Phase 2A (v0.5.368) bailed at codegen for this variant because the runtime FFI `js_create_callback(func_ptr, closure_env, param_count)` expects `func_ptr` to have signature `(closure_env: i64, args_ptr: *const f64, args_len: i64) -> f64` (the `native_callback_trampoline` contract at `crates/perry-jsruntime/src/interop.rs:993`), but Perry closure bodies have `(closure_ptr, arg0, arg1, ...)` per arity — no direct call-compatible mapping. **Phase 2B bridge**: new runtime helper `js_closure_call_array(closure_env, args_ptr, args_len) -> f64` in `crates/perry-runtime/src/closure.rs` mirrors the existing `js_native_call_value` dispatch but takes `closure_env: i64` (raw `*const ClosureHeader`) as its first arg instead of an f64 NaN-boxed value, so the SysV-x64 / Win64 first-arg register lands in the integer slot (rdi/rcx) rather than the float slot (xmm0) — matching the trampoline's `extern "C" fn(i64, *const f64, i64) -> f64` expectation. Body switches on `args_len` and calls the right `js_closure_callN` (0..16). **JsCreateCallback codegen** in `crates/perry-codegen/src/expr.rs` now lowers to: lower the closure expression, unbox to i64 closure pointer, `ptrtoint @js_closure_call_array to i64` for the trampoline target, call `js_create_callback(func_addr, closure_ptr, param_count)`. Result is a NaN-boxed JS handle (V8-handle tag 0x7FFB) that JS code can call like any other JS function. **Closure-collector update** in `crates/perry-codegen/src/collectors.rs`: 8 new arms for the `Js*` HIR family (JsLoadModule, JsGetExport, JsCallFunction, JsCallMethod, JsGetProperty, JsSetProperty, JsNew, JsNewFromHandle, JsCreateCallback) so closures nested inside JS-interop expressions don't fall through the `_ => {}` catch-all and end up referenced via `js_closure_alloc(@perry_closure_*)` with their bodies never defined (clang error: "use of undefined value" — same class of bug as Tier 1.1). **Int32 unbox at the dispatch boundary**: V8 NaN-boxes JS integers with `INT32_TAG=0x7FFE` (perry-jsruntime's `bridge.rs:215`); Perry's closure-body arithmetic uses raw `fadd` / `fsub` / `fmul` on f64 inputs and assumes plain doubles, not tagged values. Pre-fix `cb(10, 20)` from JS into a Perry closure `(a, b) => a + b` returned 10 instead of 30 because `fadd(10.0, NaN-boxed-int32-20)` produces a NaN whose payload `console.log`'s tag-aware unbox decoded as 10 (one of the operands' int32). The dispatch helper now unboxes any INT32_TAG-tagged f64 to a plain double before passing to `js_closure_callN`. New regression test `test-files/test_issue_248_phase2b_js_callback.ts` + fixture `test-files/fixtures/issue_248_phase2b_jsmod.js` exercises 0/1/2/3-arg callbacks with mutable captures plus same-callback-fired-twice (using a method-call dispatcher object so `js_call_method` forces perry-jsruntime to win Mach-O link resolution and V8 actually runs end-to-end). All 5 cases now print correctly: `call0 counter: 1`, `call1 received: 42`, `call2 sum: 30`, `call3 sum: 6`, `callTwice count: 2`. The user's exact `gp.use((ctx) => {...})` shape from `/tmp/issue248/test_callback_simple.ts` also now compiles + runs end-to-end (V8 fires the closure, capture mutations propagate back to outer Perry scope, `done, final counter = 1`). **Known follow-up**: callbacks that read fields from JS-supplied object args (`ctx.deltaTime` inside `gp.use((ctx) => log(ctx.deltaTime))`) trigger a pre-existing perry-jsruntime re-entrancy panic (`RefCell already borrowed` at `crates/perry-jsruntime/src/lib.rs:123`) — V8's trampoline holds the `JsRuntimeState` borrow while invoking the Perry callback, and the callback's `js_get_property` re-enters `with_runtime` on the same borrow. Orthogonal to #248's codegen scope. **Verified**: cargo build --release -p perry-runtime -p perry-stdlib -p perry-jsruntime -p perry clean (note: rebuilding perry-jsruntime is necessary after a perry-runtime change because Cargo bundles the perry-runtime CGU inside `libperry_jsruntime.a` for link-time symbol resolution; without the rebuild the bundled CGU has the OLD source); cargo test --release -p perry-codegen --lib 22/0; gap tests 25/28 = baseline (the 3 failing — `array_methods` / `console_methods` / `typed_arrays` — pre-existing categorical/CI gaps unrelated to this change). - **v0.5.368** — Closes #248: codegen for `arr.push(...src)` and the V8 / perry-jsruntime interop expression family. **Phase 1 (ArrayPushSpread)**: `arr.push(...src)` was rejected by the LLVM backend with `expression ArrayPushSpread not yet supported`. HIR has lowered the variant since v0.5.x (`crates/perry-hir/src/lower/expr_call.rs:2077`); only the codegen arm in `crates/perry-codegen/src/expr.rs` was missing — WASM (`crates/perry-codegen-wasm/src/emit.rs:5441`) and analysis helpers (`collectors.rs:1218`, `walker.rs:786`, `analysis.rs:34`) all knew about it. Fix is a single new arm mirroring `Expr::ArrayPush` at line 3016 (same three receiver storage cases — LocalGet / boxed-captured / plain — and the same realloc-aware writeback). No new runtime helper needed: `js_array_concat(dst, src)` already existed at `crates/perry-runtime/src/array.rs:1011`, comment-as-spec'd "reserved for the internal push-spread desugaring path"; Set sources work transparently via the SET_REGISTRY check inside `js_array_concat`. **Phase 2 (V8 interop, 8 new arms)**: the LLVM backend bailed for **JsLoadModule, JsGetExport, JsCallFunction, JsCallMethod, JsGetProperty, JsSetProperty, JsNew, JsNewFromHandle** — the HIR family `perry-hir/src/js_transform.rs::transform_js_imports` produces whenever a `.ts` entry imports from a `.js` module the resolver classifies as JS-runtime-loaded (`crates/perry/src/commands/compile/collect_modules.rs:73`, extension-driven). Pre-fix the user's `bun i @codehz/pipeline` repro bombed at codegen with `JsCallFunction not yet supported`. New arms call into the existing perry-jsruntime FFI surface: `js_load_module(path_ptr, path_len) -> u64`, `js_get_export/get_property/set_property/call_function/call_method/new_instance/new_from_handle` etc. — all eight already declared in `runtime_decls.rs` except `js_call_method` (added here at line 1631 with signature `DOUBLE, &[DOUBLE, I64, I64, I64, I64]`). New shared helper `lower_js_args_array(ctx, lowered_args) -> (ptr, len)` marshals already-lowered NaN-boxed args into a stack alloca'd `[N x double]` via the issue-#167 `alloca_entry_array` pattern (hoisted to function entry block); empty input returns `("null", "0")` for the FFI's null-pointer fallback. **Module handle representation**: V8 module ids are u64; codegen returns them as f64 via `bitcast_i64_to_double` to fit `lower_expr`'s return-type contract, then consumers bitcast back to i64 before passing to the runtime. **JS value handles** are NaN-boxed f64 with V8-handle tag 0x7FFB — handled internally by perry-jsruntime's `v8_to_native` / `native_to_v8` helpers; codegen treats them as opaque doubles. **Runtime bootstrap**: new `needs_js_runtime: bool` field on `CrossModuleCtx` (threaded from `CompileOptions::needs_js_runtime`, originally set in `collect_modules.rs:105` when any `.js` module enters `ctx.js_modules`), wired into `compile_module_entry` so the entry main's prelude calls `js_runtime_init()` between `js_gc_init` and user code. Without this, every `js_load_module` site bailed at the runtime with `[js_load_module] no JS runtime state!`. **`JsCreateCallback` deliberately deferred** to Phase 2B: the runtime FFI `js_create_callback(func_ptr, closure_env, param_count)` expects `func_ptr` to have signature `(closure_env: i64, args_ptr: *const f64, args_len: i64) -> f64` (see the `native_callback_trampoline` in `crates/perry-jsruntime/src/interop.rs:993`), but Perry closure bodies have `(closure_ptr, arg0, arg1, ...)` per arity — there's no direct call-compatible mapping. Wiring this needs either codegen-emitted per-arity adapter thunks or a runtime-side closure-array dispatcher; for now the arm bails with a clear message so users see exactly what's blocked. The user's exact `pipeline()` repro at `/tmp/issue248/test.ts` now compiles + links + runs (exit 0). **Regression tests**: `test-files/test_issue_248_array_push_spread.ts` (10 cases — number/string/object arrays, empty src/dst, array-literal spread, chained push-spread, post-spread `.indexOf` + `.length`, push-spread inside loop forcing realloc past the 16-cap, mixed `push` + `push-spread` — all byte-for-byte against `node --experimental-strip-types`), plus `test-files/test_issue_248_phase2_js_interop.ts` + fixture `test-files/fixtures/issue_248_jsmod.js` exercising JsLoadModule + JsCallFunction (compile + link + clean exit). **Verified**: cargo build --release -p perry-runtime -p perry-stdlib -p perry-jsruntime -p perry clean; cargo test --release -p perry-codegen --lib 22/0; gap tests 25/28 = baseline. Bumped 0.5.366 → 0.5.368 above origin's parallel-track 0.5.366 (HarmonyOS SDK fix #250) + 0.5.367 (HarmonyOS HAP bundler #252) per the merge-collision precedent. PR #251. - **v0.5.369** — HarmonyOS PR B.4 + B.5 + B.6 squash-equivalent: cherry-picks `3042563a` + `b01653f6` + `41d597c0` (originally v0.5.127 / v0.5.128 / v0.5.129) from the `harmony-os` branch — the audit-driven fixes the original branch made AFTER its own first emulator run. End-to-end `hdc install` is now achievable (modulo cert + bundle-name match). **B.5 (v0.5.128) — DevEco 6.x SDK + ets-loader replacement**: most of B.5's compile.rs work already on main via the v0.5.366 fast-follow (DevEco app-bundle SDK probe + macOS framework leak fix); cherry-pick fold-in here is just the NEW hunks: (a) extends the `is_harmonyos` linker arm in `compile/link.rs` with OHOS runtime libs `-Wl,--allow-multiple-definition -lm -lpthread -ldl -lace_napi.z`. `libace_napi.z.so` is what ArkTS exposes for `napi_module_register` / `napi_create_*` (consumed by `perry-runtime/src/ohos_napi.rs`); OHOS naming convention is `.z.so` and `-l` strips `lib`+`.so` but NOT the middle `.z`, so `-lace_napi.z` is the deliberate spelling. (b) Skip BSD strip on harmonyos targets — macOS strip emits a noisy `non-object and non-archive file` warning on ELF binaries. (c) `crates/perry/src/commands/harmonyos_hap.rs` rewritten to skip the ets-loader Node/rollup pipeline entirely and shell out to `es2abc --extension ts --module --merge-abc` directly — the harmony-os branch found ets-loader needs ~15 env vars (aceModuleRoot, aceModuleBuild, aceModuleJsonPath, aceProfilePath, compileMode=moduleJson, plus a full DevEco build-profile.json5); synthesizing all of that is effectively re-implementing hvigor. The Phase-1 ArkTS shim is plain TypeScript (no `@Entry`/`@Component`/`struct` decorators yet) so es2abc accepts it via the `--extension ts` flag. HAPs now ship a single merged `ets/modules.abc` instead of per-file .abc. PR C reintroduces ets-loader once the TS→ArkUI emitter produces real ArkUI decorators. (d) `EntryAbility.ets` no longer imports `@ohos.window` or has `onWindowStageCreate` with `windowStage.loadContent('pages/Index')`; window stays blank but `console.log` reaches hilog — enough to validate Phase 1's goal of "cross-compile → NAPI bind → TS main() executes". `module.json5` drops `pages: "$profile:main_pages"`, `main_pages.json` no longer emitted, `resources/base/profile/` no longer created. **B.6 (v0.5.129) — native-object pickup**: `compile/link.rs` walks `target///release/build/*/out/` for loose `.o` files emitted by `cc-rs` build scripts (notably `libmimalloc-sys`, which produces a 362-KB `-static.o` containing 154 mi_* symbols). Rust's staticlib normally bundles these into `libperry_runtime.a`, but on macOS→OHOS cross-builds the `libmimalloc.a` wrapper comes out as a zero-member BSD-format archive (BSD ar's `__.SYMDEF SORTED` layout — macOS-host `ar` creates it, llvm-ar can't read it back) and rustc's "bundle native libs into staticlib" silently skips it. Without forwarding the loose `.o` files to the final link, `libentry.so` ends up with `mi_malloc_aligned` marked UND, and the OHOS dynamic linker rejects dlopen at `EntryAbility.onCreate` with "symbol not found." Walked-pickup is coarser than Rust's per-crate link-lib directive walking (picks up `.o` from any transitive C dep, not just mimalloc), but mimalloc is the only C dep in perry-runtime's closure today and unreferenced ones are dead-stripped via the existing `--gc-sections`. **B.4 (v0.5.127) — earlier audit fixes** mostly bundle into B.5/B.6 above (`-appCertFile` vs `-profileFile` distinction in the hap-sign CLI invocation, `developtools_hapsigner` README pointers in code comments). **Cherry-pick fold-in**: 3 cherry-picks across 3 commits required Cargo.toml + CLAUDE.md conflict resolution per commit (mechanical). compile.rs conflicts taken-as-ours each time and the meaningful new hunks (linker libs, native-object pickup) hand-applied to their current homes in `compile/link.rs` since main has refactored that code out of compile.rs. The harmonyos_hap.rs es2abc rewrite + EntryAbility.ets simplification auto-merged cleanly. Bumped 0.5.129 → **0.5.369** (above main's current 0.5.368 from PR #251). diff --git a/Cargo.lock b/Cargo.lock index e07886cdf..e6fd52135 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4160,7 +4160,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perry" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "atty", @@ -4206,7 +4206,7 @@ dependencies = [ [[package]] name = "perry-codegen" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "log", @@ -4218,7 +4218,7 @@ dependencies = [ [[package]] name = "perry-codegen-glance" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-hir", @@ -4226,7 +4226,7 @@ dependencies = [ [[package]] name = "perry-codegen-js" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-dispatch", @@ -4236,7 +4236,7 @@ dependencies = [ [[package]] name = "perry-codegen-swiftui" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-hir", @@ -4245,7 +4245,7 @@ dependencies = [ [[package]] name = "perry-codegen-wasm" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "base64", @@ -4258,7 +4258,7 @@ dependencies = [ [[package]] name = "perry-codegen-wear-tiles" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-hir", @@ -4266,7 +4266,7 @@ dependencies = [ [[package]] name = "perry-diagnostics" -version = "0.5.369" +version = "0.5.370" dependencies = [ "serde", "serde_json", @@ -4274,11 +4274,11 @@ dependencies = [ [[package]] name = "perry-dispatch" -version = "0.5.369" +version = "0.5.370" [[package]] name = "perry-doc-tests" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "clap", @@ -4292,7 +4292,7 @@ dependencies = [ [[package]] name = "perry-hir" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-diagnostics", @@ -4305,7 +4305,7 @@ dependencies = [ [[package]] name = "perry-jsruntime" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "deno_core", @@ -4324,7 +4324,7 @@ dependencies = [ [[package]] name = "perry-parser" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-diagnostics", @@ -4336,7 +4336,7 @@ dependencies = [ [[package]] name = "perry-runtime" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "base64", @@ -4357,7 +4357,7 @@ dependencies = [ [[package]] name = "perry-stdlib" -version = "0.5.369" +version = "0.5.370" dependencies = [ "aes", "aes-gcm", @@ -4423,7 +4423,7 @@ dependencies = [ [[package]] name = "perry-transform" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "perry-hir", @@ -4433,7 +4433,7 @@ dependencies = [ [[package]] name = "perry-types" -version = "0.5.369" +version = "0.5.370" dependencies = [ "anyhow", "thiserror 1.0.69", @@ -4441,11 +4441,11 @@ dependencies = [ [[package]] name = "perry-ui" -version = "0.5.369" +version = "0.5.370" [[package]] name = "perry-ui-android" -version = "0.5.369" +version = "0.5.370" dependencies = [ "itoa", "jni", @@ -4459,7 +4459,7 @@ dependencies = [ [[package]] name = "perry-ui-geisterhand" -version = "0.5.369" +version = "0.5.370" dependencies = [ "rand 0.8.5", "serde", @@ -4469,7 +4469,7 @@ dependencies = [ [[package]] name = "perry-ui-gtk4" -version = "0.5.369" +version = "0.5.370" dependencies = [ "cairo-rs", "gtk4", @@ -4481,7 +4481,7 @@ dependencies = [ [[package]] name = "perry-ui-ios" -version = "0.5.369" +version = "0.5.370" dependencies = [ "block2", "libc", @@ -4496,7 +4496,7 @@ dependencies = [ [[package]] name = "perry-ui-macos" -version = "0.5.369" +version = "0.5.370" dependencies = [ "block2", "libc", @@ -4514,11 +4514,11 @@ version = "0.1.0" [[package]] name = "perry-ui-testkit" -version = "0.5.369" +version = "0.5.370" [[package]] name = "perry-ui-tvos" -version = "0.5.369" +version = "0.5.370" dependencies = [ "block2", "libc", @@ -4533,7 +4533,7 @@ dependencies = [ [[package]] name = "perry-ui-visionos" -version = "0.5.369" +version = "0.5.370" dependencies = [ "block2", "libc", @@ -4548,7 +4548,7 @@ dependencies = [ [[package]] name = "perry-ui-watchos" -version = "0.5.369" +version = "0.5.370" dependencies = [ "block2", "libc", @@ -4561,7 +4561,7 @@ dependencies = [ [[package]] name = "perry-ui-windows" -version = "0.5.369" +version = "0.5.370" dependencies = [ "libc", "perry-runtime", @@ -4573,7 +4573,7 @@ dependencies = [ [[package]] name = "perry-updater" -version = "0.5.369" +version = "0.5.370" dependencies = [ "base64", "ed25519-dalek", diff --git a/Cargo.toml b/Cargo.toml index 59caced9f..bb85cc2cc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,7 +113,7 @@ opt-level = "s" # Optimize for size in stdlib opt-level = 3 [workspace.package] -version = "0.5.369" +version = "0.5.370" edition = "2021" license = "MIT" repository = "https://github.com/PerryTS/perry" diff --git a/crates/perry-jsruntime/src/interop.rs b/crates/perry-jsruntime/src/interop.rs index b0abfc4b4..0791a0b4d 100644 --- a/crates/perry-jsruntime/src/interop.rs +++ b/crates/perry-jsruntime/src/interop.rs @@ -684,33 +684,51 @@ pub extern "C" fn js_handle_object_get_property( Err(_) => return f64::from_bits(0x7FFC_0000_0000_0001), }; + // Issue #255: when called from inside a V8 callback trampoline, + // reuse the trampoline's scope rather than creating a new one via + // `state.runtime.handle_scope()`. The latter clashes with V8's + // scope-stack tracking under deno_core (panics with "active scope + // can't be dropped" when the inner scope drops). The trampoline + // stashes its scope ptr in REENTRY_SCOPE_PTR; this branch picks + // it up. Outside a callback, fall through to the normal path. + if let Some(scope) = unsafe { crate::try_trampoline_scope() } { + return get_property_with_scope(scope, object_ptr, property_name); + } + with_runtime(|state| { let scope = &mut state.runtime.handle_scope(); + get_property_with_scope(scope, object_ptr, property_name) + }) +} - // Convert the object pointer to a V8 object - let obj_val = native_to_v8(scope, object_ptr); - if !obj_val.is_object() { - eprintln!("[js_handle_object_get_property] value is not an object!"); - return f64::from_bits(0x7FFC_0000_0000_0001); - } +/// Shared body of `js_handle_object_get_property` parameterized over the +/// V8 scope to use — extracted so both the normal path (creates a scope +/// from the runtime) and the trampoline-reuse path (issue #255) share +/// the same logic. +fn get_property_with_scope( + scope: &mut v8::HandleScope, + object_ptr: f64, + property_name: &str, +) -> f64 { + let obj_val = native_to_v8(scope, object_ptr); + if !obj_val.is_object() { + eprintln!("[js_handle_object_get_property] value is not an object!"); + return f64::from_bits(0x7FFC_0000_0000_0001); + } - let obj = obj_val.to_object(scope).unwrap(); + let obj = obj_val.to_object(scope).unwrap(); - // Get the property - let key = match v8::String::new(scope, property_name) { - Some(k) => k, - None => return f64::from_bits(0x7FFC_0000_0000_0001), - }; + let key = match v8::String::new(scope, property_name) { + Some(k) => k, + None => return f64::from_bits(0x7FFC_0000_0000_0001), + }; - let prop_val = match obj.get(scope, key.into()) { - Some(v) => v, - None => { - return f64::from_bits(0x7FFC_0000_0000_0001); - } - }; + let prop_val = match obj.get(scope, key.into()) { + Some(v) => v, + None => return f64::from_bits(0x7FFC_0000_0000_0001), + }; - v8_to_native(scope, prop_val) - }) + v8_to_native(scope, prop_val) } /// Convert a JavaScript handle value to a native string @@ -1030,6 +1048,15 @@ fn native_callback_trampoline( native_args.push(v8_to_native(scope, arg)); } + // Issue #255: stash this scope so re-entrant FFIs (e.g. js_get_property + // called from inside the Perry callback to read `ctx.deltaTime`) can + // reuse it instead of calling state.runtime.handle_scope() — which + // V8's scope tracking rejects with "active scope can't be dropped" + // because we'd be creating a new scope above the one V8 itself has + // active for this trampoline call. Guard auto-restores any prior + // stashed scope on Drop, so nested trampoline invocations work. + let _scope_guard = crate::stash_trampoline_scope(scope); + // Call the native function // Function signature: fn(closure_env: i64, args_ptr: *const f64, args_len: i64) -> f64 type CallbackFn = extern "C" fn(i64, *const f64, i64) -> f64; diff --git a/crates/perry-jsruntime/src/lib.rs b/crates/perry-jsruntime/src/lib.rs index a91d18aa2..d1c051c9c 100644 --- a/crates/perry-jsruntime/src/lib.rs +++ b/crates/perry-jsruntime/src/lib.rs @@ -28,7 +28,7 @@ pub use perry_stdlib; use deno_core::{JsRuntime, RuntimeOptions}; use once_cell::sync::OnceCell; -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::collections::HashMap; use std::path::PathBuf; use tokio::runtime::Runtime as TokioRuntime; @@ -40,6 +40,33 @@ thread_local! { /// Thread-local V8 runtime instance /// JsRuntime is not Send, so it must be thread-local static JS_RUNTIME: RefCell> = const { RefCell::new(None) }; + + /// Issue #255 — re-entrancy escape hatch. While the outer `with_runtime` + /// holds the `JS_RUNTIME.borrow_mut()` lock, V8 can call back into Perry + /// (via `native_callback_trampoline` → Perry closure body), which may + /// then call FFIs like `js_get_property` that themselves go through + /// `with_runtime` again. Pre-fix the inner `borrow_mut()` panicked with + /// "RefCell already borrowed". This raw-pointer mirror lets the inner + /// call reuse the outer's `&mut JsRuntimeState` instead of trying to + /// acquire a second borrow. Lifetime: the pointer is valid only while + /// the outer `with_runtime` body is on the stack; a Drop guard clears + /// it on normal return AND on panic-unwind. + static REENTRY_PTR: Cell<*mut JsRuntimeState> = const { Cell::new(std::ptr::null_mut()) }; + + /// Issue #255 — V8 scope passthrough for callback re-entrancy. When V8 + /// invokes `native_callback_trampoline`, it gives us a live + /// `&mut HandleScope` on the call stack. Re-entrant FFIs (`js_get_property`, + /// `js_set_property`, etc. called from inside the Perry callback) MUST + /// use that scope rather than calling `state.runtime.handle_scope()` — + /// the latter clashes with deno_core's internal scope tracking and + /// V8 panics with "active scope can't be dropped" on the inner scope's + /// Drop. The trampoline stashes its scope pointer here on entry and + /// clears it on exit (via Drop guard); FFIs check this stash and use + /// the trampoline's scope directly when non-null. The pointer's `'static` + /// lifetime is a lie — it's only valid while the trampoline frame is + /// on the stack — but that's exactly the window where re-entrant FFIs + /// can be called. + static REENTRY_SCOPE_PTR: Cell<*mut std::ffi::c_void> = const { Cell::new(std::ptr::null_mut()) }; } /// State for the JS runtime @@ -117,8 +144,69 @@ pub fn get_tokio_runtime() -> &'static TokioRuntime { }) } +/// Issue #255 — set the trampoline's V8 scope as the re-entrancy escape +/// hatch. Returns a guard that clears the stash on Drop (LIFO so nested +/// trampoline invocations restore the previous scope). +/// +/// **Safety**: caller must guarantee that `scope` outlives every FFI call +/// that might check the stash. In practice this is always true: the +/// trampoline holds `&mut HandleScope` on its stack frame while invoking +/// the Perry callback, and re-entrant FFIs only fire while the callback +/// is running. +pub struct TrampolineScopeGuard { + prev: *mut std::ffi::c_void, +} + +impl Drop for TrampolineScopeGuard { + fn drop(&mut self) { + REENTRY_SCOPE_PTR.with(|p| p.set(self.prev)); + } +} + +pub fn stash_trampoline_scope(scope: &mut deno_core::v8::HandleScope) -> TrampolineScopeGuard { + let prev = REENTRY_SCOPE_PTR.with(|p| p.get()); + let scope_ptr = scope as *mut deno_core::v8::HandleScope as *mut std::ffi::c_void; + REENTRY_SCOPE_PTR.with(|p| p.set(scope_ptr)); + TrampolineScopeGuard { prev } +} + +/// Issue #255 — try to get the trampoline's stashed V8 scope for +/// re-entrant FFI calls. Returns `Some(&mut HandleScope)` when called +/// from inside a `native_callback_trampoline` invocation, +/// `None` otherwise. +/// +/// **Safety**: the returned reference is only valid for the duration of +/// the current synchronous call chain (until the trampoline's stack +/// frame is unwound). Don't store it across `await` points or threads. +/// +/// # Safety +/// +/// Caller must ensure the returned reference doesn't outlive the +/// trampoline frame that stashed it. +pub unsafe fn try_trampoline_scope<'a>() -> Option<&'a mut deno_core::v8::HandleScope<'a>> { + let stashed = REENTRY_SCOPE_PTR.with(|p| p.get()); + if stashed.is_null() { + return None; + } + // Cast back to a HandleScope reference. The lifetime 'a is unconstrained + // here — it's the caller's responsibility to use the reference only + // within the trampoline's frame lifetime. + let scope: &mut deno_core::v8::HandleScope<'_> = + &mut *(stashed as *mut deno_core::v8::HandleScope<'_>); + Some(std::mem::transmute(scope)) +} + /// Initialize the JS runtime for the current thread pub fn ensure_runtime_initialized() { + // Issue #255 — short-circuit when re-entered from a V8 callback. + // The outer `with_runtime` already holds the borrow + has stashed its + // `&mut JsRuntimeState` in `REENTRY_PTR`; doing `borrow_mut` here + // again would panic. Since the state must already be initialized + // (we couldn't be inside `with_runtime` otherwise), there's nothing + // to do. + if REENTRY_PTR.with(|p| !p.get().is_null()) { + return; + } JS_RUNTIME.with(|cell| { let mut opt = cell.borrow_mut(); if opt.is_none() { @@ -127,20 +215,61 @@ pub fn ensure_runtime_initialized() { }); } -/// Execute a closure with the JS runtime +/// Execute a closure with the JS runtime. +/// +/// **Re-entrancy (issue #255):** safe to call from inside a V8 callback +/// invoked while another `with_runtime` body is on the stack. The inner +/// call detects the outer's stashed `REENTRY_PTR` and reuses the same +/// `&mut JsRuntimeState` instead of trying to acquire a second +/// `RefCell::borrow_mut`. This is the standard callback-driven +/// re-entrancy pattern: the outer `&mut` reference is paused (control +/// is in V8 → trampoline → Perry callback) while the inner reference +/// is active, so they never alias in time. pub fn with_runtime(f: F) -> R where F: FnOnce(&mut JsRuntimeState) -> R, { + // Re-entrant fast path: outer with_runtime is still on the stack; + // reuse its &mut via the stashed raw pointer instead of borrowing again. + let stashed = REENTRY_PTR.with(|p| p.get()); + if !stashed.is_null() { + // SAFETY: REENTRY_PTR is non-null only while the outer + // `with_runtime` body holds the RefCell borrow AND its &mut is + // suspended on the call stack. The outer reference can't be used + // concurrently because control is here, not at the outer's site. + // The Drop guard below clears the pointer on return / panic so + // the next outer call sees null again. + let state = unsafe { &mut *stashed }; + return f(state); + } + ensure_runtime_initialized(); JS_RUNTIME.with(|cell| { let mut opt = cell.borrow_mut(); let state = opt.as_mut().expect("Runtime should be initialized"); + let state_ptr: *mut JsRuntimeState = state; + + // Stash the pointer so re-entrant calls (V8 → Perry callback → + // js_get_property → with_runtime) take the fast path above. + REENTRY_PTR.with(|p| p.set(state_ptr)); + // Guard clears the pointer on normal return AND on panic-unwind. + // Without this, a panic during `f` would leave a dangling pointer + // that the next thread-local user would dereference. + struct Guard; + impl Drop for Guard { + fn drop(&mut self) { + REENTRY_PTR.with(|p| p.set(std::ptr::null_mut())); + } + } + let _guard = Guard; + f(state) }) } -/// Execute an async closure with the JS runtime +/// Execute an async closure with the JS runtime. +/// +/// Same re-entrancy semantics as `with_runtime` (issue #255). pub fn with_runtime_async(f: F) -> R where F: FnOnce(&mut JsRuntimeState) -> Fut, @@ -148,10 +277,35 @@ where { let tokio_rt = get_tokio_runtime(); tokio_rt.block_on(async { + // Re-entrant fast path mirrors with_runtime. + let stashed = REENTRY_PTR.with(|p| p.get()); + if !stashed.is_null() { + // SAFETY: same as with_runtime — REENTRY_PTR is non-null only + // while the outer with_runtime/with_runtime_async body holds + // the borrow. + let state = unsafe { &mut *stashed }; + return tokio::task::block_in_place(|| { + let local_rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("Failed to create local Tokio runtime"); + local_rt.block_on(f(state)) + }); + } + ensure_runtime_initialized(); JS_RUNTIME.with(|cell| { let mut opt = cell.borrow_mut(); let state = opt.as_mut().expect("Runtime should be initialized"); + let state_ptr: *mut JsRuntimeState = state; + REENTRY_PTR.with(|p| p.set(state_ptr)); + struct Guard; + impl Drop for Guard { + fn drop(&mut self) { + REENTRY_PTR.with(|p| p.set(std::ptr::null_mut())); + } + } + let _guard = Guard; // Use a dedicated current-thread Tokio runtime to avoid thread pool starvation deadlock. // The outer block_on holds a worker thread; using Handle::current().block_on() would // create a nested block_on on the same runtime, deadlocking if async JS operations diff --git a/test-files/fixtures/issue_255_jsmod.js b/test-files/fixtures/issue_255_jsmod.js new file mode 100644 index 000000000..b9adeb89e --- /dev/null +++ b/test-files/fixtures/issue_255_jsmod.js @@ -0,0 +1,24 @@ +// Phase 2B + Phase 2 + #255 fixture: a dispatcher whose methods invoke +// Perry callbacks with JS objects as arguments — exercising the +// trampoline → Perry → js_get_property re-entrancy path that pre-fix +// panicked with "RefCell already borrowed" then "active scope can't +// be dropped". +export function makeDispatcher() { + return { + // Single property read inside callback. + callWithFrame(cb) { + cb({ deltaTime: 16, frameId: 42 }); + }, + // Multiple property reads + nested method call shape. + callWithEvent(cb) { + cb({ type: "click", x: 100, y: 200, target: { id: "btn1" } }); + }, + // Same callback fired twice with different args — exercises the + // trampoline's Drop guard for REENTRY_SCOPE_PTR (each invocation + // stashes a fresh scope; the previous one's guard restores null). + callTwice(cb) { + cb({ count: 1 }); + cb({ count: 2 }); + }, + }; +} diff --git a/test-files/test_issue_255_jsruntime_reentrancy.ts b/test-files/test_issue_255_jsruntime_reentrancy.ts new file mode 100644 index 000000000..aef0baa72 --- /dev/null +++ b/test-files/test_issue_255_jsruntime_reentrancy.ts @@ -0,0 +1,46 @@ +// Issue #255: when a Perry closure passed to a JS-imported function +// (via JsCreateCallback) reads a property from a JS object passed in as +// a callback argument, perry-jsruntime panics with "RefCell already +// borrowed" then "active scope can't be dropped". The closure-marshaling +// itself works (Phase 2B / PR #254) — the panic is on the runtime side +// when js_get_property re-enters with_runtime + creates a new V8 scope +// while the trampoline's scope is still active. +// +// Fix: thread-local REENTRY_SCOPE_PTR stashed by the trampoline, picked +// up by js_handle_object_get_property to reuse the trampoline's existing +// scope instead of creating a conflicting one. Plus thread-local +// REENTRY_PTR for the with_runtime borrow re-entry. + +import { makeDispatcher } from "./fixtures/issue_255_jsmod.js"; + +const d = makeDispatcher(); + +// Single property read inside callback — the user's exact #248 repro. +d.callWithFrame((ctx: any) => { + console.log("frame: deltaTime =", ctx.deltaTime); +}); + +// Multiple property reads, including a nested object read. +d.callWithEvent((ev: any) => { + console.log("event:", ev.type, "at", ev.x, ev.y); + console.log("event target id:", ev.target.id); +}); + +// Same callback fired twice — verifies the trampoline's stash/restore +// guard handles repeated invocations (each invocation gets a fresh +// scope from V8; the guard must clear REENTRY_SCOPE_PTR cleanly between +// them so the second call sees a fresh stash). +d.callTwice((data: any) => { + console.log("count =", data.count); +}); + +// Captured-variable mutation from inside callback that ALSO reads a +// property — exercises the full re-entrant chain (with_runtime borrow +// re-entry + V8 scope reuse + capture writeback). +let total = 0; +d.callWithFrame((ctx: any) => { + total += ctx.deltaTime; +}); +console.log("total after capture+read:", total); + +console.log("done");