fix: fix multiple window memory leaky#311415
fix: fix multiple window memory leaky#311415theanarkh wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a memory leak triggered by moving an editor to a new window by ensuring multi-window-aware timeout disposables are fully deregistered from per-window disposal tracking in the workbench window implementation.
Changes:
- Remove the per-window timeout disposable from the window’s
DisposableStorewhen the timeout is cleared/disposed.
| didClear = true; | ||
| (window as { vscodeOriginalClearTimeout?: typeof window.clearTimeout }).vscodeOriginalClearTimeout?.apply(this, [handle]); | ||
| timeoutDisposables.delete(timeoutDisposable); | ||
| disposables.delete(timeoutDisposable); |
There was a problem hiding this comment.
disposables.delete(timeoutDisposable) will call timeoutDisposable.dispose() again while it's already disposing. This currently happens to be a no-op for toDisposable, but it relies on idempotent disposal and adds unnecessary reentrancy. Consider using DisposableStore.deleteAndLeak(timeoutDisposable) here to just remove the reference (since the disposable is already being disposed).
| disposables.delete(timeoutDisposable); | |
| disposables.deleteAndLeak(timeoutDisposable); |
| didClear = true; | ||
| (window as { vscodeOriginalClearTimeout?: typeof window.clearTimeout }).vscodeOriginalClearTimeout?.apply(this, [handle]); | ||
| timeoutDisposables.delete(timeoutDisposable); | ||
| disposables.delete(timeoutDisposable); |
There was a problem hiding this comment.
This change fixes a leak by removing the timeout disposable from the per-window DisposableStore, but there is no regression test asserting that the store no longer retains the disposed timeout after it fires/gets cleared. Since this behavior is subtle and easy to regress, please add a unit test in src/vs/workbench/test/browser/window.test.ts that validates the window's disposable store count returns to the original value after the multi-window timeout completes/clears.
The follow steps will trigger the memory leaky.
move file to new window.