fix(wasm): hold the imports object behind a token, not raw NaN-boxed bits (#9611 llhttp differential) - #9649
Conversation
…bits (PerryTS#9611) Closes the last open verification item on PerryTS#9611 — the llhttp differential — and fixes the bug that verification found. THE DIFFERENTIAL. `crates/perry/tests/issue_9611_llhttp_differential.rs` drives both WebAssembly builds of llhttp that undici ships, the way undici drives them: a windowed `Uint8Array` over the engine's linear memory is filled with the socket chunk, `llhttp_execute` runs, and the parser calls back into JS. It covers whole-message, byte-at-a-time and 4 KiB chunkings of simple, chunked, pipelined, 100-continue, many-header and 300 KiB-body responses, and reads every callback span BOTH out of linear memory and through undici's own trick of mapping the wasm pointer back into the source chunk — a span that is right in one view and wrong in the other fails loudly instead of silently agreeing with itself. node's trace is the checked-in oracle; both builds reproduce it identically. THE BUG IT FOUND. The wasm host held the imports object as raw NaN-boxed bits (`perry_wasm_host_instance_set_import_context`), and nothing rooted or rewrote them. A collection triggered INSIDE one import callback relocated the object, so every later import in the same call resolved a stale pointer, `call_wasm_import` returned 0, and the host substituted the import's default result — wasm continued with no error reported anywhere. Through llhttp that silently dropped `on_message_complete`: a truncated HTTP response reported as a clean parse, on cc's network path. It is not a regression from the zero-copy change. The same binary shape reproduces on a perry built from `666481e27` (the commit before PerryTS#9611 landed), and the minimiser isolates it to allocation volume inside the callback, not to memory growth: big_nobuild body=307200 build=false pages=3->6 complete=1 big_build body=307200 build=true pages=6->6 complete=0 and tenuring the imports object before instantiation — so a copying minor cannot move it — restores complete=1 on every run. That is the diagnosis. THE FIX. The host now holds an opaque token; the imports object stays on the runtime side in `WASM_IMPORT_OBJECTS`, which a registered scanner rewrites when a collection moves it. The token is assigned at instantiation, so the start function is covered too, and the per-call `perry_wasm_host_instance_set_import_context` store is gone — one less FFI call on the export path. The scanner is rewrite-only (`visit_metadata_nanbox_f64_slot`), matching the memory binding beside it: every path that can reach an import already roots the imports object on the stack, so this table is a lookup side table, not the reference that keeps the object alive. Marking would instead pin the imports object of every instance ever created. Verified on Linux: byte-identical to node on both llhttp builds; the wasm ESM suite, 14 wasm-host tests and 3,037 runtime tests pass; the root-holder and file-size gates pass. Disabling only the rewrite fails the new test at exactly the right line: node : message_complete perry: execute len=307243 ret=0 errpos=-76384 Claude-Session: https://claude.ai/code/session_01VxP3FEDgV4zUocSDBhD8qh
proggeramlug
added
the
run-extended-tests
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe wasm host now passes opaque import tokens instead of NaN-boxed object bits. The runtime rewrites import references during GC. A new llhttp differential test compares perry output with Node across multiple parser inputs and chunk sizes. ChangesWasm import context and llhttp regression
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, concrete changes, related issue reference, test plan, verification results, fixture details, and confirmation that no version bump was included. It does not use the exact template headings or checklist format, but it contains the required substantive information and is mostly complete. Full details: Docstring CoverageExplanation 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 26 functions across 4 files. (4 skipped: 4 unsupported.)
✨ 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 |
|
Landed via merge train #9653 (rebase-merge, authorship preserved). |
|
Note on the red That gate has failed on every main-line run for the last three days, including Its pinned baseline is stale: most of the rows it flags as REGRESSION are decreases ( This PR cannot affect it in any case: the scanner it adds is behind The first batch of checks also shows as failed — those were cancelled when I added |
1 participant
Closes the last open verification item on #9611 — the llhttp differential — and fixes the bug that verification found.
#9611 listed three acceptance criteria. The three-row benchmark and the views-observability test landed with the zero-copy change; this is the third: "llhttp differential (same HTTP bytes through perry-cc and node-cc, byte-identical parse results)".
The differential
crates/perry/tests/issue_9611_llhttp_differential.rsdrives both WebAssembly builds of llhttp that undici ships, extracted from a cc bundle, the way undici drives them: a windowedUint8Arrayover the engine's linear memory is filled with the socket chunk,llhttp_executeruns, and the parser calls back into JS.The bug it found
The wasm host held the imports object as raw NaN-boxed bits (
perry_wasm_host_instance_set_import_context), and nothing rooted or rewrote them. A collection triggered inside one import callback relocated the object, so every later import in the same call resolved a stale pointer,call_wasm_importreturned 0, and the host substituted the import's default result — wasm continued with no error reported anywhere.Through llhttp that silently dropped
on_message_complete. A truncated HTTP response,ret=0, no error, on cc's network path.It is not a regression from the zero-copy change. The same shape reproduces on a perry built from
666481e27, the commit before #9611 landed. The minimiser isolates it to allocation volume inside the callback, not to memory growth:and tenuring the imports object before instantiation — so a copying minor cannot move it — restores
complete=1on every run:The fix
The host now holds an opaque token; the imports object stays on the runtime side in
WASM_IMPORT_OBJECTS, which a registered scanner rewrites when a collection moves it. The token is assigned at instantiation, so the start function is covered too, and the per-callset_import_contextstore is gone — one less FFI call on the export path.The scanner is rewrite-only (
visit_metadata_nanbox_f64_slot), matching the memory binding beside it: every path that can reach an import already roots the imports object on the stack (call_captured_wasm_exportroots the closure's capture for the whole call; instantiation roots it across the start function), so this table is a lookup side table, not the reference that keeps the object alive. Marking would instead pin the imports object of every instance ever created. The token counter carries anot_a_gc_pointerverdict — it is a monotonic id, never an address, which is the whole point of the indirection.Verified
On the Linux box, against current
main:llhttp.wasmllhttp_simd.wasmDisabling only the rewrite (keeping the token indirection) fails the new test at exactly the right line, so the test cannot pass without the fix:
Fixtures
crates/perry/tests/fixtures/llhttp/carries the two wasm builds (MIT, like undici and llhttp), the driver, node's oracle trace, and a README recording provenance and sha256 for each. 140 KB total.With this, all three of #9611's verification items are done and the issue is ready to close.
No version bump.
https://claude.ai/code/session_01VxP3FEDgV4zUocSDBhD8qh
Summary by CodeRabbit
Bug Fixes
Tests
Documentation