development/vscode
development/vscode
1
0
Fork 0
mirror of https://github.com/Microsoft/vscode synced 2024-08-27 04:49:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

Reduce memory leaks (fix #195889) (#206950)

Browse source 
* leaks - fix group listener leak in open editors

* leaks - stop patching `window.focus()`

* fix more leaks

* fix more leaks
This commit is contained in:
Benjamin Pasero 2024-03-06 14:47:45 +01:00 committed by GitHub
parent 639b71bdce
commit b4e63addcb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 49 additions and 73 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
src/vs/base/browser/dom.ts
View file

@ -921,35 +921,6 @@ export function getActiveWindow(): CodeWindow {
return (document.defaultView?.window ?? mainWindow) as CodeWindow;
}
/**
* Given an element, will attempt to pass focus() to the window it belongs
* to, depending on the options passed in:
* - force: always focus the element's window
* - otherwise: only focus the element's window if another window in the same
* workspace group has focus (when auxiliary windows are opened).
*
* @param element used to figure out the window the element belongs to
*/
export function focusWindow(element: Node, options?: { force: boolean }): void {
const window = getWindow(element);
// Force: always focus the element window
if (options?.force) {
window.focus();
}
// Not forced: only focus the element window if another
// window in the same workspace group has focus (when auxiliary
// windows are opened).
// This prevents stealing focus from another workspace window.
else {
const activeWindow = getActiveWindow();
if (activeWindow !== window && activeWindow.document.hasFocus()) {
window.focus();
}
}
}
const globalStylesheets = new Map<HTMLStyleElement /* main stylesheet */, Set<HTMLStyleElement /* aux window clones that track the main stylesheet */>>();
export function isGlobalStylesheet(node: Node): boolean {

5
src/vs/workbench/browser/part.ts
View file

@ -193,7 +193,10 @@ export abstract class MultiWindowParts<T extends IMultiWindowPart> extends Compo
registerPart(part: T): IDisposable {
this._parts.add(part);
return this._register(toDisposable(() => this.unregisterPart(part)));
return this._register(toDisposable(() => {
this.unregisterPart(part);
part = undefined!; // helps to avoid a memory leak with closures where part is captured
}));
}
protected unregisterPart(part: T): void {

2
src/vs/workbench/browser/parts/editor/editorStatus.ts
View file

@ -366,7 +366,7 @@ class EditorStatus extends Disposable {
}
private registerCommands(): void {
CommandsRegistry.registerCommand({ id: `changeEditorIndentation${this.targetWindowId}`, handler: () => this.showIndentationPicker() });
this._register(CommandsRegistry.registerCommand({ id: `changeEditorIndentation${this.targetWindowId}`, handler: () => this.showIndentationPicker() }));
}
private async showIndentationPicker(): Promise<unknown> {

61
src/vs/workbench/browser/window.ts
View file

@ -4,14 +4,14 @@
*--------------------------------------------------------------------------------------------*/
import { isSafari, setFullscreen } from 'vs/base/browser/browser';
import { addDisposableListener, EventHelper, EventType, focusWindow, getWindowById, getWindows, getWindowsCount, windowOpenNoOpener, windowOpenPopup, windowOpenWithSuccess } from 'vs/base/browser/dom';
import { addDisposableListener, EventHelper, EventType, getActiveWindow, getWindow, getWindowById, getWindows, getWindowsCount, windowOpenNoOpener, windowOpenPopup, windowOpenWithSuccess } from 'vs/base/browser/dom';
import { DomEmitter } from 'vs/base/browser/event';
import { HidDeviceData, requestHidDevice, requestSerialPort, requestUsbDevice, SerialPortData, UsbDeviceData } from 'vs/base/browser/deviceAccess';
import { timeout } from 'vs/base/common/async';
import { Event } from 'vs/base/common/event';
import { Disposable, IDisposable, dispose, toDisposable } from 'vs/base/common/lifecycle';
import { matchesScheme, Schemas } from 'vs/base/common/network';
import { isIOS, isMacintosh, isNative } from 'vs/base/common/platform';
import { isIOS, isMacintosh } from 'vs/base/common/platform';
import Severity from 'vs/base/common/severity';
import { URI } from 'vs/base/common/uri';
import { localize } from 'vs/nls';
@ -45,11 +45,7 @@ export abstract class BaseWindow extends Disposable {
) {
super();
if (isNative) {
this.enableNativeWindowFocus(targetWindow);
}
this.enableWindowFocusOnElementFocus(targetWindow);
this.enableMultiWindowAwareTimeout(targetWindow, dom);
this.registerFullScreenListeners(targetWindow.vscodeWindowId);
@ -57,39 +53,50 @@ export abstract class BaseWindow extends Disposable {
//#region focus handling in multi-window applications
protected enableNativeWindowFocus(targetWindow: CodeWindow): void {
const originalWindowFocus = targetWindow.focus.bind(targetWindow);
protected enableWindowFocusOnElementFocus(targetWindow: CodeWindow): void {
const originalFocus = targetWindow.HTMLElement.prototype.focus;
const that = this;
targetWindow.focus = function () {
originalWindowFocus();
if (
!that.environmentService.extensionTestsLocationURI && // never steal focus when running tests
!targetWindow.document.hasFocus() // skip when already having focus
) {
// Enable `window.focus()` to work in Electron by
// asking the main process to focus the window.
// https://github.com/electron/electron/issues/25578
that.hostService.focus(targetWindow);
}
};
}
protected enableWindowFocusOnElementFocus(targetWindow: CodeWindow): void {
const originalFocus = HTMLElement.prototype.focus;
targetWindow.HTMLElement.prototype.focus = function (this: HTMLElement, options?: FocusOptions | undefined): void {
// Ensure the window the element belongs to is focused
// in scenarios where auxiliary windows are present
focusWindow(this);
that.onElementFocus(getWindow(this));
// Pass to original focus() method
originalFocus.apply(this, [options]);
};
}
private onElementFocus(targetWindow: CodeWindow): void {
const activeWindow = getActiveWindow();
if (activeWindow !== targetWindow && activeWindow.document.hasFocus()) {
// Call original focus()
targetWindow.focus();
// In Electron, `window.focus()` fails to bring the window
// to the front if multiple windows exist in the same process
// group (floating windows). As such, we ask the host service
// to focus the window which can take care of bringin the
// window to the front.
//
// To minimise disruption by bringing windows to the front
// by accident, we only do this if the window is not already
// focused and the active window is not the target window
// but has focus. This is an indication that multiple windows
// are opened in the same process group while the target window
// is not focused.
if (
!this.environmentService.extensionTestsLocationURI &&
!targetWindow.document.hasFocus()
) {
this.hostService.focus(targetWindow);
}
}
}
//#endregion
//#region timeout handling in multi-window applications

2
src/vs/workbench/contrib/chat/electron-sandbox/actions/voiceChatActions.ts
View file

@ -980,7 +980,7 @@ class KeywordActivationStatusEntry extends Disposable {
) {
super();
CommandsRegistry.registerCommand(KeywordActivationStatusEntry.STATUS_COMMAND, () => this.commandService.executeCommand('workbench.action.openSettings', KEYWORD_ACTIVIATION_SETTING_ID));
this._register(CommandsRegistry.registerCommand(KeywordActivationStatusEntry.STATUS_COMMAND, () => this.commandService.executeCommand('workbench.action.openSettings', KEYWORD_ACTIVIATION_SETTING_ID)));
this.registerListeners();
this.updateStatusEntry();

6
src/vs/workbench/contrib/codeEditor/browser/emptyTextEditorHint/emptyTextEditorHint.ts
View file

@ -226,7 +226,7 @@ class EmptyTextEditorHintContentWidget implements IContentWidget {
id: 'inlineChat.hintAction',
from: 'hint'
});
void this.commandService.executeCommand(inlineChatId, { from: 'hint' });
this.commandService.executeCommand(inlineChatId, { from: 'hint' });
};
const hintHandler: IContentActionHandler = {
@ -254,7 +254,7 @@ class EmptyTextEditorHintContentWidget implements IContentWidget {
const hintPart = $('a', undefined, fragment);
hintPart.style.fontStyle = 'italic';
hintPart.style.cursor = 'pointer';
hintPart.onclick = handleClick;
this.toDispose.add(dom.addDisposableListener(hintPart, dom.EventType.CLICK, handleClick));
return hintPart;
} else {
const hintPart = $('span', undefined, fragment);
@ -272,7 +272,7 @@ class EmptyTextEditorHintContentWidget implements IContentWidget {
if (this.options.clickable) {
label.element.style.cursor = 'pointer';
label.element.onclick = handleClick;
this.toDispose.add(dom.addDisposableListener(label.element, dom.EventType.CLICK, handleClick));
}
hintElement.appendChild(after);

7
src/vs/workbench/contrib/files/browser/views/openEditorsView.ts
View file

@ -26,7 +26,7 @@ import { IListVirtualDelegate, IListRenderer, IListContextMenuEvent, IListDragAn
import { ResourceLabels, IResourceLabel } from 'vs/workbench/browser/labels';
import { ActionBar } from 'vs/base/browser/ui/actionbar/actionbar';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IDisposable, dispose } from 'vs/base/common/lifecycle';
import { DisposableMap, IDisposable, dispose } from 'vs/base/common/lifecycle';
import { MenuId, Action2, registerAction2, MenuRegistry } from 'vs/platform/actions/common/actions';
import { OpenEditorsDirtyEditorContext, OpenEditorsGroupContext, OpenEditorsReadonlyEditorContext, SAVE_ALL_LABEL, SAVE_ALL_COMMAND_ID, NEW_UNTITLED_FILE_COMMAND_ID } from 'vs/workbench/contrib/files/browser/fileConstants';
import { ResourceContextKey, MultipleEditorGroupsContext } from 'vs/workbench/common/contextkeys';
@ -118,7 +118,7 @@ export class OpenEditorsView extends ViewPane {
this.listRefreshScheduler?.schedule(this.structuralRefreshDelay);
};
const groupDisposables = new Map<number, IDisposable>();
const groupDisposables = this._register(new DisposableMap<number>());
const addGroupListener = (group: IEditorGroup) => {
const groupModelChangeListener = group.onDidModelChange(e => {
if (this.listRefreshScheduler?.isScheduled()) {
@ -156,7 +156,6 @@ export class OpenEditorsView extends ViewPane {
}
});
groupDisposables.set(group.id, groupModelChangeListener);
this._register(groupDisposables.get(group.id)!);
};
this.editorGroupService.groups.forEach(g => addGroupListener(g));
@ -167,7 +166,7 @@ export class OpenEditorsView extends ViewPane {
this._register(this.editorGroupService.onDidMoveGroup(() => updateWholeList()));
this._register(this.editorGroupService.onDidChangeActiveGroup(() => this.focusActiveEditor()));
this._register(this.editorGroupService.onDidRemoveGroup(group => {
dispose(groupDisposables.get(group.id));
groupDisposables.deleteAndDispose(group.id);
updateWholeList();
}));
}

9
src/vs/workbench/contrib/webview/browser/webviewElement.ts
View file

@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/
import { isFirefox } from 'vs/base/browser/browser';
import { addDisposableListener, EventType, focusWindow, getActiveWindow } from 'vs/base/browser/dom';
import { addDisposableListener, EventType, getActiveWindow } from 'vs/base/browser/dom';
import { IMouseWheelEvent } from 'vs/base/browser/mouseEvent';
import { promiseWithResolvers, ThrottledDelayer } from 'vs/base/common/async';
import { streamToBuffer, VSBufferReadableStream } from 'vs/base/common/buffer';
@ -803,12 +803,9 @@ export class WebviewElement extends Disposable implements IWebview, WebviewFindD
return;
}
// Ensure the window the element belongs to is focused
// in scenarios where auxiliary windows are present
focusWindow(this.element);
try {
this.element.contentWindow?.focus();
this.element.parentElement?.focus(); // this helps to move floating windows to the front if any...
this.element.contentWindow?.focus(); // ...because `contentWindow` is not able to do so
} catch {
// noop
}

1
src/vs/workbench/test/browser/window.test.ts
View file

@ -22,7 +22,6 @@ suite('Window', () => {
super(window, dom, new TestHostService(), TestEnvironmentService);
}
protected override enableNativeWindowFocus(): void { }
protected override enableWindowFocusOnElementFocus(): void { }
}