Skip to content

Conversation

@SimonSiefke
Copy link
Contributor

@SimonSiefke SimonSiefke commented Dec 2, 2025

Fixes a memory leak in composite bar.

A bit simplified the function looks like this:

private updateCompositeSwitcher(donotTrigger?: boolean): void {
  if (totalComposites === compositesToShow.length && this.compositeOverflowAction) {
	this.compositeOverflowAction.dispose();
	this.compositeOverflowAction = undefined;  // item is still registered in this._store
	this.compositeOverflowActionViewItem.dispose();
	this.compositeOverflowActionViewItem = undefined;
}

if (totalComposites > compositesToShow.length && !this.compositeOverflowAction) {
	this.compositeOverflowAction = this._register(this.instantiationService.createInstance(CompositeOverflowActivityAction, () => {
		this.compositeOverflowActionViewItem?.showMenu();
	}));
	this.compositeOverflowActionViewItem = this._register(this.instantiationService.createInstance(
		CompositeOverflowActivityActionViewItem
	));
  }
}

Since the updateCompositeSwitcher can run many times, it can call this._register many times, registering the compositeOverflowAction and the compositeOverflowActionViewItem disposables to this._store each time.

Change

The change is making sure to remove compositeOverflowAction and compositeOverflowActionViewItem from this._store when disposing it and setting it to undefined.

private updateCompositeSwitcher(donotTrigger?: boolean): void {
  if (totalComposites === compositesToShow.length && this.compositeOverflowAction) {
	this._store.delete(this.compositeOverflowAction); // Ensure to remove disposable from this._store
	this.compositeOverflowAction.dispose();
	this.compositeOverflowAction = undefined;

	if (this.compositeOverflowActionViewItem) {
		this._store.delete(this.compositeOverflowActionViewItem);
		this.compositeOverflowActionViewItem.dispose();
	}
}

Before

When creating and closing a terminal 197 times, the number of functions related to CompositeOverflowActivityItem grows by one each time:

terminal create

After

No more memory leak related to composite bar is detected:
terminal create

@vs-code-engineering
Copy link

vs-code-engineering bot commented Dec 2, 2025

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@bpasero

Matched files:

  • src/vs/workbench/browser/parts/compositeBar.ts

@benibenj
Copy link
Contributor

benibenj commented Dec 3, 2025

Thank you very much for looking into this and creating a PR! I think mutable disposables would be very handy in this case. I created a PR against your branch, hope you are fine with that approach. SimonSiefke#60

@benibenj benibenj enabled auto-merge (squash) December 4, 2025 16:10
@vs-code-engineering vs-code-engineering bot added this to the November 2025 milestone Dec 4, 2025
@benibenj benibenj merged commit eed1cb5 into microsoft:main Dec 4, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants