From a56dc02cc7a7e7ce1d830a21791367221b97ce11 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Sat, 4 Jul 2026 12:46:27 -0700 Subject: [PATCH] Fix scroll off-by-one in synthetic flood; guard repro against buffer scroll generateColoredAsciiFlood emitted a trailing \r\n after its final cell, so a flood whose last line lands on the bottom row scrolled the screen. In the alt-buffer repro this destroyed the pinned header row in the BUFFER, making the 255/255 signature diff a content change, not renderer corruption. The single-terminal merge path is actually healthy: with the scroll removed the same 30-chunk merge-heavy workload renders the header with zero drift. - generateColoredAsciiFlood: no linefeed after the final cell - WebglReproExperiment: now a green guard; asserts row-1 buffer text first so a content bug can never masquerade as corruption again - WebglVisual: accurate comments; erase rewritten lines so leftover prefill text doesn't look like corruption - WebglSingleTermStress: replace stale EXPECTED-RED comment (test passes) --- .../test/WebglReproExperiment.test.ts | 29 +++++++++++++++---- .../test/WebglSingleTermStress.test.ts | 11 +++---- addons/addon-webgl/test/WebglVisual.test.ts | 25 +++++++++------- test/playwright/SyntheticTui.ts | 7 ++++- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/addons/addon-webgl/test/WebglReproExperiment.test.ts b/addons/addon-webgl/test/WebglReproExperiment.test.ts index 4c2d07576..99034b940 100644 --- a/addons/addon-webgl/test/WebglReproExperiment.test.ts +++ b/addons/addon-webgl/test/WebglReproExperiment.test.ts @@ -1,32 +1,43 @@ /** * Copyright (c) 2025 The xterm.js authors. All rights reserved. * @license MIT - * EXPERIMENT: try to reproduce vscode#322756 garbled single-terminal render. + * + * Single-terminal guard for the vscode#322756 "garbled glyph" class: a pinned + * alt-buffer header must render identically while the body churns enough + * distinct colored glyphs to force texture-atlas page merges. + * + * Note this scenario is GREEN on current master: the single-terminal mid-frame + * merge race was fixed upstream (merge-retry loop in WebglRenderer.renderRows + + * globally monotonic AtlasPage versions). The test asserts the buffer content + * first so a content bug (e.g. an accidental scroll from the churn stream) can + * never masquerade as renderer corruption. */ import test, { expect } from '@playwright/test'; import { ITestContext, createTestContext, openTerminal } from '../../../test/playwright/TestUtils'; -import { captureRowSignatures, expectSignatureMatches, getAtlasPageCount, loadWebglStrict, setMaxAtlasPages, waitForRender, writeAndWaitForRender } from '../../../test/playwright/RendererTestUtils'; +import { captureRowSignatures, expectSignatureMatches, getAtlasPageCount, loadWebglStrict, setMaxAtlasPages, writeAndWaitForRender } from '../../../test/playwright/RendererTestUtils'; import { generateColoredAsciiFlood } from '../../../test/playwright/SyntheticTui'; +const HEADER = ' HEADER_REF_0123456789'; + let ctx: ITestContext; test.beforeAll(async ({ browser }) => { ctx = await createTestContext(browser); }); test.afterAll(async () => await ctx.page.close()); -test.describe('repro #322756', () => { +test.describe('single-terminal merge integrity (#322756 class)', () => { test.skip(({ browserName }) => browserName !== 'chromium'); test.describe.configure({ timeout: 60000 }); - test('header survives merge race with per-chunk sync', async () => { + test('pinned header renders identically across 30 merge-heavy body redraws', async () => { await openTerminal(ctx, { cols: 80, rows: 24 }); await loadWebglStrict(ctx); await setMaxAtlasPages(ctx, 4); const cols = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]; // Alt-buffer TUI: pinned header on row 1, body churns below (like Copilot CLI). - await writeAndWaitForRender(ctx, '\x1b[?1049h\x1b[H HEADER_REF_0123456789'); + await writeAndWaitForRender(ctx, `\x1b[?1049h\x1b[H${HEADER}`); const ref = await captureRowSignatures(ctx, 1, cols); let maxDiff = 0; for (let c = 0; c < 30; c++) { - // redraw header + churn body rows 2..24 with unique glyphs, header stays put + // Redraw body rows 2..24 with unique glyphs; the header row is never touched. let frame = '\x1b[2;1H'; frame += generateColoredAsciiFlood(23 * 78, c * 23 * 78); await writeAndWaitForRender(ctx, frame); @@ -37,6 +48,12 @@ test.describe('repro #322756', () => { } } console.error('[repro] pages=', await getAtlasPageCount(ctx), 'headerMaxDiff=', maxDiff); + + // Guard: the header must still be in the buffer. If this fails the churn + // stream scrolled the screen and any pixel diff is meaningless. + const row1Text = await ctx.page.evaluate(() => (window as any).term.buffer.active.getLine(0)?.translateToString(true)); + expect(row1Text, 'churn stream must not scroll the pinned header out of the buffer').toBe(HEADER); + const after = await captureRowSignatures(ctx, 1, cols); for (let i = 0; i < cols.length; i++) { expectSignatureMatches(after[i], ref[i], 14, `header col ${cols[i]} garbled`); diff --git a/addons/addon-webgl/test/WebglSingleTermStress.test.ts b/addons/addon-webgl/test/WebglSingleTermStress.test.ts index c4eead29d..d4bbe9be4 100644 --- a/addons/addon-webgl/test/WebglSingleTermStress.test.ts +++ b/addons/addon-webgl/test/WebglSingleTermStress.test.ts @@ -37,11 +37,12 @@ test.describe('WebGL single-terminal heavy render + merge', () => { await loadWebglStrict(ctx); }); - test('heavy merge churn must not crash the GlyphRenderer (xtermjs#322756)', async () => { - // EXPECTED RED: with a small page cap, atlas merges shrink the pages array - // while GlyphRenderer.render still indexes the old page count -> throws - // "Cannot read properties of undefined (reading 'version')" and the terminal - // garbles. Capture the crash via pageerror. + test('heavy merge churn must not crash the GlyphRenderer (vscode#322756)', async () => { + // Guard: heavy merges must not throw from GlyphRenderer.render. Note the + // page cap here is lowered AFTER the renderer was built, so _atlasTextures + // keeps its original (large) length and this test cannot hit the + // page-overflow crash; that scenario needs the cap lowered BEFORE loading + // the addon and is covered separately (WebglAtlasOverflow.test.ts). const errors: string[] = []; const onError = (e: Error): void => { errors.push(e.message); }; ctx.page.on('pageerror', onError); diff --git a/addons/addon-webgl/test/WebglVisual.test.ts b/addons/addon-webgl/test/WebglVisual.test.ts index 38949197d..6a1aa6a13 100644 --- a/addons/addon-webgl/test/WebglVisual.test.ts +++ b/addons/addon-webgl/test/WebglVisual.test.ts @@ -1,20 +1,24 @@ /** * Copyright (c) 2025 The xterm.js authors. All rights reserved. * @license MIT - * Produces a strong BEFORE/AFTER visual of single-terminal #322756 garble: - * a plain white header that should stay readable, body churns colored text - * forcing atlas merges. Saves before.png / after.png for manual viewing. + * Produces BEFORE/AFTER screenshots of a single terminal under merge-heavy + * churn for manual inspection (before.png / after.png, mono-*.png). On current + * master the single-terminal merge path is healthy, so the pinned header lines + * are expected to remain readable in the "after" images; a corrupted header in + * these captures indicates a renderer regression. Note the mono "after" body is + * SUPPOSED to look like glyph soup — that is the mixed box-drawing/braille + * payload rendered faithfully, not corruption. */ import test from '@playwright/test'; import { ITestContext, createTestContext, openTerminal } from '../../../test/playwright/TestUtils'; -import { loadWebglStrict, setMaxAtlasPages, waitForRender, writeAndWaitForRender } from '../../../test/playwright/RendererTestUtils'; +import { loadWebglStrict, setMaxAtlasPages, writeAndWaitForRender } from '../../../test/playwright/RendererTestUtils'; import { generateColoredAsciiFlood, generateMixedGlyphBlock } from '../../../test/playwright/SyntheticTui'; test.describe('visual #322756', () => { test.skip(({ browserName }) => browserName !== 'chromium'); test.describe.configure({ timeout: 60000 }); - test('capture before/after header garble', async ({ browser }) => { + test('capture before/after pinned header under merge churn', async ({ browser }) => { const ctx: ITestContext = await createTestContext(browser); await openTerminal(ctx, { cols: 80, rows: 24 }); await loadWebglStrict(ctx); @@ -30,8 +34,9 @@ test.describe('visual #322756', () => { await ctx.page.close(); }); - // Closer to vscode#322756: prefill realistic CLI text, trigger ONE merge, - // rewrite a couple lines -> some lines intact, most garble sparsely on dark bg. + // Issue-like framing: prefill realistic CLI text, force merges with a mixed + // glyph payload, then rewrite the top lines clean. The rewritten header lines + // must stay readable in mono-after.png. test('capture mono before/after (issue-like)', async ({ browser }) => { const ctx: ITestContext = await createTestContext(browser); await openTerminal(ctx, { cols: 80, rows: 24 }); @@ -46,14 +51,14 @@ test.describe('visual #322756', () => { } await writeAndWaitForRender(ctx, pre); await screen.screenshot({ path: 'out-esbuild-test/playwright/mono-before.png' }); - // Overflow the atlas (cap=4) so pages MERGE; churn most rows so their cells - // hold stale slots, then rewrite a few lines clean (like scroll/redraw). + // Overflow the atlas (cap=4) so pages MERGE while most rows keep their cells, + // then rewrite a few lines clean (like scroll/redraw). for (let c = 0; c < 12; c++) { let frame = '\x1b[3;1H'; frame += generateMixedGlyphBlock(c + 1, 18, 78); await writeAndWaitForRender(ctx, frame); } - await writeAndWaitForRender(ctx, '\x1b[1;1H $ Shell grep installed Copilot CLI binary - 53 lines\r\n Thought for 1s'); + await writeAndWaitForRender(ctx, '\x1b[1;1H\x1b[2K $ Shell grep installed Copilot CLI binary - 53 lines\r\n\x1b[2K Thought for 1s'); await screen.screenshot({ path: 'out-esbuild-test/playwright/mono-after.png' }); await ctx.page.close(); }); diff --git a/test/playwright/SyntheticTui.ts b/test/playwright/SyntheticTui.ts index e613020d7..fe657efa9 100644 --- a/test/playwright/SyntheticTui.ts +++ b/test/playwright/SyntheticTui.ts @@ -177,6 +177,11 @@ export function generateMixedGlyphBlock(seed: number = 1, lines: number = 10, co * Emits ASCII letters/words each in a distinct 256-color foreground+background. * Every (char,color) pair is a unique atlas glyph, so this fills/merges atlas * pages with realistic English text (no CJK). Deterministic. + * + * No `\r\n` is emitted after the final cell: when the flood's last line lands on + * the terminal's bottom row a trailing linefeed would scroll the screen (in the + * alt buffer this destroys pinned reference rows and makes glyph-integrity + * assertions fail for buffer-content reasons rather than rendering reasons). * @param cells Number of colored cells to emit. * @param offset Color offset so consecutive chunks use new color combos. */ @@ -188,7 +193,7 @@ export function generateColoredAsciiFlood(cells: number, offset: number = 0): st const fg = (offset + i) % 256; const bg = (offset + i * 7) % 256; out += `\x1b[38;5;${fg}m\x1b[48;5;${bg}m${ch}`; - if ((i + 1) % 78 === 0) { out += '\x1b[0m\r\n'; } + if ((i + 1) % 78 === 0 && i + 1 < cells) { out += '\x1b[0m\r\n'; } } return out + '\x1b[0m'; }