Skip to content

Fix dangling staticNode reference on indirect subtree removal - #979

Merged
sindresorhus merged 3 commits into
vadimdemedes:masterfrom
chiga0:fix/static-node-dangling-wasm
Aug 3, 2026
Merged

Fix dangling staticNode reference on indirect subtree removal#979
sindresorhus merged 3 commits into
vadimdemedes:masterfrom
chiga0:fix/static-node-dangling-wasm

Conversation

@chiga0

@chiga0 chiga0 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #904 / #905.

Problem

PR #905 fixed the case where <Static> is directly removed — the removeNode.internal_static identity check clears staticNode. But when an ancestor of <Static> is removed (e.g. a parent <Box> is conditionally unmounted), removeChild receives the ancestor, whose internal_static is false. The check skips, freeRecursive() frees the entire subtree including the static node's Yoga WASM memory, and staticNode survives as a dangling reference.

The next render then calls node.staticNode.yogaNode.getComputedWidth() in renderer.ts. The JS wrapper object is still truthy after free() (yoga-layout does not invalidate it), so the ?.yogaNode optional chain passes — and the WASM call traps with RuntimeError: memory access out of bounds.

This was reported in production by QwenLM/qwen-code#6820 (Qwen Code CLI), where transcriptFreeze toggles between <App/> (which contains <Static>) and <TranscriptView/>, unmounting the entire <App/> subtree.

Fix

Two changes in reconciler.ts:

  1. clearStaticNodeIfContained() — called before removeChildNode (while the parent chain is still intact), walks up from staticNode to check whether removeNode is an ancestor. If so, clears staticNode so the renderer skips the static output path. This subsumes the previous direct-removal identity check.

  2. removeNode.yogaNode = undefined — after freeRecursive(), 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 ?.yogaNode instead of trapping into invalid WASM memory.

Test

Added a regression test in test/components.tsx that:

  1. Renders <Static> nested inside a <Box> wrapper
  2. Unmounts the wrapper (not <Static> directly)
  3. Confirms the renderer does not crash and produces correct output on subsequent rerenders

All existing Static-related tests continue to pass (direct unmount, key-driven remount via both removeChild and removeChildFromContainer paths, fullStaticOutput reset).

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
@sindresorhus

Copy link
Copy Markdown
Collaborator

There are two issues here:

  1. The cached static node is tied to a global root.

The helper checks whichever root most recently created <Static>. With two Ink instances using different stdout streams, removing <Static> from the first root after rendering one in the second leaves the first root's pointer stale. I reproduced stale output (first-history\nfirst-2), and further Yoga memory churn could still trigger the reported use-after-free.

Please derive the owning root from the host parent passed to removeChild, use the container directly for removeChildFromContainer, and add a two-root regression test.

  1. CI is currently failing.

The new assertion is rejected by Prettier, so npm test exits before running the tests. Please format it and rerun CI.

One smaller concern: clearing yogaNode only on the removed top-level node does not invalidate descendants freed by freeRecursive(). I would either centralize that cleanup or remove this broader defense-in-depth mutation.

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>
@chiga0

chiga0 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@sindresorhus Thanks for the thorough review — all points addressed in the latest push.

1. Owning root derived from arguments, not a global

clearStaticNodeIfContained now takes the root explicitly. It's derived from the removal hook's host parent (findRootNode(node) walks up to ink-root) rather than the module-global currentRootNode. removeChildFromContainer passes the container directly. So two instances on separate stdout streams no longer clobber each other's pointer.

Added a two-instance regression test that reproduces your first-history\nfirst-2 stale output and confirms it no longer replays.

2. CI / Prettier

Fixed the formatting that was aborting npm test before the suite ran.

3. The yogaNode cleanup — centralized instead of partial

You're right that nulling only the top node left descendants dangling. I removed that partial mutation and centralized it: freeYogaSubtree() in dom.ts frees the subtree's WASM memory and then nulls yogaNode on every DOM node in the removed subtree.

This targets the actual root cause — freeRecursive() leaves each JS wrapper truthy, defeating the ?.yogaNode guards throughout the codebase. Nulling the whole subtree makes those guards effective and closes the entire use-after-free class, not just the reported instance.

Clearing staticNode is still needed alongside it — otherwise the stale pointer suppresses the onStaticChange reset and replays stale static output (and the screen-reader path reads staticNode without a ?.yogaNode guard at all).

4. Additional test coverage

Added tests for: ancestor removal directly from the root (removeChildFromContainer path), the two-instance case above, screen-reader mode, and concurrent mode.

@sindresorhus

Copy link
Copy Markdown
Collaborator

I found one issue:

The dirty flag is still set on the last root that created a <Static> node, instead of on the root being updated. With two Ink instances, render a <Static> in both, trigger a normal update in the first instance, then append an item to its <Static> during the same throttled window. The new item gets removed before the first root gets its immediate render, so it never reaches stdout.

This is reproducible with the default maxFps. The first instance keeps rendering its old static item and the new dynamic frame, but drops the newly added static item. Please derive the owning root from the updated node and add a regression test for updating <Static> in one instance after another instance has mounted <Static>. The existing two-instance test only covers ancestor removal, so it doesn’t excercise this update path.

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.
@chiga0

chiga0 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — fixed in 8f6e998.

Dirty flag now derived from the updated node

The currentRootNode module-level global is removed entirely. All three call sites (commitUpdate, removeChild, removeChildFromContainer) now derive the owning root by walking parentNode up to the ink-root node, so isStaticDirty always lands on the root that owns the updated <Static> node.

Regression test added

New test covers the exact repro you described: render <Static> in two instances, then append to the first instance's <Static> after the second instance mounted one. Before the fix the appended item was dropped; the test now asserts it reaches stdout, and that the second instance's updates remain independent.

@sindresorhus
sindresorhus merged commit 5448e6b into vadimdemedes:master Aug 3, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants