Skip to content

Commit 26fc91e

Browse files
Copilotrzhao271
andcommitted
Fix disposable leak in TOCTree by properly tracking stylesheet
Co-authored-by: rzhao271 <[email protected]>
1 parent b8b55a2 commit 26fc91e

File tree

2 files changed

+88
-2
lines changed

2 files changed

+88
-2
lines changed

src/vs/workbench/contrib/preferences/browser/tocTree.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,11 @@ export class TOCTree extends WorkbenchObjectTree<SettingsTreeGroupElement> {
214214
@IHoverService hoverService: IHoverService,
215215
@IInstantiationService instantiationService: IInstantiationService,
216216
) {
217-
// test open mode
217+
// Create a disposable store to track resources created before super()
218+
const disposableStore = new DisposableStore();
219+
220+
// Create the style element and track it for disposal
221+
const styleElement = domStylesheetsJs.createStyleSheet(container, undefined, disposableStore);
218222

219223
const filter = instantiationService.createInstance(SettingsTreeFilter, viewState);
220224
const options: IWorkbenchObjectTreeOptions<SettingsTreeGroupElement, void> = {
@@ -225,7 +229,7 @@ export class TOCTree extends WorkbenchObjectTree<SettingsTreeGroupElement> {
225229
return e.id;
226230
}
227231
},
228-
styleController: id => new DefaultStyleController(domStylesheetsJs.createStyleSheet(container), id),
232+
styleController: id => new DefaultStyleController(styleElement, id),
229233
accessibilityProvider: instantiationService.createInstance(SettingsAccessibilityProvider),
230234
collapseByDefault: true,
231235
horizontalScrolling: false,
@@ -245,6 +249,9 @@ export class TOCTree extends WorkbenchObjectTree<SettingsTreeGroupElement> {
245249
configurationService,
246250
);
247251

252+
// Register the disposable store to be disposed when the tree is disposed
253+
this.disposables.add(disposableStore);
254+
248255
this.style(getListStyles({
249256
listBackground: editorBackground,
250257
listFocusOutline: focusBorder,
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import assert from 'assert';
7+
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js';
8+
import { TOCTree, TOCTreeModel } from '../../browser/tocTree.js';
9+
import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
10+
import { IListService, ListService } from '../../../../../platform/list/browser/listService.js';
11+
import { IContextKeyService } from '../../../../../platform/contextkey/common/contextkey.js';
12+
import { ContextKeyService } from '../../../../../platform/contextkey/browser/contextKeyService.js';
13+
import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js';
14+
import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js';
15+
import { IHoverService } from '../../../../../platform/hover/browser/hover.js';
16+
import { NullHoverService } from '../../../../../platform/hover/test/browser/nullHoverService.js';
17+
import { ConfigurationTarget } from '../../../../../platform/configuration/common/configuration.js';
18+
import { ISettingsEditorViewState } from '../../browser/settingsTreeModels.js';
19+
import { toDisposable } from '../../../../../base/common/lifecycle.js';
20+
import { mainWindow } from '../../../../../base/browser/window.js';
21+
import { IWorkbenchEnvironmentService } from '../../../../services/environment/common/environmentService.js';
22+
import { mock } from '../../../../../base/test/common/mock.js';
23+
24+
suite('TOCTree', () => {
25+
const store = ensureNoDisposablesAreLeakedInTestSuite();
26+
let instantiationService: TestInstantiationService;
27+
let container: HTMLElement;
28+
29+
setup(() => {
30+
container = document.createElement('div');
31+
mainWindow.document.body.appendChild(container);
32+
store.add(toDisposable(() => container.remove()));
33+
34+
instantiationService = store.add(new TestInstantiationService());
35+
36+
// Set up required services
37+
instantiationService.stub(IListService, store.add(new ListService()));
38+
instantiationService.stub(IContextKeyService, store.add(instantiationService.createInstance(ContextKeyService)));
39+
instantiationService.stub(IConfigurationService, new TestConfigurationService());
40+
instantiationService.stub(IHoverService, NullHoverService);
41+
instantiationService.stub(IWorkbenchEnvironmentService, new class extends mock<IWorkbenchEnvironmentService>() {
42+
override remoteAuthority = undefined;
43+
});
44+
});
45+
46+
test('TOCTree should not leak disposables', () => {
47+
const viewState: ISettingsEditorViewState = {
48+
settingsTarget: ConfigurationTarget.USER_LOCAL,
49+
tagFilters: undefined,
50+
featureFilters: undefined,
51+
extensionFilters: undefined,
52+
idFilters: undefined,
53+
languageFilter: undefined,
54+
filterToCategory: undefined
55+
};
56+
57+
const tree = store.add(instantiationService.createInstance(TOCTree, container, viewState));
58+
assert.ok(tree, 'TOCTree should be created');
59+
60+
// Dispose the tree to verify no leaks
61+
tree.dispose();
62+
});
63+
64+
test('TOCTreeModel should be disposable', () => {
65+
const viewState: ISettingsEditorViewState = {
66+
settingsTarget: ConfigurationTarget.USER_LOCAL,
67+
tagFilters: undefined,
68+
featureFilters: undefined,
69+
extensionFilters: undefined,
70+
idFilters: undefined,
71+
languageFilter: undefined,
72+
filterToCategory: undefined
73+
};
74+
75+
const model = instantiationService.createInstance(TOCTreeModel, viewState);
76+
assert.ok(model, 'TOCTreeModel should be created');
77+
// TOCTreeModel doesn't implement IDisposable, so just verify it's created successfully
78+
});
79+
});

0 commit comments

Comments
 (0)