From dec5452fc0ed959f235ae00ca759f7b9f880042b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 6 Jun 2026 21:51:11 +0000 Subject: [PATCH] fix(buffer): avoid shared _extendedAttrs sentinel breaking addon-image EMPTY_SPARSE_EXTENDED was shared across lines via copyFrom; addon-image writes _extendedAttrs directly without copy-on-write, which caused tile eviction failures and broke instanceof Object checks. Use undefined for empty extended maps (keep EMPTY_SPARSE_MAP for _combined). Ensure addon-image forks the map before writes and update the accessor test. Co-authored-by: Daniel Imms --- addons/addon-image/src/ImageStorage.ts | 20 ++++----- addons/addon-image/src/Types.ts | 2 +- addons/addon-image/test/ImageAddon.test.ts | 5 ++- src/common/buffer/BufferLine.ts | 50 ++++++++++------------ 4 files changed, 38 insertions(+), 39 deletions(-) diff --git a/addons/addon-image/src/ImageStorage.ts b/addons/addon-image/src/ImageStorage.ts index 75851343c..f59c7c2ba 100644 --- a/addons/addon-image/src/ImageStorage.ts +++ b/addons/addon-image/src/ImageStorage.ts @@ -436,9 +436,9 @@ export class ImageStorage implements IDisposable { for (let col = 0; col < cols; ++col) { let e: IExtendedAttrsImage; if (line.getBg(col) & BgFlags.HAS_EXTENDED) { - e = line._extendedAttrs[col] ?? EMPTY_ATTRS; + e = line._extendedAttrs?.[col] ?? EMPTY_ATTRS; } else { - const maybeImg = line._extendedAttrs[col] as IExtendedAttrsImage | undefined; + const maybeImg = line._extendedAttrs?.[col] as IExtendedAttrsImage | undefined; if (!maybeImg || maybeImg.imageId === undefined || maybeImg.imageId === -1) { continue; } @@ -461,7 +461,7 @@ export class ImageStorage implements IDisposable { * Also check _extendedAttrs directly for cells where text cleared HAS_EXTENDED. */ while (++col < cols) { - const nextE = line._extendedAttrs[col] as IExtendedAttrsImage | undefined; + const nextE = line._extendedAttrs?.[col] as IExtendedAttrsImage | undefined; if (!nextE || nextE.imageId !== imageId || nextE.tileId !== startTile + count) { break; } @@ -515,7 +515,7 @@ export class ImageStorage implements IDisposable { for (let row = 0; row < rows; ++row) { const line = buffer.lines.get(row) as IBufferLineExt; if (line.getBg(oldCol) & BgFlags.HAS_EXTENDED) { - const e: IExtendedAttrsImage = line._extendedAttrs[oldCol] ?? EMPTY_ATTRS; + const e: IExtendedAttrsImage = line._extendedAttrs?.[oldCol] ?? EMPTY_ATTRS; const imageId = e.imageId; if (imageId === undefined || imageId === -1) { continue; @@ -560,7 +560,7 @@ export class ImageStorage implements IDisposable { const buffer = this._terminal._core.buffer; const line = buffer.lines.get(y) as IBufferLineExt; if (line && line.getBg(x) & BgFlags.HAS_EXTENDED) { - const e: IExtendedAttrsImage = line._extendedAttrs[x] ?? EMPTY_ATTRS; + const e: IExtendedAttrsImage = line._extendedAttrs?.[x] ?? EMPTY_ATTRS; if (e.imageId && e.imageId !== -1) { const orig = this._images.get(e.imageId)?.orig; if (window.ImageBitmap && orig instanceof ImageBitmap) { @@ -580,7 +580,7 @@ export class ImageStorage implements IDisposable { const buffer = this._terminal._core.buffer; const line = buffer.lines.get(y) as IBufferLineExt; if (line && line.getBg(x) & BgFlags.HAS_EXTENDED) { - const e: IExtendedAttrsImage = line._extendedAttrs[x] ?? EMPTY_ATTRS; + const e: IExtendedAttrsImage = line._extendedAttrs?.[x] ?? EMPTY_ATTRS; if (e.imageId && e.imageId !== -1 && e.tileId !== -1) { const spec = this._images.get(e.imageId); if (spec) { @@ -611,7 +611,7 @@ export class ImageStorage implements IDisposable { private _writeToCell(line: IBufferLineExt, x: number, imageId: number, tileId: number): void { if (line._data[x * Cell.SIZE + Cell.BG] & BgFlags.HAS_EXTENDED) { - const old = line._extendedAttrs[x]; + const old = line._extendedAttrs?.[x]; if (old) { if (old.imageId !== undefined) { // found an old ExtendedAttrsImage, since we know that @@ -627,13 +627,13 @@ export class ImageStorage implements IDisposable { return; } // found a plain ExtendedAttrs instance, clone it to new entry - line._extendedAttrs[x] = new ExtendedAttrsImage(old.ext, old.urlId, imageId, tileId); + (line._extendedAttrs ??= {})[x] = new ExtendedAttrsImage(old.ext, old.urlId, imageId, tileId); return; } } // fall-through: always create new ExtendedAttrsImage entry line._data[x * Cell.SIZE + Cell.BG] |= BgFlags.HAS_EXTENDED; - line._extendedAttrs[x] = new ExtendedAttrsImage(0, 0, imageId, tileId); + (line._extendedAttrs ??= {})[x] = new ExtendedAttrsImage(0, 0, imageId, tileId); } private _evictOnAlternate(): void { @@ -652,7 +652,7 @@ export class ImageStorage implements IDisposable { } for (let x = 0; x < this._terminal.cols; ++x) { if (line._data[x * Cell.SIZE + Cell.BG] & BgFlags.HAS_EXTENDED) { - const imgId = line._extendedAttrs[x]?.imageId; + const imgId = line._extendedAttrs?.[x]?.imageId; if (imgId) { const spec = this._images.get(imgId); if (spec) { diff --git a/addons/addon-image/src/Types.ts b/addons/addon-image/src/Types.ts index 53d357a97..18a0c4819 100644 --- a/addons/addon-image/src/Types.ts +++ b/addons/addon-image/src/Types.ts @@ -64,7 +64,7 @@ export interface IExtendedAttrsImage extends IExtendedAttrs { /* eslint-disable */ export interface IBufferLineExt extends IBufferLine { - _extendedAttrs: {[index: number]: IExtendedAttrsImage | undefined}; + _extendedAttrs: {[index: number]: IExtendedAttrsImage | undefined} | undefined; _data: Uint32Array; } diff --git a/addons/addon-image/test/ImageAddon.test.ts b/addons/addon-image/test/ImageAddon.test.ts index f3cde06a8..9d3ae149e 100644 --- a/addons/addon-image/test/ImageAddon.test.ts +++ b/addons/addon-image/test/ImageAddon.test.ts @@ -118,7 +118,10 @@ test.describe('ImageAddon', () => { } // bufferline privates strictEqual(await ctx.page.evaluate('window.term._core.buffer.lines.get(0)._data instanceof Uint32Array'), true); - strictEqual(await ctx.page.evaluate('window.term._core.buffer.lines.get(0)._extendedAttrs instanceof Object'), true); + strictEqual(await ctx.page.evaluate(`(() => { + const ea = window.term._core.buffer.lines.get(0)._extendedAttrs; + return ea === undefined || (typeof ea === 'object' && ea !== null); + })()`), true); // inputhandler privates strictEqual(await ctx.page.evaluate('window.term._core._inputHandler._curAttrData.constructor.name'), '_AttributeData'); strictEqual(await ctx.page.evaluate('window.term._core._inputHandler._parser.constructor.name'), 'EscapeSequenceParser'); diff --git a/src/common/buffer/BufferLine.ts b/src/common/buffer/BufferLine.ts index a83ed8ff6..885799dab 100644 --- a/src/common/buffer/BufferLine.ts +++ b/src/common/buffer/BufferLine.ts @@ -72,20 +72,19 @@ export interface IBufferLineStringCache { * memory allocs / GC pressure can be greatly reduced by reusing the CellData object. */ /** - * Shared empty sparse maps for lines with no combining/extended data. - * Uses a sentinel + copy-on-write (not `undefined`) so `copyFrom` can detect empty maps - * with reference equality. `undefined` was explored but regressed sparse `copyFrom`; - * `Object.freeze` on these sentinels is safe with COW but did not improve perf. + * Shared empty sparse map for lines with no combining data. + * Uses a sentinel + copy-on-write so `copyFrom` can detect empty `_combined` with + * reference equality. `_extendedAttrs` uses `undefined` instead (addon-image writes + * the map directly without BufferLine APIs). */ const EMPTY_SPARSE_MAP: {[index: number]: string} = Object.create(null); -const EMPTY_SPARSE_EXTENDED: {[index: number]: IExtendedAttrs | undefined} = Object.create(null); export class BufferLine implements IBufferLine { protected _data: Uint32Array; /** Sparse cache; only read when `IS_COMBINED_MASK` is set in `_data`. */ protected _combined: {[index: number]: string} = EMPTY_SPARSE_MAP; /** Sparse cache; only read when `HAS_EXTENDED` is set in `_data`. */ - protected _extendedAttrs: {[index: number]: IExtendedAttrs | undefined} = EMPTY_SPARSE_EXTENDED; + protected _extendedAttrs: {[index: number]: IExtendedAttrs | undefined} | undefined; protected _stringCacheEntryRef: WeakRef | undefined; public length: number; @@ -221,7 +220,7 @@ export class BufferLine implements IBufferLine { cell.combinedData = ''; } if (cell.bg & BgFlags.HAS_EXTENDED) { - cell.extended = this._extendedAttrs[index]!; + cell.extended = this._extendedAttrs![index]!; } else { // Do not mutate cell.extended in place: it may still reference this line's map entry from a // prior loadCell into a reused CellData (e.g. $workCell during insert/delete). @@ -240,8 +239,7 @@ export class BufferLine implements IBufferLine { this._combined[index] = cell.combinedData; } if (cell.bg & BgFlags.HAS_EXTENDED) { - this._ensureExtendedAttrsMap(); - this._extendedAttrs[index] = cell.extended; + this._ensureExtendedAttrsMap()[index] = cell.extended; } this._data[index * Constants.CELL_INDICIES + Cell.CONTENT] = cell.content; this._data[index * Constants.CELL_INDICIES + Cell.FG] = cell.fg; @@ -256,8 +254,7 @@ export class BufferLine implements IBufferLine { public setCellFromCodepoint(index: number, codePoint: number, width: number, attrs: IAttributeData): void { this._invalidateStringCache(); if (attrs.bg & BgFlags.HAS_EXTENDED) { - this._ensureExtendedAttrsMap(); - this._extendedAttrs[index] = attrs.extended; + this._ensureExtendedAttrsMap()[index] = attrs.extended; } this._data[index * Constants.CELL_INDICIES + Cell.CONTENT] = codePoint | (width << Content.WIDTH_SHIFT); this._data[index * Constants.CELL_INDICIES + Cell.FG] = attrs.fg; @@ -425,11 +422,13 @@ export class BufferLine implements IBufferLine { } } // remove any cut off extended attributes - const extKeys = Object.keys(this._extendedAttrs); - for (let i = 0; i < extKeys.length; i++) { - const key = parseInt(extKeys[i], 10); - if (key >= cols) { - delete this._extendedAttrs[key]; + if (this._extendedAttrs) { + const extKeys = Object.keys(this._extendedAttrs); + for (let i = 0; i < extKeys.length; i++) { + const key = parseInt(extKeys[i], 10); + if (key >= cols) { + delete this._extendedAttrs[key]; + } } } } @@ -466,7 +465,7 @@ export class BufferLine implements IBufferLine { return; } this._combined = EMPTY_SPARSE_MAP; - this._extendedAttrs = EMPTY_SPARSE_EXTENDED; + this._extendedAttrs = undefined; for (let i = 0; i < this.length; ++i) { this.setCell(i, fillCellData); } @@ -627,8 +626,7 @@ export class BufferLine implements IBufferLine { this._combined[destCol] = src._combined[srcCol]; } if (src._data[srcStart + Cell.BG] & BgFlags.HAS_EXTENDED) { - this._ensureExtendedAttrsMap(); - this._extendedAttrs[destCol] = src._extendedAttrs[srcCol]; + this._ensureExtendedAttrsMap()[destCol] = src._extendedAttrs![srcCol]!; } } @@ -637,10 +635,10 @@ export class BufferLine implements IBufferLine { const srcCombined = line._combined; const srcExtended = line._extendedAttrs; - // Fast path: blank / scroll-recycle source (shared empty sentinel or plain empty object). - if (srcCombined === EMPTY_SPARSE_MAP && srcExtended === EMPTY_SPARSE_EXTENDED) { + // Fast path: blank / scroll-recycle source (empty `_combined` sentinel, no extended map). + if (srcCombined === EMPTY_SPARSE_MAP && !srcExtended) { this._combined = EMPTY_SPARSE_MAP; - this._extendedAttrs = EMPTY_SPARSE_EXTENDED; + this._extendedAttrs = undefined; return; } this._copySparseMapsFromDense(line); @@ -663,7 +661,7 @@ export class BufferLine implements IBufferLine { combined[i] = srcCombined[i]; } if (srcData[start + Cell.BG] & BgFlags.HAS_EXTENDED) { - extendedAttrs[i] = srcExtended[i]; + extendedAttrs[i] = srcExtended![i]; } } } @@ -674,9 +672,7 @@ export class BufferLine implements IBufferLine { } } - private _ensureExtendedAttrsMap(): void { - if (this._extendedAttrs === EMPTY_SPARSE_EXTENDED) { - this._extendedAttrs = {}; - } + private _ensureExtendedAttrsMap(): {[index: number]: IExtendedAttrs | undefined} { + return this._extendedAttrs ??= {}; } }