From 79a9276e6a0cbbb8d19a720f87af3c3098a9380a Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Wed, 6 Sep 2023 15:22:30 +0200 Subject: [PATCH 1/3] Uses disposable tracking for autorun --- src/vs/base/common/lifecycle.ts | 4 ++-- src/vs/base/common/observableInternal/autorun.ts | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/vs/base/common/lifecycle.ts b/src/vs/base/common/lifecycle.ts index c044d998203..1682b7932ae 100644 --- a/src/vs/base/common/lifecycle.ts +++ b/src/vs/base/common/lifecycle.ts @@ -80,12 +80,12 @@ if (TRACK_DISPOSABLES) { }); } -function trackDisposable(x: T): T { +export function trackDisposable(x: T): T { disposableTracker?.trackDisposable(x); return x; } -function markAsDisposed(disposable: IDisposable): void { +export function markAsDisposed(disposable: IDisposable): void { disposableTracker?.markAsDisposed(disposable); } diff --git a/src/vs/base/common/observableInternal/autorun.ts b/src/vs/base/common/observableInternal/autorun.ts index 482e592d883..050e9bae1c3 100644 --- a/src/vs/base/common/observableInternal/autorun.ts +++ b/src/vs/base/common/observableInternal/autorun.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { assertFn } from 'vs/base/common/assert'; -import { DisposableStore, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; +import { DisposableStore, IDisposable, markAsDisposed, toDisposable, trackDisposable } from 'vs/base/common/lifecycle'; import { IReader, IObservable, IObserver, IChangeContext, getFunctionName } from 'vs/base/common/observableInternal/base'; import { getLogger } from 'vs/base/common/observableInternal/logging'; @@ -115,6 +115,8 @@ export class AutorunObserver implements IObserver, IReader this.changeSummary = this.createChangeSummary?.(); getLogger()?.handleAutorunCreated(this); this._runIfNeeded(); + + trackDisposable(this); } public dispose(): void { @@ -123,6 +125,8 @@ export class AutorunObserver implements IObserver, IReader o.removeObserver(this); } this.dependencies.clear(); + + markAsDisposed(this); } private _runIfNeeded() { From b506b0ce31fa74cb20063c0dc3c69a4bb1b24c72 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Wed, 6 Sep 2023 15:27:13 +0200 Subject: [PATCH 2/3] Fixes leaks --- .../test/browser/suggestWidgetModel.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vs/editor/contrib/inlineCompletions/test/browser/suggestWidgetModel.test.ts b/src/vs/editor/contrib/inlineCompletions/test/browser/suggestWidgetModel.test.ts index e46c38499f1..06cc564f3d9 100644 --- a/src/vs/editor/contrib/inlineCompletions/test/browser/suggestWidgetModel.test.ts +++ b/src/vs/editor/contrib/inlineCompletions/test/browser/suggestWidgetModel.test.ts @@ -35,8 +35,11 @@ import { InlineCompletionsController } from 'vs/editor/contrib/inlineCompletions import { autorun } from 'vs/base/common/observable'; import { setUnexpectedErrorHandler } from 'vs/base/common/errors'; import { IAudioCueService } from 'vs/platform/audioCues/browser/audioCueService'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; suite('Suggest Widget Model', () => { + ensureNoDisposablesAreLeakedInTestSuite(); + setup(() => { setUnexpectedErrorHandler(function (err) { throw err; @@ -136,7 +139,7 @@ async function withAsyncTestCodeEditorAndInlineCompletionsModel( const serviceCollection = new ServiceCollection( [ITelemetryService, NullTelemetryService], [ILogService, new NullLogService()], - [IStorageService, new InMemoryStorageService()], + [IStorageService, disposableStore.add(new InMemoryStorageService())], [IKeybindingService, new MockKeybindingService()], [IEditorWorkerService, new class extends mock() { override computeWordRanges() { @@ -166,8 +169,7 @@ async function withAsyncTestCodeEditorAndInlineCompletionsModel( if (options.provider) { const languageFeaturesService = new LanguageFeaturesService(); serviceCollection.set(ILanguageFeaturesService, languageFeaturesService); - const d = languageFeaturesService.completionProvider.register({ pattern: '**' }, options.provider); - disposableStore.add(d); + disposableStore.add(languageFeaturesService.completionProvider.register({ pattern: '**' }, options.provider)); } await withAsyncTestCodeEditor(text, { ...options, serviceCollection }, async (editor, editorViewModel, instantiationService) => { From 87f8a9fcfd70ff5d1ef7bb6521f11856ee65060f Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Wed, 6 Sep 2023 15:34:22 +0200 Subject: [PATCH 3/3] Fixes leaking tests --- .../services/abstractCodeEditorService.ts | 7 ++++++- .../services/decorationRenderOptions.test.ts | 19 +++++++++++-------- .../common/model/modelInjectedText.test.ts | 18 ++++++------------ 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/vs/editor/browser/services/abstractCodeEditorService.ts b/src/vs/editor/browser/services/abstractCodeEditorService.ts index cc0f8669a03..327a5828966 100644 --- a/src/vs/editor/browser/services/abstractCodeEditorService.ts +++ b/src/vs/editor/browser/services/abstractCodeEditorService.ts @@ -150,7 +150,7 @@ export abstract class AbstractCodeEditorService extends Disposable implements IC this._editorStyleSheets.delete(editorId); } - public registerDecorationType(description: string, key: string, options: IDecorationRenderOptions, parentTypeKey?: string, editor?: ICodeEditor): void { + public registerDecorationType(description: string, key: string, options: IDecorationRenderOptions, parentTypeKey?: string, editor?: ICodeEditor): IDisposable { let provider = this._decorationOptionProviders.get(key); if (!provider) { const styleSheet = this._getOrCreateStyleSheet(editor); @@ -169,6 +169,11 @@ export abstract class AbstractCodeEditorService extends Disposable implements IC this._onDecorationTypeRegistered.fire(key); } provider.refCount++; + return { + dispose: () => { + this.removeDecorationType(key); + } + }; } public listDecorationTypes(): string[] { diff --git a/src/vs/editor/test/browser/services/decorationRenderOptions.test.ts b/src/vs/editor/test/browser/services/decorationRenderOptions.test.ts index 68fa314066d..b8eb27e6d0f 100644 --- a/src/vs/editor/test/browser/services/decorationRenderOptions.test.ts +++ b/src/vs/editor/test/browser/services/decorationRenderOptions.test.ts @@ -6,11 +6,14 @@ import * as assert from 'assert'; import * as platform from 'vs/base/common/platform'; import { URI } from 'vs/base/common/uri'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; import { IDecorationRenderOptions } from 'vs/editor/common/editorCommon'; import { TestCodeEditorService, TestGlobalStyleSheet } from 'vs/editor/test/browser/editorTestServices'; import { TestColorTheme, TestThemeService } from 'vs/platform/theme/test/common/testThemeService'; suite('Decoration Render Options', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + const themeServiceMock = new TestThemeService(); const options: IDecorationRenderOptions = { @@ -20,12 +23,12 @@ suite('Decoration Render Options', () => { borderColor: 'yellow' }; test('register and resolve decoration type', () => { - const s = new TestCodeEditorService(themeServiceMock); - s.registerDecorationType('test', 'example', options); + const s = store.add(new TestCodeEditorService(themeServiceMock)); + store.add(s.registerDecorationType('test', 'example', options)); assert.notStrictEqual(s.resolveDecorationOptions('example', false), undefined); }); test('remove decoration type', () => { - const s = new TestCodeEditorService(themeServiceMock); + const s = store.add(new TestCodeEditorService(themeServiceMock)); s.registerDecorationType('test', 'example', options); assert.notStrictEqual(s.resolveDecorationOptions('example', false), undefined); s.removeDecorationType('example'); @@ -37,9 +40,9 @@ suite('Decoration Render Options', () => { } test('css properties', () => { - const s = new TestCodeEditorService(themeServiceMock); + const s = store.add(new TestCodeEditorService(themeServiceMock)); const styleSheet = s.globalStyleSheet; - s.registerDecorationType('test', 'example', options); + store.add(s.registerDecorationType('test', 'example', options)); const sheet = readStyleSheet(styleSheet); assert(sheet.indexOf(`{background:url('https://github.com/microsoft/vscode/blob/main/resources/linux/code.png') center center no-repeat;background-size:contain;}`) >= 0); assert(sheet.indexOf(`{background-color:red;border-color:yellow;box-sizing: border-box;}`) >= 0); @@ -54,7 +57,7 @@ suite('Decoration Render Options', () => { const themeService = new TestThemeService(new TestColorTheme({ editorBackground: '#FF0000' })); - const s = new TestCodeEditorService(themeService); + const s = store.add(new TestCodeEditorService(themeService)); const styleSheet = s.globalStyleSheet; s.registerDecorationType('test', 'example', options); assert.strictEqual(readStyleSheet(styleSheet), '.monaco-editor .ced-example-0 {background-color:#ff0000;border-color:transparent;box-sizing: border-box;}'); @@ -87,7 +90,7 @@ suite('Decoration Render Options', () => { editorBackground: '#FF0000', infoForeground: '#444444' })); - const s = new TestCodeEditorService(themeService); + const s = store.add(new TestCodeEditorService(themeService)); const styleSheet = s.globalStyleSheet; s.registerDecorationType('test', 'example', options); const expected = [ @@ -103,7 +106,7 @@ suite('Decoration Render Options', () => { }); test('css properties, gutterIconPaths', () => { - const s = new TestCodeEditorService(themeServiceMock); + const s = store.add(new TestCodeEditorService(themeServiceMock)); const styleSheet = s.globalStyleSheet; // URI, only minimal encoding diff --git a/src/vs/editor/test/common/model/modelInjectedText.test.ts b/src/vs/editor/test/common/model/modelInjectedText.test.ts index f31983f142f..72d0f9ba0a3 100644 --- a/src/vs/editor/test/common/model/modelInjectedText.test.ts +++ b/src/vs/editor/test/common/model/modelInjectedText.test.ts @@ -4,32 +4,26 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils'; import { EditOperation } from 'vs/editor/common/core/editOperation'; import { Range } from 'vs/editor/common/core/range'; -import { TextModel } from 'vs/editor/common/model/textModel'; import { InternalModelContentChangeEvent, LineInjectedText, ModelRawChange, RawContentChangedType } from 'vs/editor/common/textModelEvents'; import { createTextModel } from 'vs/editor/test/common/testTextModel'; suite('Editor Model - Injected Text Events', () => { - let thisModel: TextModel; - - setup(() => { - thisModel = createTextModel('First Line\nSecond Line'); - }); - - teardown(() => { - thisModel.dispose(); - }); + const store = ensureNoDisposablesAreLeakedInTestSuite(); test('Basic', () => { + const thisModel = store.add(createTextModel('First Line\nSecond Line')); + const recordedChanges = new Array(); - thisModel.onDidChangeContentOrInjectedText((e) => { + store.add(thisModel.onDidChangeContentOrInjectedText((e) => { const changes = (e instanceof InternalModelContentChangeEvent ? e.rawContentChangedEvent.changes : e.changes); for (const change of changes) { recordedChanges.push(mapChange(change)); } - }); + })); // Initial decoration let decorations = thisModel.deltaDecorations([], [{