fix(gc): a typed array's backing ArrayBuffer was invisible to the collector#6408
Merged
proggeramlug merged 2 commits intoJul 14, 2026
Merged
Conversation
…lector
`subarray` on a live typed array returned a garbage-length array after a
collection:
const a = new Int32Array(17);
a.subarray(0, 11).length; // 11
gc();
a.subarray(0, 11).length; // 68 (17 * 4 — the *byte* length)
A plain typed array materializes a backing `ArrayBuffer` the first time one is
needed, and the only reference to it is the raw address recorded in
`TYPED_ARRAY_VIEW_META`. Nothing traced that table, so the buffer was swept out
from under the still-live typed array. (The table's own comment asserted the
opposite — "the backing BufferHeader lives for the thread's lifetime […] free of
use-after-free" — which stopped being true once buffers became collectable.)
Nothing failed loudly. `js_typed_array_backing_buffer` handed the dead pointer to
`js_typed_array_view`, which found it was not a registered ArrayBuffer, quietly
fell back to `js_typed_array_new`, and reinterpreted the dead buffer's *byte*
length as an element count — so a 17-element Int32Array yielded a 68-element
view over a fresh store. The `set` that followed threw "offset is out of bounds",
far from the actual fault. `slice` was unaffected: it copies out of the array's
own storage and never consults the backing.
Trace the table like every other object-keyed side table:
* a mutable root scanner visits each entry's backing and lets the collector
rewrite it, and
* a finalize hook drops a dead typed array's entry, so the buffer is not
rooted forever and a later allocation at the recycled address cannot
inherit a backing that is not its own.
Found compiling a large esbuild-bundled CLI app, whose terminal UI blits the
previous frame with `dst.set(src.subarray(y0, y1), y0)`. Once the process had
GC'd, every frame raised "offset is out of bounds" inside the renderer's
try/catch: the UI painted its first screen and then silently stopped repainting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTyped array view metadata is registered as a mutable GC root, typed arrays clear metadata during finalization, and regression tests validate backing-buffer and subarray behavior under default and evacuation GC modes. ChangesTyped array backing GC
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GC
participant gc_init
participant scan_typed_array_view_meta_roots_mut
participant RuntimeRootVisitor
participant gc_type_finalize_unmarked_payload
participant typedarray_view
GC->>gc_init: run registered metadata root scanner
gc_init->>scan_typed_array_view_meta_roots_mut: scan ViewMeta backing pointers
scan_typed_array_view_meta_roots_mut->>RuntimeRootVisitor: visit and rewrite backing pointer
RuntimeRootVisitor-->>scan_typed_array_view_meta_roots_mut: return updated pointer
GC->>gc_type_finalize_unmarked_payload: finalize unmarked typed array
gc_type_finalize_unmarked_payload->>typedarray_view: clear_view_meta(array buffer header)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_typed_array_backing_gc.sh (1)
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid disabling
errexitto prevent masking failures.Temporarily disabling
set -ecan mask unexpected failures if additional commands are ever inserted inside this block. You can keeperrexitenabled and explicitly handle failures using||instead.♻️ Proposed refactor
- set +e + rc=0 if [[ "$mode" == "evacuate" ]]; then - PERRY_GC_FORCE_EVACUATE=1 "$BIN" >"$OUT_LOG" 2>&1 + PERRY_GC_FORCE_EVACUATE=1 "$BIN" >"$OUT_LOG" 2>&1 || rc=$? else - "$BIN" >"$OUT_LOG" 2>&1 + "$BIN" >"$OUT_LOG" 2>&1 || rc=$? fi - rc=$? - set -e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_typed_array_backing_gc.sh` around lines 33 - 40, Update the command execution block in the mode-based test flow to keep errexit enabled, and explicitly capture the binary’s exit status using an || handling pattern around the evacuate and non-evacuate invocations. Preserve the existing PERRY_GC_FORCE_EVACUATE behavior, output redirection, and rc assignment without relying on set +e/set -e.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_typed_array_backing_gc.sh`:
- Around line 33-40: Update the command execution block in the mode-based test
flow to keep errexit enabled, and explicitly capture the binary’s exit status
using an || handling pattern around the evacuate and non-evacuate invocations.
Preserve the existing PERRY_GC_FORCE_EVACUATE behavior, output redirection, and
rc assignment without relying on set +e/set -e.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63541fb9-e45b-4a27-a5e0-62ad89b94f33
📒 Files selected for processing (6)
crates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/tests/alloc.rscrates/perry-runtime/src/gc/types.rscrates/perry-runtime/src/typedarray_view.rstests/issue_typed_array_backing_gc.jstests/test_typed_array_backing_gc.sh
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
1 participant
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.
The bug
A plain typed array materializes its backing
ArrayBufferlazily(
js_typed_array_backing_buffer), and the only reference to that buffer is theraw address recorded in
TYPED_ARRAY_VIEW_META. Nothing traced that table, sothe collector swept the buffer out from under a live typed array.
The table's own comment asserted the invariant that made this "safe":
That is no longer true.
Why it surfaced so far from the fault
Nothing failed loudly.
js_typed_array_backing_bufferhanded the dead pointer tojs_typed_array_view, which saw it was not a registered ArrayBuffer and quietlytook its fallback:
js_typed_array_newthen read the dead buffer's byte length as an elementcount, so
subarray(0, 11)on anInt32Array(17)returned a 68-elementarray over a fresh store — no longer a view, and the wrong length. The
RangeError: offset is out of boundsonly appeared later, in thesetthatconsumed it.
slicewas unaffected: it copies out of the array's own storage and neverconsults the backing, which is why the two disagreed.
The fix
Trace the table the way every other object-keyed side table already is:
scan_typed_array_view_meta_roots_mut— a mutable root scanner that visitseach entry's backing and lets the collector rewrite the pointer.
GcFinalizeHookKind::TypedArrayViewMeta— drops a dead typed array's entryon sweep, so the backing is not rooted forever and a later allocation at the
recycled address cannot inherit a backing that is not its own. (A finalize
hook rather than a move hook:
GC_TYPE_TYPED_ARRAYismovable: false.)Test
tests/issue_typed_array_backing_gc.js+tests/test_typed_array_backing_gc.sh:materialize the backing, force collections, then assert
subarraystill reports11 elements,
a.buffer.byteLengthis still 68, and the view still aliases thearray. Run under the default collector and again with
PERRY_GC_FORCE_EVACUATE=1.Without the fix the fixture fails on the first round with the exact symptom:
cargo test -p perry-runtime --release -- --test-threads=1: 1233 passed, 0 failed.How it was found
Compiling a large esbuild-bundled CLI app. Its terminal UI blits the previous
frame with
dst.set(src.subarray(y0, y1), y0)over the screen'sInt32Arraycell buffers. Once the process had GC'd, every frame threw "offset is out of
bounds" inside the renderer's
try/catch— so the app painted its first screenand then silently stopped repainting, with no error anywhere.
Summary by CodeRabbit
Bug Fixes
ArrayBufferstays alive and consistent after garbage collection.Tests