From 3c44096ff8117528fb74e1ee68d9e4fb11901985 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 27 Sep 2024 09:10:57 -0700 Subject: [PATCH 1/3] Fix TerminalInstance leaks Part of #191768 --- src/vs/base/common/lifecycle.ts | 11 +++++++ src/vs/base/test/common/lifecycle.test.ts | 13 +++++++- .../contrib/terminal/browser/terminal.ts | 5 +-- .../terminal/browser/terminalEvents.ts | 22 +++++++++---- .../terminal/browser/terminalInstance.ts | 33 ++++++++++--------- .../terminalStickyScrollContribution.ts | 2 +- 6 files changed, 60 insertions(+), 26 deletions(-) diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index 4cc2d172bf4f6..c5a3a2230139c 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -698,6 +698,17 @@ export class ImmortalReference implements IReference { dispose(): void { /* noop */ } } +/** + * A reference that will be automatically cleared upon dispose. This is useful + * to clear out any references that could lead to leaks upon dispose. + */ +export class AutoClearingReference implements IReference { + private _object: T | undefined; + get object() { return this._object; } + constructor(object: T) { this._object = object; } + dispose(): void { this._object = undefined; } +} + export function disposeOnReturn(fn: (store: DisposableStore) => void): void { const store = new DisposableStore(); try { diff --git a/src/vs/base/test/common/lifecycle.test.ts b/src/vs/base/test/common/lifecycle.test.ts index fd2fa4ef562c3..5fc771384d7e0 100644 --- a/src/vs/base/test/common/lifecycle.test.ts +++ b/src/vs/base/test/common/lifecycle.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import { Emitter } from '../../common/event.js'; -import { DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, toDisposable } from '../../common/lifecycle.js'; +import { AutoClearingReference, DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, toDisposable } from '../../common/lifecycle.js'; import { ensureNoDisposablesAreLeakedInTestSuite, throwIfDisposablesAreLeaked } from './utils.js'; class Disposable implements IDisposable { @@ -263,6 +263,17 @@ suite('Reference Collection', () => { }); }); +suite('AutoClearingReference', () => { + ensureNoDisposablesAreLeakedInTestSuite(); + + test('simple', () => { + const ref = new AutoClearingReference(1); + assert.strictEqual(ref.object, 1); + ref.dispose(); + assert.strictEqual(ref.object, undefined); + }); +}); + function assertThrows(fn: () => void, test: (error: any) => void) { try { fn(); diff --git a/src/vs/workbench/contrib/terminal/browser/terminal.ts b/src/vs/workbench/contrib/terminal/browser/terminal.ts index b4f6779650213..acd54b82ede05 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminal.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminal.ts @@ -7,7 +7,7 @@ import { IDimension } from '../../../../base/browser/dom.js'; import { Orientation } from '../../../../base/browser/ui/splitview/splitview.js'; import { Color } from '../../../../base/common/color.js'; import { Event, IDynamicListEventMultiplexer, type DynamicListEventMultiplexer } from '../../../../base/common/event.js'; -import { DisposableStore, IDisposable } from '../../../../base/common/lifecycle.js'; +import { DisposableStore, IDisposable, type IReference } from '../../../../base/common/lifecycle.js'; import { OperatingSystem } from '../../../../base/common/platform.js'; import { URI } from '../../../../base/common/uri.js'; import { createDecorator } from '../../../../platform/instantiation/common/instantiation.js'; @@ -628,7 +628,8 @@ export interface ITerminalInstance extends IBaseTerminalInstance { /** * The position of the terminal. */ - target?: TerminalLocation; + target: TerminalLocation | undefined; + targetRef: IReference; /** * The id of a persistent process. This is defined if this is a terminal created by a pty host diff --git a/src/vs/workbench/contrib/terminal/browser/terminalEvents.ts b/src/vs/workbench/contrib/terminal/browser/terminalEvents.ts index c0ae3bc97ff5a..46b4481f9683c 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalEvents.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalEvents.ts @@ -17,14 +17,19 @@ export function createInstanceCapabilityEventMultiplexer { const store = new DisposableStore(); const multiplexer = store.add(new EventMultiplexer<{ instance: ITerminalInstance; data: K }>()); - const capabilityListeners = store.add(new DisposableMap()); + const capabilityListeners = store.add(new DisposableMap>()); function addCapability(instance: ITerminalInstance, capability: ITerminalCapabilityImplMap[T]) { const listener = multiplexer.add(Event.map(getEvent(capability), data => ({ instance, data }))); - capabilityListeners.set(capability, listener); + let instanceCapabilityListeners = capabilityListeners.get(instance.instanceId); + if (!instanceCapabilityListeners) { + instanceCapabilityListeners = new DisposableMap(); + capabilityListeners.set(instance.instanceId, instanceCapabilityListeners); + } + instanceCapabilityListeners.set(capability, listener); } - // Existing capabilities + // Existing instances for (const instance of currentInstances) { const capability = instance.capabilities.get(capabilityId); if (capability) { @@ -32,6 +37,11 @@ export function createInstanceCapabilityEventMultiplexer { + capabilityListeners.deleteAndDispose(instance.instanceId); + })); + // Added capabilities const addCapabilityMultiplexer = store.add(new DynamicListEventMultiplexer( currentInstances, @@ -50,11 +60,11 @@ export function createInstanceCapabilityEventMultiplexer instance.capabilities.onDidRemoveCapability + instance => Event.map(instance.capabilities.onDidRemoveCapability, changeEvent => ({ instance, changeEvent })) )); store.add(removeCapabilityMultiplexer.event(e => { - if (e.id === capabilityId) { - capabilityListeners.deleteAndDispose(e.capability); + if (e.changeEvent.id === capabilityId) { + capabilityListeners.get(e.instance.instanceId)?.deleteAndDispose(e.changeEvent.id); } })); diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index f90969a2c60c4..78bbf8481ba27 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -17,7 +17,7 @@ import { ErrorNoTelemetry, onUnexpectedError } from '../../../../base/common/err import { Emitter, Event } from '../../../../base/common/event.js'; import { KeyCode } from '../../../../base/common/keyCodes.js'; import { ISeparator, template } from '../../../../base/common/labels.js'; -import { Disposable, DisposableStore, IDisposable, MutableDisposable, dispose, toDisposable } from '../../../../base/common/lifecycle.js'; +import { AutoClearingReference, Disposable, DisposableStore, IDisposable, ImmortalReference, MutableDisposable, dispose, toDisposable, type IReference } from '../../../../base/common/lifecycle.js'; import { Schemas } from '../../../../base/common/network.js'; import * as path from '../../../../base/common/path.js'; import { OS, OperatingSystem, isMacintosh, isWindows } from '../../../../base/common/platform.js'; @@ -188,7 +188,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { private _labelComputer?: TerminalLabelComputer; private _userHome?: string; private _hasScrollBar?: boolean; - private _target?: TerminalLocation | undefined; private _usedShellIntegrationInjection: boolean = false; get usedShellIntegrationInjection(): boolean { return this._usedShellIntegrationInjection; } private _lineDataEventAddon: LineDataEventAddon | undefined; @@ -216,9 +215,12 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { this._shellLaunchConfig.waitOnExit = value; } - get target(): TerminalLocation | undefined { return this._target; } + private _targetRef: ImmortalReference = new ImmortalReference(undefined); + get targetRef(): IReference { return this._targetRef; } + + get target(): TerminalLocation | undefined { return this._targetRef.object; } set target(value: TerminalLocation | undefined) { - this._target = value; + this._targetRef.object = value; this._onDidChangeTarget.fire(value); } @@ -317,10 +319,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { readonly onData = this._onData.event; private readonly _onBinary = this._register(new Emitter()); readonly onBinary = this._onBinary.event; - private readonly _onLineData = this._register(new Emitter({ - onDidAddFirstListener: () => this._onLineDataSetup() - })); - readonly onLineData = this._onLineData.event; private readonly _onRequestExtHostProcess = this._register(new Emitter()); readonly onRequestExtHostProcess = this._onRequestExtHostProcess.event; private readonly _onDimensionsChanged = this._register(new Emitter()); @@ -357,6 +355,13 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { private readonly _onDidPaste = this._register(new Emitter()); readonly onDidPaste = this._onDidPaste.event; + private _onLineDataFirstListener: IReference = this._register(new AutoClearingReference(async () => { + const xterm = this.xterm || await this._xtermReadyPromise; + xterm.raw.loadAddon(this._lineDataEventAddon!); + })); + private readonly _onLineData = this._register(new Emitter({ onDidAddFirstListener: this._onLineDataFirstListener.object?.() })); + readonly onLineData = this._onLineData.event; + constructor( private readonly _terminalShellTypeContextKey: IContextKey, private readonly _terminalInRunCommandPicker: IContextKey, @@ -753,7 +758,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { Terminal, this._cols, this._rows, - this._scopedInstantiationService.createInstance(TerminalInstanceColorProvider, this), + this._scopedInstantiationService.createInstance(TerminalInstanceColorProvider, this._targetRef), this.capabilities, this._processManager.shellIntegrationNonce, disableShellIntegrationReporting @@ -775,6 +780,7 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { await this._updatePtyDimensions(xterm.raw); } )); + this._register(toDisposable(() => this._resizeDebouncer = undefined)); this.updateAccessibilitySupport(); this._register(this.xterm.onDidRequestRunCommand(e => { if (e.copyAsHtml) { @@ -863,11 +869,6 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { return xterm; } - private async _onLineDataSetup(): Promise { - const xterm = this.xterm || await this._xtermReadyPromise; - xterm.raw.loadAddon(this._lineDataEventAddon!); - } - async runCommand(commandLine: string, shouldExecute: boolean): Promise { let commandDetection = this.capabilities.get(TerminalCapability.CommandDetection); @@ -2676,7 +2677,7 @@ export function parseExitResult( export class TerminalInstanceColorProvider implements IXtermColorProvider { constructor( - private readonly _instance: ITerminalInstance, + private readonly _target: IReference, @IViewDescriptorService private readonly _viewDescriptorService: IViewDescriptorService, ) { } @@ -2686,7 +2687,7 @@ export class TerminalInstanceColorProvider implements IXtermColorProvider { if (terminalBackground) { return terminalBackground; } - if (this._instance.target === TerminalLocation.Editor) { + if (this._target.object === TerminalLocation.Editor) { return theme.getColor(editorBackground); } const location = this._viewDescriptorService.getViewLocationById(TERMINAL_VIEW_ID)!; diff --git a/src/vs/workbench/contrib/terminalContrib/stickyScroll/browser/terminalStickyScrollContribution.ts b/src/vs/workbench/contrib/terminalContrib/stickyScroll/browser/terminalStickyScrollContribution.ts index 5a1f934bcb894..c35f5b789f880 100644 --- a/src/vs/workbench/contrib/terminalContrib/stickyScroll/browser/terminalStickyScrollContribution.ts +++ b/src/vs/workbench/contrib/terminalContrib/stickyScroll/browser/terminalStickyScrollContribution.ts @@ -103,7 +103,7 @@ export class TerminalStickyScrollContribution extends Disposable implements ITer TerminalStickyScrollOverlay, this._instance, this._xterm!, - this._instantiationService.createInstance(TerminalInstanceColorProvider, this._instance), + this._instantiationService.createInstance(TerminalInstanceColorProvider, this._instance.targetRef), this._instance.capabilities.get(TerminalCapability.CommandDetection)!, xtermCtorEventually ); From 1cb112d243ea61dfef93ab0e6ea4e033445a4d03 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 27 Sep 2024 09:12:46 -0700 Subject: [PATCH 2/3] Fix XtermTerminal leaking --- .../browser/terminal.commandGuide.contribution.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/commandGuide/browser/terminal.commandGuide.contribution.ts b/src/vs/workbench/contrib/terminalContrib/commandGuide/browser/terminal.commandGuide.contribution.ts index 7051364ea44ff..ec3adb8093704 100644 --- a/src/vs/workbench/contrib/terminalContrib/commandGuide/browser/terminal.commandGuide.contribution.ts +++ b/src/vs/workbench/contrib/terminalContrib/commandGuide/browser/terminal.commandGuide.contribution.ts @@ -42,11 +42,11 @@ class TerminalCommandGuideContribution extends Disposable implements ITerminalCo xtermOpen(xterm: IXtermTerminal & { raw: RawXtermTerminal }): void { this._xterm = xterm; this._refreshActivatedState(); - this._configurationService.onDidChangeConfiguration(e => { + this._register(this._configurationService.onDidChangeConfiguration(e => { if (e.affectsConfiguration(TerminalCommandGuideSettingId.ShowCommandGuide)) { this._refreshActivatedState(); } - }); + })); } private _refreshActivatedState() { From 40290c9f4fb6f4e6f9bfdd72a0058b5a99836183 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Fri, 27 Sep 2024 09:29:43 -0700 Subject: [PATCH 3/3] Terminal developer contrib leaks, simplify line data option --- src/vs/base/common/lifecycle.ts | 11 -------- src/vs/base/test/common/lifecycle.test.ts | 13 +-------- .../terminal/browser/terminalInstance.ts | 8 +++--- .../terminal.developer.contribution.ts | 27 ++++++++++--------- 4 files changed, 19 insertions(+), 40 deletions(-) diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index c5a3a2230139c..4cc2d172bf4f6 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -698,17 +698,6 @@ export class ImmortalReference implements IReference { dispose(): void { /* noop */ } } -/** - * A reference that will be automatically cleared upon dispose. This is useful - * to clear out any references that could lead to leaks upon dispose. - */ -export class AutoClearingReference implements IReference { - private _object: T | undefined; - get object() { return this._object; } - constructor(object: T) { this._object = object; } - dispose(): void { this._object = undefined; } -} - export function disposeOnReturn(fn: (store: DisposableStore) => void): void { const store = new DisposableStore(); try { diff --git a/src/vs/base/test/common/lifecycle.test.ts b/src/vs/base/test/common/lifecycle.test.ts index 5fc771384d7e0..fd2fa4ef562c3 100644 --- a/src/vs/base/test/common/lifecycle.test.ts +++ b/src/vs/base/test/common/lifecycle.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import { Emitter } from '../../common/event.js'; -import { AutoClearingReference, DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, toDisposable } from '../../common/lifecycle.js'; +import { DisposableStore, dispose, IDisposable, markAsSingleton, ReferenceCollection, SafeDisposable, toDisposable } from '../../common/lifecycle.js'; import { ensureNoDisposablesAreLeakedInTestSuite, throwIfDisposablesAreLeaked } from './utils.js'; class Disposable implements IDisposable { @@ -263,17 +263,6 @@ suite('Reference Collection', () => { }); }); -suite('AutoClearingReference', () => { - ensureNoDisposablesAreLeakedInTestSuite(); - - test('simple', () => { - const ref = new AutoClearingReference(1); - assert.strictEqual(ref.object, 1); - ref.dispose(); - assert.strictEqual(ref.object, undefined); - }); -}); - function assertThrows(fn: () => void, test: (error: any) => void) { try { fn(); diff --git a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts index 78bbf8481ba27..c821d7ccf4888 100644 --- a/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts +++ b/src/vs/workbench/contrib/terminal/browser/terminalInstance.ts @@ -17,7 +17,7 @@ import { ErrorNoTelemetry, onUnexpectedError } from '../../../../base/common/err import { Emitter, Event } from '../../../../base/common/event.js'; import { KeyCode } from '../../../../base/common/keyCodes.js'; import { ISeparator, template } from '../../../../base/common/labels.js'; -import { AutoClearingReference, Disposable, DisposableStore, IDisposable, ImmortalReference, MutableDisposable, dispose, toDisposable, type IReference } from '../../../../base/common/lifecycle.js'; +import { Disposable, DisposableStore, IDisposable, ImmortalReference, MutableDisposable, dispose, toDisposable, type IReference } from '../../../../base/common/lifecycle.js'; import { Schemas } from '../../../../base/common/network.js'; import * as path from '../../../../base/common/path.js'; import { OS, OperatingSystem, isMacintosh, isWindows } from '../../../../base/common/platform.js'; @@ -355,11 +355,9 @@ export class TerminalInstance extends Disposable implements ITerminalInstance { private readonly _onDidPaste = this._register(new Emitter()); readonly onDidPaste = this._onDidPaste.event; - private _onLineDataFirstListener: IReference = this._register(new AutoClearingReference(async () => { - const xterm = this.xterm || await this._xtermReadyPromise; - xterm.raw.loadAddon(this._lineDataEventAddon!); + private readonly _onLineData = this._register(new Emitter({ + onDidAddFirstListener: async () => (this.xterm ?? await this._xtermReadyPromise).raw.loadAddon(this._lineDataEventAddon!) })); - private readonly _onLineData = this._register(new Emitter({ onDidAddFirstListener: this._onLineDataFirstListener.object?.() })); readonly onLineData = this._onLineData.event; constructor( diff --git a/src/vs/workbench/contrib/terminalContrib/developer/browser/terminal.developer.contribution.ts b/src/vs/workbench/contrib/terminalContrib/developer/browser/terminal.developer.contribution.ts index bd9447f80013c..5ee9ea2db8d30 100644 --- a/src/vs/workbench/contrib/terminalContrib/developer/browser/terminal.developer.contribution.ts +++ b/src/vs/workbench/contrib/terminalContrib/developer/browser/terminal.developer.contribution.ts @@ -7,7 +7,7 @@ import type { Terminal } from '@xterm/xterm'; import { Delayer } from '../../../../../base/common/async.js'; import { VSBuffer } from '../../../../../base/common/buffer.js'; import { Event } from '../../../../../base/common/event.js'; -import { Disposable, DisposableStore, IDisposable, MutableDisposable, combinedDisposable, dispose } from '../../../../../base/common/lifecycle.js'; +import { Disposable, DisposableMap, DisposableStore, IDisposable, MutableDisposable, combinedDisposable, dispose } from '../../../../../base/common/lifecycle.js'; import { URI } from '../../../../../base/common/uri.js'; import './media/developer.css'; import { localize, localize2 } from '../../../../../nls.js'; @@ -256,8 +256,11 @@ class DevModeContribution extends Disposable implements ITerminalContribution { const commandDetection = this._instance.capabilities.get(TerminalCapability.CommandDetection); if (devMode) { if (commandDetection) { - const commandDecorations = new Map(); + const commandDecorations = new DisposableMap(); + const otherDisposables = new DisposableStore(); this._activeDevModeDisposables.value = combinedDisposable( + commandDecorations, + otherDisposables, // Prompt input this._instance.onDidBlur(() => this._updateDevMode()), this._instance.onDidFocus(() => this._updateDevMode()), @@ -266,17 +269,17 @@ class DevModeContribution extends Disposable implements ITerminalContribution { commandDetection.onCommandFinished(command => { const colorClass = `color-${this._currentColor}`; const decorations: IDisposable[] = []; - commandDecorations.set(command, decorations); + commandDecorations.set(command, combinedDisposable(...decorations)); if (command.promptStartMarker) { const d = this._instance.xterm!.raw?.registerDecoration({ marker: command.promptStartMarker }); if (d) { decorations.push(d); - d.onRender(e => { + otherDisposables.add(d.onRender(e => { e.textContent = 'A'; e.classList.add('xterm-sequence-decoration', 'top', 'left', colorClass); - }); + })); } } if (command.marker) { @@ -286,10 +289,10 @@ class DevModeContribution extends Disposable implements ITerminalContribution { }); if (d) { decorations.push(d); - d.onRender(e => { + otherDisposables.add(d.onRender(e => { e.textContent = 'B'; e.classList.add('xterm-sequence-decoration', 'top', 'right', colorClass); - }); + })); } } if (command.executedMarker) { @@ -299,10 +302,10 @@ class DevModeContribution extends Disposable implements ITerminalContribution { }); if (d) { decorations.push(d); - d.onRender(e => { + otherDisposables.add(d.onRender(e => { e.textContent = 'C'; e.classList.add('xterm-sequence-decoration', 'bottom', 'left', colorClass); - }); + })); } } if (command.endMarker) { @@ -311,10 +314,10 @@ class DevModeContribution extends Disposable implements ITerminalContribution { }); if (d) { decorations.push(d); - d.onRender(e => { + otherDisposables.add(d.onRender(e => { e.textContent = 'D'; e.classList.add('xterm-sequence-decoration', 'bottom', 'right', colorClass); - }); + })); } } this._currentColor = (this._currentColor + 1) % 2; @@ -325,7 +328,7 @@ class DevModeContribution extends Disposable implements ITerminalContribution { if (decorations) { dispose(decorations); } - commandDecorations.delete(c); + commandDecorations.deleteAndDispose(c); } }) );