fix listener leaks (#17913)

* fix listener leaks

* add null check

* simplify

* pr comments

src/sql/platform/connection/common/connectionConfig.ts

@@ -13,6 +13,7 @@ import { generateUuid } from 'vs/base/common/uuid';
 import * as nls from 'vs/nls';
 import { ConfigurationTarget, IConfigurationService } from 'vs/platform/configuration/common/configuration';
 import { deepClone } from 'vs/base/common/objects';
+import { isDisposable } from 'vs/base/common/lifecycle';
 
 export const GROUPS_CONFIG_KEY = 'datasource.connectionGroups';
 export const CONNECTIONS_CONFIG_KEY = 'datasource.connections';
@@ -99,8 +100,10 @@ export class ConnectionConfig {
 
 				// Remove the profile if already set
 				let sameProfileInList = profiles.find(value => {
-					let providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService);
+					const providerConnectionProfile = ConnectionProfile.createFromStoredProfile(value, this._capabilitiesService);
-					return matcher(providerConnectionProfile, connectionProfile);
+					const match = matcher(providerConnectionProfile, connectionProfile);
+					providerConnectionProfile.dispose();
+					return match;
 				});
 				if (sameProfileInList) {
 					let profileIndex = profiles.findIndex(value => value === sameProfileInList);
@@ -111,7 +114,14 @@ export class ConnectionConfig {
 					profiles.push(newProfile);
 				}
 
-				return this.configurationService.updateValue(CONNECTIONS_CONFIG_KEY, profiles, ConfigurationTarget.USER).then(() => connectionProfile);
+				return this.configurationService.updateValue(CONNECTIONS_CONFIG_KEY, profiles, ConfigurationTarget.USER).then(() => {
+					profiles.forEach(p => {
+						if (isDisposable(p)) {
+							p.dispose();
+						}
+					});
+					return connectionProfile;
+				});
 			});
 		} else {
 			return Promise.resolve(profile);

src/sql/platform/connection/common/connectionStore.ts

@@ -11,6 +11,7 @@ import { ConnectionProfile } from 'sql/platform/connection/common/connectionProf
 import { ConnectionProfileGroup, IConnectionProfileGroup } from 'sql/platform/connection/common/connectionProfileGroup';
 import { IConnectionProfile, ProfileMatcher } from 'sql/platform/connection/common/interfaces';
 import { ICredentialsService } from 'sql/platform/credentials/common/credentialsService';
+import { isDisposable } from 'vs/base/common/lifecycle';
 import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
 import { IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage';
 
@@ -112,6 +113,9 @@ export class ConnectionStore {
 		// Add the profile to the saved list, taking care to clear out the password field if necessary
 		const savedProfile = forceWritePlaintextPassword ? profile : this.getProfileWithoutPassword(profile);
 		const savedConnectionProfile = await this.saveProfileToConfig(savedProfile, matcher);
+		if (isDisposable(savedProfile)) {
+			savedProfile.dispose();
+		}
 		profile.groupId = savedConnectionProfile.groupId;
 		profile.id = savedConnectionProfile.id;
 		// Only save if we successfully added the profile
@@ -177,9 +181,9 @@ export class ConnectionStore {
 
 	public getProfileWithoutPassword(conn: IConnectionProfile): ConnectionProfile {
 		let savedConn = ConnectionProfile.fromIConnectionProfile(this.capabilitiesService, conn);
-		savedConn = savedConn.withoutPassword();
+		let newSavedConn = savedConn.withoutPassword();
-
+		savedConn.dispose();
-		return savedConn;
+		return newSavedConn;
 	}
 
 	/**
@@ -225,7 +229,9 @@ export class ConnectionStore {
 
 		list.unshift(savedProfile);
 
-		return list.filter(n => n !== undefined).map(c => c.toIConnectionProfile());
+		const profiles = list.filter(n => n !== undefined).map(c => c.toIConnectionProfile());
+		list.forEach(c => c.dispose());
+		return profiles;
 	}
 
 	private removeFromConnectionList(conn: IConnectionProfile, list: ConnectionProfile[]): IConnectionProfile[] {

src/sql/platform/connection/common/providerConnectionInfo.ts

@@ -3,7 +3,7 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { Disposable, dispose, IDisposable } from 'vs/base/common/lifecycle';
+import { Disposable } from 'vs/base/common/lifecycle';
 import { isString } from 'vs/base/common/types';
 
 import * as azdata from 'azdata';
@@ -18,8 +18,6 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	options: { [name: string]: any } = {};
 
 	private _providerName?: string;
-	private _onCapabilitiesRegisteredDisposable?: IDisposable;
-	protected _serverCapabilities?: ConnectionProviderProperties;
 	private static readonly SqlAuthentication = 'SqlLogin';
 	public static readonly ProviderPropertyName = 'providerName';
 
@@ -33,8 +31,8 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 			this.providerName = isString(model) ? model : model.providerName;
 
 			if (!isString(model)) {
-				if (model.options && this._serverCapabilities) {
+				if (model.options && this.serverCapabilities) {
-					this._serverCapabilities.connectionOptions.forEach(option => {
+					this.serverCapabilities.connectionOptions.forEach(option => {
 						let value = model.options[option.name];
 						this.options[option.name] = value;
 					});
@@ -69,26 +67,13 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 
 	public set providerName(name: string) {
 		this._providerName = name;
-		if (!this._serverCapabilities && this.capabilitiesService) {
-			let capabilities = this.capabilitiesService.getCapabilities(this.providerName);
-			if (capabilities) {
-				this._serverCapabilities = capabilities.connection;
-			}
-			if (this._onCapabilitiesRegisteredDisposable) {
-				dispose(this._onCapabilitiesRegisteredDisposable);
-			}
-			this._onCapabilitiesRegisteredDisposable = this.capabilitiesService.onCapabilitiesRegistered(e => {
-				if (e.id === this.providerName) {
-					this._serverCapabilities = e.features.connection;
-				}
-			});
-		}
 	}
 
 	public override dispose(): void {
-		if (this._onCapabilitiesRegisteredDisposable) {
+		// Notes:
-			dispose(this._onCapabilitiesRegisteredDisposable);
+		// 1. It is not recommended to add disposables to this class considering the way we create and use the connection objects,
-		}
+		//    there are places that the connection objects are not being disposed properly.
+		// 2. Remember to dispose the listeners added to this class.
 		super.dispose();
 	}
 
@@ -99,7 +84,7 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	}
 
 	public get serverCapabilities(): ConnectionProviderProperties | undefined {
-		return this._serverCapabilities;
+		return this.capabilitiesService?.getCapabilities(this.providerName)?.connection;
 	}
 
 	public get connectionName(): string {
@@ -196,11 +181,11 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 
 	public isPasswordRequired(): boolean {
 		// if there is no provider capabilities metadata assume a password is not required
-		if (!this._serverCapabilities) {
+		if (!this.serverCapabilities) {
 			return false;
 		}
 
-		let optionMetadata = this._serverCapabilities.connectionOptions.find(
+		let optionMetadata = this.serverCapabilities.connectionOptions.find(
 			option => option.specialValueType === ConnectionOptionSpecialType.password)!; // i guess we are going to assume there is a password field
 		let isPasswordRequired = optionMetadata.isRequired;
 		if (this.providerName === Constants.mssqlProviderName) {
@@ -224,8 +209,8 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	 */
 	public getOptionsKey(): string {
 		let idNames = [];
-		if (this._serverCapabilities) {
+		if (this.serverCapabilities) {
-			idNames = this._serverCapabilities.connectionOptions.map(o => {
+			idNames = this.serverCapabilities.connectionOptions.map(o => {
 				if ((o.specialValueType || o.isIdentity)
 					&& o.specialValueType !== ConnectionOptionSpecialType.password
 					&& o.specialValueType !== ConnectionOptionSpecialType.connectionName) {
@@ -270,8 +255,8 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	}
 
 	public getSpecialTypeOptionName(type: string): string | undefined {
-		if (this._serverCapabilities) {
+		if (this.serverCapabilities) {
-			let optionMetadata = this._serverCapabilities.connectionOptions.find(o => o.specialValueType === type);
+			let optionMetadata = this.serverCapabilities.connectionOptions.find(o => o.specialValueType === type);
 			return !!optionMetadata ? optionMetadata.name : undefined;
 		} else {
 			return type.toString();
@@ -286,7 +271,7 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	}
 
 	public get authenticationTypeDisplayName(): string {
-		let optionMetadata = this._serverCapabilities ? this._serverCapabilities.connectionOptions.find(o => o.specialValueType === ConnectionOptionSpecialType.authType) : undefined;
+		let optionMetadata = this.serverCapabilities ? this.serverCapabilities.connectionOptions.find(o => o.specialValueType === ConnectionOptionSpecialType.authType) : undefined;
 		let authType = this.authenticationType;
 		let displayName: string = authType;
 
@@ -301,7 +286,7 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 	}
 
 	public getProviderOptions(): azdata.ConnectionOption[] | undefined {
-		return this._serverCapabilities?.connectionOptions;
+		return this.serverCapabilities?.connectionOptions;
 	}
 
 	public static get idSeparator(): string {
@@ -319,8 +304,8 @@ export class ProviderConnectionInfo extends Disposable implements azdata.Connect
 		parts.push(this.databaseName);
 		parts.push(this.authenticationTypeDisplayName);
 
-		if (this._serverCapabilities) {
+		if (this.serverCapabilities) {
-			this._serverCapabilities.connectionOptions.forEach(element => {
+			this.serverCapabilities.connectionOptions.forEach(element => {
 				if (element.specialValueType !== ConnectionOptionSpecialType.serverName &&
 					element.specialValueType !== ConnectionOptionSpecialType.databaseName &&
 					element.specialValueType !== ConnectionOptionSpecialType.authType &&

src/sql/workbench/contrib/objectExplorer/browser/serverTreeView.ts

@@ -276,6 +276,7 @@ export class ServerTreeView extends Disposable implements IServerTreeView {
 				if (profile) {
 					newProfile = profile;
 				}
+				groups.forEach(group => group.dispose());
 			}
 
 			const currentSelections = this._tree!.getSelection();

src/sql/workbench/services/connection/browser/connectionDialogService.ts

@@ -480,6 +480,9 @@ export class ConnectionDialogService implements IConnectionDialogService {
 			this._connectionDialog.onFillinConnectionInputs((input) => this.handleFillInConnectionInputs(input as IConnectionProfile));
 			this._connectionDialog.onResetConnection(() => this.handleProviderOnResetConnection());
 			this._connectionDialog.render();
+			this._connectionDialog.onClosed(() => {
+				this._model?.dispose();
+			});
 		}
 		this._connectionDialog.newConnectionParams = params;
 		this._connectionDialog.updateProvider(this._providerNameToDisplayNameMap[this._currentProviderType]);

src/sql/workbench/services/objectExplorer/browser/asyncServerTree.ts

@@ -8,7 +8,14 @@ import { WorkbenchAsyncDataTree } from 'vs/platform/list/browser/listService';
 import { FuzzyScore } from 'vs/base/common/filters';
 import { TreeNode } from 'sql/workbench/services/objectExplorer/common/treeNode';
 import { ConnectionProfile } from 'sql/platform/connection/common/connectionProfile';
+import { IAsyncDataTreeViewState } from 'vs/base/browser/ui/tree/asyncDataTree';
 
-export class AsyncServerTree extends WorkbenchAsyncDataTree<ConnectionProfileGroup, ServerTreeElement, FuzzyScore> { }
+export class AsyncServerTree extends WorkbenchAsyncDataTree<ConnectionProfileGroup, ServerTreeElement, FuzzyScore> {
+	override async setInput(input: ConnectionProfileGroup, viewState?: IAsyncDataTreeViewState): Promise<void> {
+		const originalInput = this.getInput();
+		await super.setInput(input, viewState);
+		originalInput?.dispose();
+	}
+}
 
 export type ServerTreeElement = ConnectionProfile | ConnectionProfileGroup | TreeNode;

src/sql/workbench/services/objectExplorer/browser/treeUpdateUtils.ts

@@ -11,7 +11,7 @@ import { IObjectExplorerService } from 'sql/workbench/services/objectExplorer/br
 import { NodeType } from 'sql/workbench/services/objectExplorer/common/nodeType';
 
 import { TreeNode } from 'sql/workbench/services/objectExplorer/common/treeNode';
-import { Disposable } from 'vs/base/common/lifecycle';
+import { Disposable, isDisposable } from 'vs/base/common/lifecycle';
 import { onUnexpectedError } from 'vs/base/common/errors';
 import { AsyncServerTree, ServerTreeElement } from 'sql/workbench/services/objectExplorer/browser/asyncServerTree';
 
@@ -125,8 +125,12 @@ export class TreeUpdateUtils {
 
 			let treeInput = TreeUpdateUtils.getTreeInput(connectionManagementService);
 			if (treeInput) {
-				if (treeInput !== tree.getInput()) {
+				const originalInput = tree.getInput();
+				if (treeInput !== originalInput) {
 					return tree.setInput(treeInput).then(async () => {
+						if (isDisposable(originalInput)) {
+							originalInput.dispose();
+						}
 						// Make sure to expand all folders that where expanded in the previous session
 						if (targetsToExpand) {
 							await tree.expandAll(targetsToExpand);