Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions src/vs/workbench/contrib/terminal/browser/terminalInstance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -816,14 +816,23 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
() => this._isVisible,
() => xterm,
async (cols, rows) => {
if (this.isDisposed) {
return;
}
xterm.resize(cols, rows);
await this._updatePtyDimensions(xterm.raw);
},
async (cols) => {
if (this.isDisposed) {
return;
}
xterm.resize(cols, xterm.raw.rows);
await this._updatePtyDimensions(xterm.raw);
},
async (rows) => {
if (this.isDisposed) {
return;
}
xterm.resize(xterm.raw.cols, rows);
await this._updatePtyDimensions(xterm.raw);
}
Expand Down Expand Up @@ -2047,8 +2056,22 @@ export class TerminalInstance extends Disposable implements ITerminalInstance {
}

private async _updatePtyDimensions(rawXterm: XTermTerminal): Promise<void> {
const pixelWidth = rawXterm.dimensions?.css.canvas.width;
const pixelHeight = rawXterm.dimensions?.css.canvas.height;
if (this.isDisposed) {
return;
}
// `rawXterm.dimensions` proxies to xterm.js' RenderService which throws
// `Cannot read properties of undefined (reading 'dimensions')` if the
// renderer was disposed between scheduling and invocation (debounced or
// idle resize callbacks racing with terminal teardown). Guard against
// that here so the optional chaining short-circuits to undefined.
let pixelWidth: number | undefined;
let pixelHeight: number | undefined;
try {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just looks like we want to swallow known error instead of actually addressing root cause.

Seems like actual throw is from xterm not vscode.

@bryanchen-d What is the repro step for this dimension error? Is it causing breakage in terminal behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to find the repro step. Essentially the error comes from calling term.dimensions after term.dispose(), so user shouldn't see visible breakage since the terminal has been disposed.

I think the try-catch here is no-harm here, but I'd agree we can remove it since we have already check isDisposed above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be harm if the root cause is somewhere else and this would prevent us from root-causing the issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, I made a follow up PR to revert that part, can you review?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, already approved.

pixelWidth = rawXterm.dimensions?.css.canvas.width;
pixelHeight = rawXterm.dimensions?.css.canvas.height;
} catch {
// Renderer disposed mid-flight; fall through with undefined dimensions.
}
const roundedPixelWidth = pixelWidth ? Math.round(pixelWidth) : undefined;
const roundedPixelHeight = pixelHeight ? Math.round(pixelHeight) : undefined;
await this._processManager.setDimensions(rawXterm.cols, rawXterm.rows, undefined, roundedPixelWidth, roundedPixelHeight);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ export class TerminalResizeDebouncer extends Disposable {
}

async resize(cols: number, rows: number, immediate: boolean): Promise<void> {
if (this._store.isDisposed) {
return;
}
this._latestX = cols;
this._latestY = rows;

Expand All @@ -49,12 +52,18 @@ export class TerminalResizeDebouncer extends Disposable {
if (win && !this._isVisible()) {
if (!this._resizeXJob.value) {
this._resizeXJob.value = runWhenWindowIdle(win, async () => {
if (this._store.isDisposed) {
return;
}
this._resizeXCallback(this._latestX);
this._resizeXJob.clear();
});
}
if (!this._resizeYJob.value) {
this._resizeYJob.value = runWhenWindowIdle(win, async () => {
if (this._store.isDisposed) {
return;
}
this._resizeYCallback(this._latestY);
this._resizeYJob.clear();
});
Expand All @@ -70,6 +79,9 @@ export class TerminalResizeDebouncer extends Disposable {
}

flush(): void {
if (this._store.isDisposed) {
return;
}
if (this._resizeXJob.value || this._resizeYJob.value) {
this._resizeXJob.clear();
this._resizeYJob.clear();
Expand All @@ -79,6 +91,12 @@ export class TerminalResizeDebouncer extends Disposable {

@debounce(100)
private _debounceResizeX(cols: number) {
// The @debounce decorator schedules a setTimeout that is not tied to the
// disposable store, so this can fire after the terminal/xterm renderer is
// disposed. Bail out to avoid throwing from xterm.js dimension getters.
if (this._store.isDisposed) {
return;
}
this._resizeXCallback(cols);
}
}
Loading