diff --git a/src/main/browser/popup-origin-bar-window.test.ts b/src/main/browser/popup-origin-bar-window.test.ts index 3e816e74b07..fedcd4dfdd0 100644 --- a/src/main/browser/popup-origin-bar-window.test.ts +++ b/src/main/browser/popup-origin-bar-window.test.ts @@ -285,6 +285,7 @@ describe('openPopupWithOriginBar', () => { it('closes the window when the popup content is destroyed, without re-closing the contents', () => { const adopted = createFakeWebContents() openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/') + const { bar } = lastViews() adopted.emit('destroyed') @@ -292,6 +293,7 @@ describe('openPopupWithOriginBar', () => { // The window's closed handler must not call close() on already-destroyed // contents — that throws in real Electron. expect(adopted.close).not.toHaveBeenCalled() + expect(bar.webContents.close).toHaveBeenCalledTimes(1) }) it('re-asserts the origin when the popup finishes loading', () => { @@ -326,11 +328,54 @@ describe('openPopupWithOriginBar', () => { const adopted = createFakeWebContents() const onClosed = vi.fn() const popup = openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/') + const { bar } = lastViews() popup.onClosed(onClosed) popup.close() expect(adopted.close).toHaveBeenCalledTimes(1) + expect(bar.webContents.close).toHaveBeenCalledTimes(1) expect(onClosed).toHaveBeenCalledTimes(1) }) + + it('releases every origin bar across repeated popup lifecycles', () => { + const cycleCount = 25 + const originBars: FakeWebContents[] = [] + + for (let cycle = 0; cycle < cycleCount; cycle += 1) { + const popup = openPopupWithOriginBar({}, `https://example.com/${cycle}`) + originBars.push(lastViews().bar.webContents) + popup.close() + } + + expect(originBars.filter((contents) => contents.close.mock.calls.length === 1)).toHaveLength( + cycleCount + ) + }) + + it('tolerates an origin bar destroyed before its window closes', () => { + const adopted = createFakeWebContents() + const onClosed = vi.fn() + const popup = openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/') + popup.onClosed(onClosed) + const { bar } = lastViews() + const barWebContents = bar.webContents + barWebContents.emit('destroyed') + Object.defineProperty(bar, 'webContents', { get: () => undefined }) + + expect(() => popup.close()).not.toThrow() + expect(adopted.close).toHaveBeenCalledTimes(1) + expect(barWebContents.close).not.toHaveBeenCalled() + expect(onClosed).toHaveBeenCalledTimes(1) + }) + + it('closes both child contents when popup preparation fails', () => { + const adopted = createFakeWebContents() + + openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/', () => false) + + expect(adopted.close).toHaveBeenCalledTimes(1) + expect(lastViews().bar.webContents.close).toHaveBeenCalledTimes(1) + expect(lastWindow().close).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/main/browser/popup-origin-bar-window.ts b/src/main/browser/popup-origin-bar-window.ts index 376c318d1ae..cfbe3ef3d3b 100644 --- a/src/main/browser/popup-origin-bar-window.ts +++ b/src/main/browser/popup-origin-bar-window.ts @@ -92,7 +92,8 @@ function clampPopupContentSize(options: PopupChildWindowOptions): { // caller's popup bookkeeping cannot leak an entry for a window that no longer exists. function closeUnpreparedPopup( window: BaseWindow, - contentWebContents: Electron.WebContents + contentWebContents: Electron.WebContents, + originBarWebContents: Electron.WebContents ): PopupOriginBarWindow { const closedListeners: (() => void)[] = [] let closed = false @@ -105,6 +106,9 @@ function closeUnpreparedPopup( if (!contentWebContents.isDestroyed()) { contentWebContents.close() } + if (!originBarWebContents.isDestroyed()) { + originBarWebContents.close() + } if (!window.isDestroyed()) { window.close() } @@ -159,6 +163,7 @@ export function openPopupWithOriginBar( const originBarView = new WebContentsView({ webPreferences: { contextIsolation: true, nodeIntegration: false, sandbox: true } }) + const originBarWebContents = originBarView.webContents const contentView = new WebContentsView({ // Why: Electron rejects an explicitly undefined webContents; omitting it // lets WebContentsView create contents for Cmd/Ctrl-click popups. @@ -188,7 +193,7 @@ export function openPopupWithOriginBar( const contentWebContents = contentView.webContents if (prepareContent && !prepareContent(contentWebContents)) { - return closeUnpreparedPopup(window, contentWebContents) + return closeUnpreparedPopup(window, contentWebContents, originBarWebContents) } let currentUrl = initialUrl const renderOrigin = (): void => { @@ -202,15 +207,15 @@ export function openPopupWithOriginBar( } // Why: textContent + JSON encoding — the URL is attacker-controlled and // must never be interpolated into the bar's markup. - void originBarView.webContents + void originBarWebContents .executeJavaScript( `document.body.classList.toggle('insecure', ${insecure ? 'true' : 'false'});` + `document.getElementById('origin').textContent = ${JSON.stringify(label)};` ) .catch(() => {}) } - originBarView.webContents.once('did-finish-load', renderOrigin) - void originBarView.webContents.loadURL( + originBarWebContents.once('did-finish-load', renderOrigin) + void originBarWebContents.loadURL( `data:text/html;charset=utf-8,${encodeURIComponent(ORIGIN_BAR_HTML)}` ) @@ -253,6 +258,10 @@ export function openPopupWithOriginBar( // pages often notify the opener from unload. contentWebContents.close() } + // Why: BaseWindow does not destroy child WebContentsView contents when it closes. + if (!originBarWebContents.isDestroyed()) { + originBarWebContents.close() + } for (const listener of closedListeners) { listener() }