Fix dangling staticNode reference on indirect subtree removal - #979
Conversation
When a component containing <Static> is unmounted, removeChild/ removeChildFromContainer calls freeRecursive() on the ancestor, freeing the static node's Yoga WASM memory as part of the subtree. The existing identity check (staticNode === removeNode) only catches direct removal of the <Static> element itself — it does not detect removal of an ancestor that contains it. The dangling staticNode reference then causes getComputedWidth() to access freed WASM memory on the next render, crashing with RuntimeError: memory access out of bounds. This is a follow-up to vadimdemedes#904/vadimdemedes#905 which fixed direct <Static> removal but missed the indirect (ancestor) case. Changes: - Add clearStaticNodeIfContained() which walks up the parent chain from staticNode to detect whether it is contained in the subtree being removed - Null out removeNode.yogaNode after freeRecursive() so stale JS references short-circuit on ?.yogaNode optional chaining - Add regression test for indirect <Static> unmount
|
There are two issues here:
The helper checks whichever root most recently created Please derive the owning root from the host parent passed to
The new assertion is rejected by Prettier, so One smaller concern: clearing The test should also cover ancestor removal directly from the root, plus screen-reader and concurent rendering. |
Addresses review feedback on the dangling-staticNode fix: - Derive the owning root from the removal hook's host parent instead of the module-global `currentRootNode`, so Ink instances with separate stdout streams no longer clobber each other's `staticNode` pointer. - Replace the incomplete top-node-only `yogaNode = undefined` with a centralized `freeYogaSubtree()` in dom.ts that frees the subtree's WASM memory and then nulls the `yogaNode` reference on every DOM node within it. yoga-layout leaves the JS wrapper truthy after freeRecursive(), so this makes the existing `?.yogaNode` guards effective and closes the whole use-after-free class, not just the reported instance. - Fix Prettier formatting that was blocking CI. - Add regression tests: removeChildFromContainer path, two independent Ink instances, screen-reader mode, and concurrent mode. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@sindresorhus Thanks for the thorough review — all points addressed in the latest push. 1. Owning root derived from arguments, not a global
Added a two-instance regression test that reproduces your 2. CI / Prettier Fixed the formatting that was aborting 3. The You're right that nulling only the top node left descendants dangling. I removed that partial mutation and centralized it: This targets the actual root cause — Clearing 4. Additional test coverage Added tests for: ancestor removal directly from the root ( |
|
I found one issue: The dirty flag is still set on the last root that created a This is reproducible with the default |
The module-level currentRootNode global was still used in commitUpdate to set isStaticDirty, causing the dirty flag to land on whichever root most recently created a <Static> node rather than the root being updated. With two Ink instances, appending to the first instance's <Static> after the second instance mounted one would miss the immediate render, losing the new static output. Remove the currentRootNode global entirely — all three call sites (commitUpdate, removeChild, removeChildFromContainer) now derive the owning root by walking parentNode up to ink-root.
|
Thanks — fixed in 8f6e998. Dirty flag now derived from the updated node The Regression test added New test covers the exact repro you described: render |
Follow-up to #904 / #905.
Problem
PR #905 fixed the case where
<Static>is directly removed — theremoveNode.internal_staticidentity check clearsstaticNode. But when an ancestor of<Static>is removed (e.g. a parent<Box>is conditionally unmounted),removeChildreceives the ancestor, whoseinternal_staticisfalse. The check skips,freeRecursive()frees the entire subtree including the static node's Yoga WASM memory, andstaticNodesurvives as a dangling reference.The next render then calls
node.staticNode.yogaNode.getComputedWidth()inrenderer.ts. The JS wrapper object is still truthy afterfree()(yoga-layout does not invalidate it), so the?.yogaNodeoptional chain passes — and the WASM call traps withRuntimeError: memory access out of bounds.This was reported in production by QwenLM/qwen-code#6820 (Qwen Code CLI), where
transcriptFreezetoggles between<App/>(which contains<Static>) and<TranscriptView/>, unmounting the entire<App/>subtree.Fix
Two changes in
reconciler.ts:clearStaticNodeIfContained()— called beforeremoveChildNode(while the parent chain is still intact), walks up fromstaticNodeto check whetherremoveNodeis an ancestor. If so, clearsstaticNodeso the renderer skips the static output path. This subsumes the previous direct-removal identity check.removeNode.yogaNode = undefined— afterfreeRecursive(), nulls out the freed WASM pointer on the removed DOM node. This is defense-in-depth: any stale JS reference to the removed node will short-circuit on?.yogaNodeinstead of trapping into invalid WASM memory.Test
Added a regression test in
test/components.tsxthat:<Static>nested inside a<Box>wrapper<Static>directly)All existing Static-related tests continue to pass (direct unmount, key-driven remount via both
removeChildandremoveChildFromContainerpaths,fullStaticOutputreset).