fix: viewport corruption and stale cell data from WASM page memory reuse (#138, #139)#19
Conversation
Ports fixes for upstream issues coder#138 and coder#139: Issue coder#139 (viewport corruption when viewport spans multiple pages): - renderStateGetViewport: replace per-row pages.pin(.active) calls with cached row pins from RenderState.row_data, matching the native renderer. Independent per-row pin resolution produced inconsistent results across page boundaries. - terminal_new_with_config: convert scrollback_limit from line count to bytes using page layout calculation (Terminal.init expects bytes, not lines). This makes the page-spanning condition much less frequent. Issue coder#138 (stale cell data visible after scroll with default cursor style): - cursorDownScroll in Screen.zig: make row clearing unconditional. The old check `if (bg_color != .none)` skipped clearing when cursor style was default (after ESC[0m), leaving stale cells from reused page memory visible on empty lines. Inspired by: coder#133 Inspired by: coder#134 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review
This pull request addresses a WASM viewport and ring-buffer corruption issue by ensuring that the scrollback_limit is correctly converted from lines to bytes and that new row cells are cleared during scrolling to prevent stale data visibility. The Zig C API wrapper was also updated to use cached row pins in renderStateGetViewport for better consistency with the native renderer. Multiple test files were introduced to reproduce the reported bugs and verify the stability of the fixes. A critical feedback point was raised concerning a potential integer overflow in the scrollback byte calculation on 32-bit WASM, with a suggestion to use saturating multiplication to ensure safety.
| + else blk: { | ||
| + // Convert lines to bytes: each page holds cap.rows rows in total_size bytes | ||
| + const cap = pagepkg.std_capacity.adjust(.{ .cols = @intCast(cols) }) catch | ||
| + break :blk scrollback_lines * 1024; // fallback: ~1KB/line | ||
| + const page_size = Page.layout(cap).total_size; | ||
| + const bytes_per_line = page_size / cap.rows; | ||
| + break :blk scrollback_lines * bytes_per_line; | ||
| + }; |
There was a problem hiding this comment.
The calculation of scrollback_limit in bytes can overflow the usize type (which is 32-bit on WASM) if a large scrollback_limit is provided in the configuration. For example, with a large line count and wide terminal columns, the resulting byte count could exceed 4GB. It is safer to use saturating multiplication to handle this case.
else blk: {
// Convert lines to bytes: each page holds cap.rows rows in total_size bytes
const cap = pagepkg.std_capacity.adjust(.{ .cols = @intCast(cols) }) catch
break :blk std.math.mulSaturating(usize, scrollback_lines, 1024);
const page_size = Page.layout(cap).total_size;
const bytes_per_line = page_size / cap.rows;
break :blk std.math.mulSaturating(usize, scrollback_lines, bytes_per_line);
};


Summary
Ports fixes for two related upstream issues caused by WASM internal page memory reuse:
Issue coder#139 —
getViewport()corrupted data across page boundariesrenderStateGetViewport: replaced per-rowpages.pin(.active)calls with cached row pins fromRenderState.row_data, matching how the native renderer reads cell data. Independent per-row resolution of the viewport top-left position could produce inconsistent page references when the viewport straddled an internal page boundary.terminal_new_with_config: convertscrollback_limitfrom line count to bytes using page layout calculation.Terminal.initexpectsmax_scrollbackin bytes, not lines — passing line counts directly resulted in a much smaller scrollback buffer, making the page-boundary condition occur far more frequently.Issue coder#138 — Stale cell data visible on new rows after scrolling
cursorDownScrollinScreen.zig: make row clearing unconditional. The previousif (bg_color != .none)guard skipped clearing when cursor style was default (afterESC[0mreset), leaving stale cell data from previously erased rows visible on new empty lines.Test plan
bun test— 397 pass, 0 fail (13 test files)bun run typecheck— no errorsbun run build:wasm— clean build (patch applies, Zig 0.15.2)iris-repro-final.test.ts— scrollback lengths monotonically increasing, no dropsiris-repro-fix-verify.test.ts— scrollback=10000 has 0 drops with bytes conversionviewport-corruption.test.ts— consistent viewport data after escape-heavy writesviewport-row-merge.test.ts— no row-merge corruption at cols=120/130Inspired by: coder#133
Inspired by: coder#134
🤖 Generated with Claude Code