From ea0b52304c794824bc443aa04e5b202299bcd7c0 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 31 May 2026 05:36:16 -0700 Subject: [PATCH] perf: clean up destroyed browser guest policies (#4132) --- src/main/browser/browser-manager-grab.test.ts | 6 ++-- src/main/browser/browser-manager.test.ts | 30 ++++++++++++++++ src/main/browser/browser-manager.ts | 35 ++++++++++++------- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/src/main/browser/browser-manager-grab.test.ts b/src/main/browser/browser-manager-grab.test.ts index b3c16e08165..739dda6db52 100644 --- a/src/main/browser/browser-manager-grab.test.ts +++ b/src/main/browser/browser-manager-grab.test.ts @@ -817,9 +817,9 @@ describe('browserManager grab operations', () => { const promise = browserManager.awaitGrabSelection('tab-1', 'op-1', guest) // Find the destroyed handler and trigger it - const destroyedHandler = guestOnMock.mock.calls.find( - ([event]) => event === 'destroyed' - )?.[1] as (() => void) | undefined + const destroyedHandler = guestOnMock.mock.calls + .filter(([event]) => event === 'destroyed') + .at(-1)?.[1] as (() => void) | undefined expect(destroyedHandler).toBeTypeOf('function') destroyedHandler?.() diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index bbdefb15c76..93f2c1efbed 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -819,6 +819,36 @@ describe('browserManager', () => { expect(guestOnMock.mock.calls.filter(([event]) => event === 'will-redirect')).toHaveLength(1) }) + it('cleans attached guest policy state when a guest is destroyed before registration', () => { + const guest = { + id: 304, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + webContentsFromIdMock.mockReturnValue(guest) + + browserManager.attachGuestPolicies(guest as never) + + const destroyedHandler = guestOnMock.mock.calls.find( + ([event]) => event === 'destroyed' + )?.[1] as (() => void) | undefined + expect(destroyedHandler).toBeTypeOf('function') + + destroyedHandler?.() + browserManager.registerGuest({ + browserPageId: 'browser-destroyed-before-register', + webContentsId: guest.id, + rendererWebContentsId + }) + + expect(browserManager.getGuestWebContentsId('browser-destroyed-before-register')).toBeNull() + }) + it('replays a queued main-frame load failure after the guest registers', () => { const rendererSendMock = vi.fn() const guest = { diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index c7d7f958fbe..6e3b392629e 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -640,12 +640,23 @@ export class BrowserManager { guest.on('will-navigate', navigationGuard) guest.on('will-redirect', navigationGuard) guest.on('did-fail-load', didFailLoadHandler) + const handleDestroyed = (): void => { + // Why: guests can be destroyed before renderer registration. Without + // this, attach-time policy closures remain retained until app shutdown. + this.cleanupGuestPolicyAttachment(guest.id) + } + guest.on('destroyed', handleDestroyed) // Why: store cleanup so unregisterGuest can remove these listeners when the // guest surface is torn down, preventing the callbacks from preventing GC of // the underlying WebContents wrapper. this.policyCleanupByGuestId.set(guest.id, () => { disposeAntiDetection() + try { + guest.off('destroyed', handleDestroyed) + } catch { + // guest may already be destroyed + } if (!guest.isDestroyed()) { guest.off('will-navigate', navigationGuard) guest.off('will-redirect', navigationGuard) @@ -660,17 +671,20 @@ export class BrowserManager { // resolving to the live page, or stale download/popup/permission callbacks // can be delivered to the wrong session after the swap. this.tabIdByWebContentsId.delete(previousWebContentsId) + this.cleanupGuestPolicyAttachment(previousWebContentsId) + } - const policyCleanup = this.policyCleanupByGuestId.get(previousWebContentsId) + private cleanupGuestPolicyAttachment(guestWebContentsId: number): void { + const policyCleanup = this.policyCleanupByGuestId.get(guestWebContentsId) if (policyCleanup) { policyCleanup() - this.policyCleanupByGuestId.delete(previousWebContentsId) + this.policyCleanupByGuestId.delete(guestWebContentsId) } - this.policyAttachedGuestIds.delete(previousWebContentsId) - this.pendingLoadFailuresByGuestId.delete(previousWebContentsId) - this.pendingPermissionEventsByGuestId.delete(previousWebContentsId) - this.pendingPopupEventsByGuestId.delete(previousWebContentsId) - this.pendingDownloadIdsByGuestId.delete(previousWebContentsId) + this.policyAttachedGuestIds.delete(guestWebContentsId) + this.pendingLoadFailuresByGuestId.delete(guestWebContentsId) + this.pendingPermissionEventsByGuestId.delete(guestWebContentsId) + this.pendingPopupEventsByGuestId.delete(guestWebContentsId) + this.pendingDownloadIdsByGuestId.delete(guestWebContentsId) } registerGuest({ @@ -754,12 +768,7 @@ export class BrowserManager { // the underlying Chromium surface after the guest is destroyed. const guestWebContentsId = this.webContentsIdByTabId.get(browserTabId) if (guestWebContentsId !== undefined) { - const policyCleanup = this.policyCleanupByGuestId.get(guestWebContentsId) - if (policyCleanup) { - policyCleanup() - this.policyCleanupByGuestId.delete(guestWebContentsId) - } - this.policyAttachedGuestIds.delete(guestWebContentsId) + this.cleanupGuestPolicyAttachment(guestWebContentsId) } const cleanup = this.contextMenuCleanupByTabId.get(browserTabId)