Introduce DisposableStore

Fixes #74242

Our current usage of dispoable arrays can leak disposables (see #74242 for details). This change introduces a new class called `DisposableStore` that can be used mostly as a drop in replacement for an array of disposables but will not leak disposbles if it is disposed

`DisposableStore` was extracted from the existing `Dispoable` class, which already implements this pattern. However `Disposable` is intended to be used as a base class while `DispoableStore` is a value class.

In addition, this change hides the `toDispose` / `_toDipose` members of `Disposable` as direct write access to the disposable array also allows for leaks (and breaks encapsulation)

src/vs/base/common/lifecycle.ts

@@ -42,12 +42,9 @@ export function toDisposable(fn: () => void): IDisposable {
 	return { dispose() { fn(); } };
 }
 
-export abstract class Disposable implements IDisposable {
 
-	static None = Object.freeze<IDisposable>({ dispose() { } });
-
-	protected _toDispose: IDisposable[] = [];
-	protected get toDispose(): IDisposable[] { return this._toDispose; }
+export class DisposableStore implements IDisposable {
+	private _toDispose: IDisposable[] = [];
 
 	private _lifecycle_disposable_isDisposed = false;
 
@@ -56,7 +53,7 @@ export abstract class Disposable implements IDisposable {
 		this._toDispose = dispose(this._toDispose);
 	}
 
-	protected _register<T extends IDisposable>(t: T): T {
+	public push<T extends IDisposable>(t: T): T {
 		if (this._lifecycle_disposable_isDisposed) {
 			console.warn('Registering disposable on object that has already been disposed.');
 			t.dispose();
@@ -68,6 +65,21 @@ export abstract class Disposable implements IDisposable {
 	}
 }
 
+export abstract class Disposable implements IDisposable {
+
+	static None = Object.freeze<IDisposable>({ dispose() { } });
+
+	private readonly _store = new DisposableStore();
+
+	public dispose(): void {
+		this._store.dispose();
+	}
+
+	protected _register<T extends IDisposable>(t: T): T {
+		return this._store.push(t);
+	}
+}
+
 export interface IReference<T> extends IDisposable {
 	readonly object: T;
 }

src/vs/workbench/api/electron-browser/mainThreadWebview.ts

@@ -64,12 +64,12 @@ export class MainThreadWebviews extends Disposable implements MainThreadWebviews
 		super();
 
 		this._proxy = context.getProxy(ExtHostContext.ExtHostWebviews);
-		_editorService.onDidActiveEditorChange(this.onActiveEditorChanged, this, this._toDispose);
-		_editorService.onDidVisibleEditorsChange(this.onVisibleEditorsChanged, this, this._toDispose);
+		this._register(_editorService.onDidActiveEditorChange(this.onActiveEditorChanged, this));
+		this._register(_editorService.onDidVisibleEditorsChange(this.onVisibleEditorsChanged, this));
 
 		// This reviver's only job is to activate webview extensions
 		// This should trigger the real reviver to be registered from the extension host side.
-		this._toDispose.push(_webviewService.registerReviver({
+		this._register(_webviewService.registerReviver({
 			canRevive: (webview) => {
 				const viewType = webview.state.viewType;
 				if (viewType) {
@@ -80,9 +80,9 @@ export class MainThreadWebviews extends Disposable implements MainThreadWebviews
 			reviveWebview: () => { throw new Error('not implemented'); }
 		}));
 
-		lifecycleService.onBeforeShutdown(e => {
+		this._register(lifecycleService.onBeforeShutdown(e => {
 			e.veto(this._onBeforeShutdown());
-		}, this, this._toDispose);
+		}, this));
 	}
 
 	public $createWebviewPanel(

src/vs/workbench/browser/parts/editor/noTabsTitleControl.ts

@@ -47,7 +47,7 @@ export class NoTabsTitleControl extends TitleControl {
 		// Breadcrumbs
 		this.createBreadcrumbsControl(labelContainer, { showFileIcons: false, showSymbolIcons: true, showDecorationColors: false, breadcrumbsBackground: () => Color.transparent });
 		toggleClass(this.titleContainer, 'breadcrumbs', Boolean(this.breadcrumbsControl));
-		this.toDispose.push({ dispose: () => removeClass(this.titleContainer, 'breadcrumbs') }); // import to remove because the container is a shared dom node
+		this._register({ dispose: () => removeClass(this.titleContainer, 'breadcrumbs') }); // import to remove because the container is a shared dom node
 
 		// Right Actions Container
 		const actionsContainer = document.createElement('div');

src/vs/workbench/browser/parts/editor/resourceViewer.ts

@@ -15,7 +15,7 @@ import { clamp } from 'vs/base/common/numbers';
 import { Themable } from 'vs/workbench/common/theme';
 import { IStatusbarItem } from 'vs/workbench/browser/parts/statusbar/statusbar';
 import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
-import { IDisposable, Disposable, combinedDisposable, toDisposable, dispose } from 'vs/base/common/lifecycle';
+import { IDisposable, Disposable, toDisposable, DisposableStore } from 'vs/base/common/lifecycle';
 import { IThemeService } from 'vs/platform/theme/common/themeService';
 import { Action } from 'vs/base/common/actions';
 import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
@@ -160,7 +160,7 @@ class LargeImageView {
 
 		DOM.clearNode(container);
 
-		const disposables: IDisposable[] = [];
+		const disposables = new DisposableStore();
 
 		const label = document.createElement('p');
 		label.textContent = nls.localize('largeImageError', "The image is not displayed in the editor because it is too large ({0}).", size);
@@ -175,7 +175,7 @@ class LargeImageView {
 			disposables.push(DOM.addDisposableListener(link, DOM.EventType.CLICK, () => openExternal(descriptor.resource)));
 		}
 
-		return combinedDisposable(disposables);
+		return disposables;
 	}
 }
 
@@ -212,7 +212,7 @@ class FileSeemsBinaryFileView {
 
 		DOM.clearNode(container);
 
-		const disposables: IDisposable[] = [];
+		const disposables = new DisposableStore();
 
 		const label = document.createElement('p');
 		label.textContent = nls.localize('nativeBinaryError', "The file is not displayed in the editor because it is either binary or uses an unsupported text encoding.");
@@ -228,7 +228,7 @@ class FileSeemsBinaryFileView {
 
 		scrollbar.scanDomNode();
 
-		return combinedDisposable(disposables);
+		return disposables;
 	}
 }
 
@@ -363,11 +363,11 @@ class InlineImageView {
 		scrollbar: DomScrollableElement,
 		delegate: ResourceViewerDelegate
 	) {
-		const disposables: IDisposable[] = [];
+		const disposables = new DisposableStore();
 
 		const context: ResourceViewerContext = {
 			layout(dimension: DOM.Dimension) { },
-			dispose: () => dispose(disposables)
+			dispose: () => disposables.dispose()
 		};
 
 		const cacheKey = `${descriptor.resource.toString()}:${descriptor.etag}`;

src/vs/workbench/contrib/comments/browser/commentFormActions.ts

@@ -6,24 +6,25 @@
 import * as DOM from 'vs/base/browser/dom';
 import { Button } from 'vs/base/browser/ui/button/button';
 import { IAction } from 'vs/base/common/actions';
-import { Disposable, dispose } from 'vs/base/common/lifecycle';
+import { DisposableStore } from 'vs/base/common/lifecycle';
 import { IMenu } from 'vs/platform/actions/common/actions';
 import { attachButtonStyler } from 'vs/platform/theme/common/styler';
 import { IThemeService } from 'vs/platform/theme/common/themeService';
 
-export class CommentFormActions extends Disposable {
+export class CommentFormActions {
 	private _buttonElements: HTMLElement[] = [];
+	private _toDispose = new DisposableStore();
 
 	constructor(
 		private container: HTMLElement,
 		private actionHandler: (action: IAction) => void,
 		private themeService: IThemeService
-	) {
-		super();
-	}
+	) { }
 
 	setActions(menu: IMenu) {
-		dispose(this._toDispose);
+		this._toDispose.dispose();
+		this._toDispose = new DisposableStore();
+
 		this._buttonElements.forEach(b => DOM.removeNode(b));
 
 		const groups = menu.getActions({ shouldForwardArgs: true });

src/vs/workbench/contrib/comments/browser/commentNode.ts

@@ -178,8 +178,8 @@ export class CommentNode extends Disposable {
 
 		let commentMenus = this.commentService.getCommentMenus(this.owner);
 		const menu = commentMenus.getCommentTitleActions(this.comment, this._contextKeyService);
-		this._toDispose.push(menu);
-		this._toDispose.push(menu.onDidChange(e => {
+		this._register(menu);
+		this._register(menu.onDidChange(e => {
 			const contributedActions = menu.getActions({ shouldForwardArgs: true }).reduce((r, [, actions]) => [...r, ...actions], <MenuItemAction[]>[]);
 			this.toolbar.setActions(contributedActions);
 		}));
@@ -217,7 +217,7 @@ export class CommentNode extends Disposable {
 
 			this.registerActionBarListeners(this._actionsToolbarContainer);
 			this.toolbar.setActions(actions, [])();
-			this._toDispose.push(this.toolbar);
+			this._register(this.toolbar);
 		}
 	}
 
@@ -354,7 +354,7 @@ export class CommentNode extends Disposable {
 				return this.actionViewItemProvider(action as Action);
 			}
 		});
-		this._toDispose.push(this._reactionsActionBar);
+		this._register(this._reactionsActionBar);
 
 		this.comment.commentReactions!.map(reaction => {
 			let action = new ReactionAction(`reaction.${reaction.label}`, `${reaction.label}`, reaction.hasReacted && reaction.canEdit ? 'active' : '', reaction.canEdit, async () => {
@@ -444,8 +444,8 @@ export class CommentNode extends Disposable {
 			}));
 		}
 
-		this._toDispose.push(this._commentEditor);
-		this._toDispose.push(this._commentEditorModel);
+		this._register(this._commentEditor);
+		this._register(this._commentEditorModel);
 	}
 
 	private removeCommentEditor() {
@@ -561,8 +561,8 @@ export class CommentNode extends Disposable {
 		const menus = this.commentService.getCommentMenus(this.owner);
 		const menu = menus.getCommentActions(this.comment, this._contextKeyService);
 
-		this._toDispose.push(menu);
-		this._toDispose.push(menu.onDidChange(() => {
+		this._register(menu);
+		this._register(menu.onDidChange(() => {
 			this._commentFormActions.setActions(menu);
 		}));
 
@@ -599,17 +599,17 @@ export class CommentNode extends Disposable {
 
 		const cancelEditButton = new Button(formActions);
 		cancelEditButton.label = nls.localize('label.cancel', "Cancel");
-		this._toDispose.push(attachButtonStyler(cancelEditButton, this.themeService));
+		this._register(attachButtonStyler(cancelEditButton, this.themeService));
 
-		this._toDispose.push(cancelEditButton.onDidClick(_ => {
+		this._register(cancelEditButton.onDidClick(_ => {
 			this.removeCommentEditor();
 		}));
 
 		this._updateCommentButton = new Button(formActions);
 		this._updateCommentButton.label = UPDATE_COMMENT_LABEL;
-		this._toDispose.push(attachButtonStyler(this._updateCommentButton, this.themeService));
+		this._register(attachButtonStyler(this._updateCommentButton, this.themeService));
 
-		this._toDispose.push(this._updateCommentButton.onDidClick(_ => {
+		this._register(this._updateCommentButton.onDidClick(_ => {
 			this.editComment();
 		}));
 
@@ -622,21 +622,21 @@ export class CommentNode extends Disposable {
 	}
 
 	private registerActionBarListeners(actionsContainer: HTMLElement): void {
-		this._toDispose.push(dom.addDisposableListener(this._domNode, 'mouseenter', () => {
+		this._register(dom.addDisposableListener(this._domNode, 'mouseenter', () => {
 			actionsContainer.classList.remove('hidden');
 		}));
 
-		this._toDispose.push(dom.addDisposableListener(this._domNode, 'focus', () => {
+		this._register(dom.addDisposableListener(this._domNode, 'focus', () => {
 			actionsContainer.classList.remove('hidden');
 		}));
 
-		this._toDispose.push(dom.addDisposableListener(this._domNode, 'mouseleave', () => {
+		this._register(dom.addDisposableListener(this._domNode, 'mouseleave', () => {
 			if (!this._domNode.contains(document.activeElement)) {
 				actionsContainer.classList.add('hidden');
 			}
 		}));
 
-		this._toDispose.push(dom.addDisposableListener(this._domNode, 'focusout', (e: FocusEvent) => {
+		this._register(dom.addDisposableListener(this._domNode, 'focusout', (e: FocusEvent) => {
 			if (!this._domNode.contains((<HTMLElement>e.relatedTarget))) {
 				actionsContainer.classList.add('hidden');
 
@@ -709,8 +709,4 @@ export class CommentNode extends Disposable {
 			}, 3000);
 		}
 	}
-
-	dispose() {
-		this._toDispose.forEach(disposeable => disposeable.dispose());
-	}
 }

src/vs/workbench/contrib/debug/browser/debugActionViewItems.ts

@@ -199,9 +199,9 @@ export class FocusSessionActionViewItem extends SelectActionViewItem {
 	) {
 		super(null, action, [], -1, contextViewService, { ariaLabel: nls.localize('debugSession', 'Debug Session') });
 
-		this.toDispose.push(attachSelectBoxStyler(this.selectBox, themeService));
+		this._register(attachSelectBoxStyler(this.selectBox, themeService));
 
-		this.toDispose.push(this.debugService.getViewModel().onDidFocusSession(() => {
+		this._register(this.debugService.getViewModel().onDidFocusSession(() => {
 			const session = this.debugService.getViewModel().focusedSession;
 			if (session) {
 				const index = this.getSessions().indexOf(session);
@@ -209,8 +209,8 @@ export class FocusSessionActionViewItem extends SelectActionViewItem {
 			}
 		}));
 
-		this.toDispose.push(this.debugService.onDidNewSession(() => this.update()));
-		this.toDispose.push(this.debugService.onDidEndSession(() => this.update()));
+		this._register(this.debugService.onDidNewSession(() => this.update()));
+		this._register(this.debugService.onDidEndSession(() => this.update()));
 
 		this.update();
 	}

src/vs/workbench/contrib/debug/browser/debugViewlet.ts

@@ -112,7 +112,7 @@ export class DebugViewlet extends ViewContainerViewlet {
 
 		if (!this.debugToolBarMenu) {
 			this.debugToolBarMenu = this.menuService.createMenu(MenuId.DebugToolBar, this.contextKeyService);
-			this.toDispose.push(this.debugToolBarMenu);
+			this._register(this.debugToolBarMenu);
 		}
 		return DebugToolBar.getActions(this.debugToolBarMenu, this.debugService, this.instantiationService);
 	}

src/vs/workbench/contrib/debug/browser/repl.ts

@@ -381,7 +381,7 @@ export class Repl extends Panel implements IPrivateReplService, IHistoryNavigati
 				supportDynamicHeights: true
 			}) as WorkbenchAsyncDataTree<IDebugSession, IReplElement, FuzzyScore>;
 
-		this.toDispose.push(this.tree.onContextMenu(e => this.onContextMenu(e)));
+		this._register(this.tree.onContextMenu(e => this.onContextMenu(e)));
 		// Make sure to select the session if debugging is already active
 		this.selectSession();
 		this.styleElement = dom.createStyleSheet(this.container);

src/vs/workbench/contrib/feedback/electron-browser/feedbackStatusbarItem.ts

@@ -97,14 +97,14 @@ export class FeedbackStatusbarItem extends Themable implements IStatusbarItem {
 		this.container = element;
 
 		// Prevent showing dropdown on anything but left click
-		this.toDispose.push(addDisposableListener(this.container, 'mousedown', (e: MouseEvent) => {
+		this._register(addDisposableListener(this.container, 'mousedown', (e: MouseEvent) => {
 			if (e.button !== 0) {
 				EventHelper.stop(e, true);
 			}
 		}, true));
 
 		// Offer context menu to hide status bar entry
-		this.toDispose.push(addDisposableListener(this.container, 'contextmenu', e => {
+		this._register(addDisposableListener(this.container, 'contextmenu', e => {
 			EventHelper.stop(e, true);
 
 			this.contextMenuService.showContextMenu({

src/vs/workbench/contrib/markers/browser/markersPanel.ts

@@ -348,7 +348,7 @@ export class MarkersPanel extends Panel implements IMarkerFilterController {
 			}
 		}));
 
-		this.tree.onContextMenu(this.onContextMenu, this, this._toDispose);
+		this._register(this.tree.onContextMenu(this.onContextMenu, this));
 
 		this._register(this.configurationService.onDidChangeConfiguration(e => {
 			if (this.filterAction.useFilesExclude && e.affectsConfiguration('files.exclude')) {

src/vs/workbench/contrib/output/browser/outputActions.ts

@@ -141,10 +141,10 @@ export class SwitchOutputActionViewItem extends SelectActionViewItem {
 		super(null, action, [], 0, contextViewService, { ariaLabel: nls.localize('outputChannels', 'Output Channels.') });
 
 		let outputChannelRegistry = Registry.as<IOutputChannelRegistry>(OutputExt.OutputChannels);
-		this.toDispose.push(outputChannelRegistry.onDidRegisterChannel(() => this.updateOtions()));
-		this.toDispose.push(outputChannelRegistry.onDidRemoveChannel(() => this.updateOtions()));
-		this.toDispose.push(this.outputService.onActiveOutputChannel(() => this.updateOtions()));
-		this.toDispose.push(attachSelectBoxStyler(this.selectBox, themeService));
+		this._register(outputChannelRegistry.onDidRegisterChannel(() => this.updateOtions()));
+		this._register(outputChannelRegistry.onDidRemoveChannel(() => this.updateOtions()));
+		this._register(this.outputService.onActiveOutputChannel(() => this.updateOtions()));
+		this._register(attachSelectBoxStyler(this.selectBox, themeService));
 
 		this.updateOtions();
 	}

src/vs/workbench/contrib/scm/browser/scmViewlet.ts

@@ -1106,15 +1106,15 @@ export class SCMViewlet extends ViewContainerViewlet implements IViewModel {
 		super(VIEWLET_ID, SCMViewlet.STATE_KEY, true, configurationService, layoutService, telemetryService, storageService, instantiationService, themeService, contextMenuService, extensionService, contextService);
 
 		this.menus = instantiationService.createInstance(SCMMenus, undefined);
-		this.menus.onDidChangeTitle(this.updateTitleArea, this, this.toDispose);
+		this._register(this.menus.onDidChangeTitle(this.updateTitleArea, this));
 
 		this.message = $('.empty-message', { tabIndex: 0 }, localize('no open repo', "No source control providers registered."));
 
-		configurationService.onDidChangeConfiguration(e => {
+		this._register(configurationService.onDidChangeConfiguration(e => {
 			if (e.affectsConfiguration('scm.alwaysShowProviders')) {
 				this.onDidChangeRepositories();
 			}
-		}, this.toDispose);
+		}));
 	}
 
 	create(parent: HTMLElement): void {
@@ -1124,8 +1124,8 @@ export class SCMViewlet extends ViewContainerViewlet implements IViewModel {
 		addClasses(parent, 'scm-viewlet', 'empty');
 		append(parent, this.message);
 
-		this.scmService.onDidAddRepository(this.onDidAddRepository, this, this.toDispose);
-		this.scmService.onDidRemoveRepository(this.onDidRemoveRepository, this, this.toDispose);
+		this._register(this.scmService.onDidAddRepository(this.onDidAddRepository, this));
+		this._register(this.scmService.onDidRemoveRepository(this.onDidRemoveRepository, this));
 		this.scmService.repositories.forEach(r => this.onDidAddRepository(r));
 	}
 

src/vs/workbench/contrib/webview/electron-browser/webviewElement.ts

@@ -509,7 +509,7 @@ export class WebviewElement extends Disposable implements Webview {
 		}
 
 		this.style(themeService.getTheme());
-		themeService.onThemeChange(this.style, this, this._toDispose);
+		this._register(themeService.onThemeChange(this.style, this));
 	}
 
 	public mountTo(parent: HTMLElement) {