Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leak/exception and links becoming bricked within the terminal #203915

Merged
merged 6 commits into from Jan 31, 2024

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jan 31, 2024

This PR fixes the long standing issue where links suddenly break in the terminal (#184046) as well as a few leak issues, one of which could trigger an exception which would likely cause other problems.

I reproduced the primary issue by using the following code on the exthost side to simulate an extension registering and disposing a single link provider:

let i = 0;
setInterval(() => {
	const id = ++i;
	console.log(`LINKS: register new (${id})`);
	const d = this.registerLinkProvider({
		handleTerminalLink(link) {
			return null!;
		},
		provideTerminalLinks(context, token) {
			return [];
		},
	});
	setTimeout(() => {
		console.log(`LINKS: dispose old (${id})`);
		d.dispose();
	}, 5000);
}, 10000);

This is why the issue was so hard to reproduce in OSS since it happened when extensions did something that was somewhat unexpected. After putting this in place it became clear that the problem was related to the clunky re-creation of TerminalLinkManager when the extension host disposes of all of its link providers here:

this.add(this._terminalLinkProviderService.onDidRemoveLinkProvider(e => {
linkManager.dispose();
this.xtermReady(xterm);
}));

Going deeper I found that xterm.js is holding onto disposed link adapters that were registered here:

this._linkProvidersDisposables.push(this._xterm.registerLinkProvider(p));

const newLinkProvider = this._xterm.registerLinkProvider(wrappedLinkProvider);

So that fix was a matter of correctly disposing them when the link manager was replaced.

The TerminalLinkManager disposal is still in place and I noticed that when that happens it causes links to disappear, so it could be related to another issue where link underlines flicker. This is deferred though as this PR is complicated enough and that would need a new upstream API.

Fixes #184046
Fixes #203437

@Tyriar Tyriar added this to the February 2024 milestone Jan 31, 2024
@Tyriar Tyriar self-assigned this Jan 31, 2024
@Tyriar Tyriar enabled auto-merge January 31, 2024 17:30
Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

👏🏼

@Tyriar Tyriar merged commit f7b26ac into main Jan 31, 2024
7 checks passed
@Tyriar Tyriar deleted the tyriar/203437 branch January 31, 2024 18:31
@a8t
Copy link

a8t commented Feb 9, 2024

omg thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants