Merge pull request #203915 from microsoft/tyriar/203437

Fix leak/exception and links becoming bricked within the terminal

src/vs/workbench/api/browser/mainThreadTerminalService.ts

@@ -43,13 +43,14 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
 	private readonly _quickFixProviders = new Map<string, IDisposable>();
 	private readonly _dataEventTracker = new MutableDisposable<TerminalDataEventTracker>();
 	private readonly _sendCommandEventListener = new MutableDisposable();
+
 	/**
 	 * A single shared terminal link provider for the exthost. When an ext registers a link
 	 * provider, this is registered with the terminal on the renderer side and all links are
 	 * provided through this, even from multiple ext link providers. Xterm should remove lower
 	 * priority intersecting links itself.
 	 */
-	private _linkProvider: IDisposable | undefined;
+	private _linkProvider = this._store.add(new MutableDisposable());
 
 	private _os: OperatingSystem = OS;
 
@@ -111,7 +112,6 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
 
 	public dispose(): void {
 		this._store.dispose();
-		this._linkProvider?.dispose();
 		for (const provider of this._profileProviders.values()) {
 			provider.dispose();
 		}
@@ -253,13 +253,11 @@ export class MainThreadTerminalService implements MainThreadTerminalServiceShape
 	}
 
 	public $startLinkProvider(): void {
-		this._linkProvider?.dispose();
-		this._linkProvider = this._terminalLinkProviderService.registerLinkProvider(new ExtensionTerminalLinkProvider(this._proxy));
+		this._linkProvider.value = this._terminalLinkProviderService.registerLinkProvider(new ExtensionTerminalLinkProvider(this._proxy));
 	}
 
 	public $stopLinkProvider(): void {
-		this._linkProvider?.dispose();
-		this._linkProvider = undefined;
+		this._linkProvider.clear();
 	}
 
 	public $registerProcessSupport(isSupported: boolean): void {

src/vs/workbench/contrib/terminalContrib/links/browser/terminal.links.contribution.ts

@@ -3,6 +3,7 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
+import { Event } from 'vs/base/common/event';
 import { KeyCode, KeyMod } from 'vs/base/common/keyCodes';
 import { DisposableStore } from 'vs/base/common/lifecycle';
 import { localize2 } from 'vs/nls';
@@ -50,27 +51,30 @@ class TerminalLinkContribution extends DisposableStore implements ITerminalContr
 	}
 
 	xtermReady(xterm: IXtermTerminal & { raw: RawXtermTerminal }): void {
-		const linkManager = this._instantiationService.createInstance(TerminalLinkManager, xterm.raw, this._processManager, this._instance.capabilities, this._linkResolver);
+		const linkManager = this._linkManager = this.add(this._instantiationService.createInstance(TerminalLinkManager, xterm.raw, this._processManager, this._instance.capabilities, this._linkResolver));
+
+		// Set widget manager
 		if (isTerminalProcessManager(this._processManager)) {
-			this._processManager.onProcessReady(() => {
+			const disposable = this.add(Event.once(this._processManager.onProcessReady)(() => {
 				linkManager.setWidgetManager(this._widgetManager);
-			});
+				this.delete(disposable);
+			}));
 		} else {
 			linkManager.setWidgetManager(this._widgetManager);
 		}
-		this._linkManager = this.add(linkManager);
 
 		// Attach the link provider(s) to the instance and listen for changes
 		if (!isDetachedTerminalInstance(this._instance)) {
 			for (const linkProvider of this._terminalLinkProviderService.linkProviders) {
-				this._linkManager.registerExternalLinkProvider(linkProvider.provideLinks.bind(linkProvider, this._instance));
+				linkManager.registerExternalLinkProvider(linkProvider.provideLinks.bind(linkProvider, this._instance));
 			}
-			this.add(this._terminalLinkProviderService.onDidAddLinkProvider(e => {
+			linkManager.add(this._terminalLinkProviderService.onDidAddLinkProvider(e => {
 				linkManager.registerExternalLinkProvider(e.provideLinks.bind(e, this._instance as ITerminalInstance));
 			}));
 		}
+
 		// TODO: Currently only a single link provider is supported; the one registered by the ext host
-		this.add(this._terminalLinkProviderService.onDidRemoveLinkProvider(e => {
+		linkManager.add(this._terminalLinkProviderService.onDidRemoveLinkProvider(e => {
 			linkManager.dispose();
 			this.xtermReady(xterm);
 		}));

src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkDetectorAdapter.ts

@@ -76,7 +76,7 @@ export class TerminalLinkDetectorAdapter extends Disposable implements ILinkProv
 		// Cap the maximum context on either side of the line being provided, by taking the context
 		// around the line being provided for this ensures the line the pointer is on will have
 		// links provided.
-		const maxLineContext = Math.max(this._detector.maxLinkLength / this._detector.xterm.cols);
+		const maxLineContext = Math.max(this._detector.maxLinkLength, this._detector.xterm.cols);
 		const minStartLine = Math.max(startLine - maxLineContext, 0);
 		const maxEndLine = Math.min(endLine + maxLineContext, this._detector.xterm.buffer.active.length);
 

src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkManager.ts

@@ -91,6 +91,8 @@ export class TerminalLinkManager extends DisposableStore {
 		let activeHoverDisposable: IDisposable | undefined;
 		let activeTooltipScheduler: RunOnceScheduler | undefined;
 		this.add(toDisposable(() => {
+			this._clearLinkProviders();
+			dispose(this._externalLinkProviders);
 			activeHoverDisposable?.dispose();
 			activeTooltipScheduler?.dispose();
 		}));
@@ -352,15 +354,18 @@ export class TerminalLinkManager extends DisposableStore {
 		}
 	}
 
-	registerExternalLinkProvider(provideLinks: OmitFirstArg<ITerminalExternalLinkProvider['provideLinks']>): IDisposable {
+	registerExternalLinkProvider(provideLinks: OmitFirstArg<ITerminalExternalLinkProvider['provideLinks']>): void {
+		// Avoid any leaks in case this is already disposed
+		if (this.isDisposed) {
+			return;
+		}
 		// Clear and re-register the standard link providers so they are a lower priority than the new one
 		this._clearLinkProviders();
 		const detectorId = `extension-${this._externalLinkProviders.length}`;
 		const wrappedLinkProvider = this._setupLinkDetector(detectorId, new TerminalExternalLinkDetector(detectorId, this._xterm, provideLinks), true);
 		const newLinkProvider = this._xterm.registerLinkProvider(wrappedLinkProvider);
 		this._externalLinkProviders.push(newLinkProvider);
 		this._registerStandardLinkProviders();
-		return newLinkProvider;
 	}
 
 	protected _isLinkActivationModifierDown(event: MouseEvent): boolean {

src/vs/workbench/contrib/terminalContrib/links/browser/terminalLinkResolver.ts

@@ -15,8 +15,6 @@ import { ITerminalBackend } from 'vs/platform/terminal/common/terminal';
 import { mainWindow } from 'vs/base/browser/window';
 
 export class TerminalLinkResolver implements ITerminalLinkResolver {
-	declare _serviceBrand: undefined;
-
 	// Link cache could be shared across all terminals, but that could lead to weird results when
 	// both local and remote terminals are present
 	private readonly _resolvedLinkCaches: Map<string, LinkCache> = new Map();

src/vs/workbench/contrib/terminalContrib/links/test/browser/terminalLinkManager.test.ts

@@ -99,6 +99,14 @@ suite('TerminalLinkManager', () => {
 		} as Partial<ITerminalCapabilityStore> as any, instantiationService.createInstance(TerminalLinkResolver)));
 	});
 
+	suite('registerExternalLinkProvider', () => {
+		test('should not leak disposables if the link manager is already disposed', () => {
+			linkManager.registerExternalLinkProvider(async () => undefined);
+			linkManager.dispose();
+			linkManager.registerExternalLinkProvider(async () => undefined);
+		});
+	});
+
 	suite('getLinks and open recent link', () => {
 		test('should return no links', async () => {
 			const links = await linkManager.getLinks();