From bc4cc20d2194c8a56c99d209dd7fef542d9f4d91 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 21 Jul 2026 19:56:07 -0700 Subject: [PATCH] fix(browser): stop failing goto when a redirect or download aborts the load (#9822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): stop failing goto when a redirect or download aborts the load Electron's loadURL rejects with ERR_ABORTED (-3) when the initial navigation is superseded — a client-side/meta redirect (common in SSO flows) or a download-triggered load. Since #9633 drives goto through wc.loadURL directly, that rejection surfaced as a spurious 'Failed to navigate' browser_error even though the page landed fine. Treat ERR_ABORTED like offscreen-browser-backend already does: resolve with the page's actual URL/title. Every other loadURL failure still fails closed. * fix(browser): settle replacement navigation after abort * fix(browser): clean up aborted navigation destruction race --- src/main/browser/agent-browser-bridge.test.ts | 172 +++++++++++++++++- src/main/browser/agent-browser-bridge.ts | 112 +++++++++++- 2 files changed, 277 insertions(+), 7 deletions(-) diff --git a/src/main/browser/agent-browser-bridge.test.ts b/src/main/browser/agent-browser-bridge.test.ts index 5df35b0d3c8..dc804a2dbe1 100644 --- a/src/main/browser/agent-browser-bridge.test.ts +++ b/src/main/browser/agent-browser-bridge.test.ts @@ -98,6 +98,9 @@ function mockWebContents(id: number, url = 'https://example.com', title = 'Examp loadURL: vi.fn(async (nextUrl: string) => { currentUrl = nextUrl }), + isLoading: vi.fn(() => false), + on: vi.fn(), + removeListener: vi.fn(), isDestroyed: () => false, invalidate: vi.fn(), focus: vi.fn(), @@ -1617,6 +1620,7 @@ describe('AgentBrowserBridge', () => { }) expect(wc.loadURL).toHaveBeenCalledWith('https://example.com/next') + expect(wc.isLoading).not.toHaveBeenCalled() expect(execFileMock).not.toHaveBeenCalled() }) @@ -1667,17 +1671,177 @@ describe('AgentBrowserBridge', () => { } }) - it('fails closed when direct navigation is aborted', async () => { - const wc = mockWebContents(100, 'https://example.com/current', 'Current') + it('waits for a superseding navigation to land after direct navigation aborts', async () => { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + let currentUrl = 'https://example.com/current' + let loading = true + const listeners = new Map void>() + wc.getURL = () => currentUrl + wc.isLoading.mockImplementation(() => loading) + wc.on.mockImplementation((event: string, listener: () => void) => { + listeners.set(event, listener) + }) wc.loadURL.mockRejectedValue( - Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED' }) + Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED', errno: -3 }) ) webContentsFromIdMock.mockReturnValue(wc) - await expect(bridge.goto('https://example.com/download')).rejects.toMatchObject({ + const navigation = bridge.goto('https://example.com/sso') + await vi.waitFor(() => expect(listeners.get('did-stop-loading')).toBeDefined()) + + currentUrl = 'https://example.com/login' + loading = false + listeners.get('did-stop-loading')!() + + await expect(navigation).resolves.toEqual({ + url: 'https://example.com/login', + title: 'Example' + }) + expect(wc.removeListener).toHaveBeenCalledWith( + 'did-stop-loading', + listeners.get('did-stop-loading') + ) + expect(wc.removeListener).toHaveBeenCalledWith('destroyed', listeners.get('destroyed')) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('resolves with the unchanged page when a download-triggered load aborts', async () => { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + wc.loadURL.mockRejectedValue(Object.assign(new Error('ERR_ABORTED (-3)'), { errno: -3 })) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('https://example.com/download')).resolves.toEqual({ + url: 'https://example.com/current', + title: 'Example' + }) + expect(wc.isLoading).toHaveBeenCalledTimes(1) + expect(wc.on).not.toHaveBeenCalledWith('did-stop-loading', expect.any(Function)) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('fails closed when the page prevents direct navigation', async () => { + const wc = mockWebContents(100, 'https://example.com/unsaved', 'Unsaved changes') + wc.loadURL.mockImplementation(async () => { + const preventUnload = wc.on.mock.calls.find( + ([event]) => event === 'will-prevent-unload' + )?.[1] as ((event: { defaultPrevented: boolean }) => void) | undefined + preventUnload!({ defaultPrevented: false }) + throw Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED', errno: -3 }) + }) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('https://example.com/next')).rejects.toMatchObject({ code: 'browser_error', message: 'Failed to navigate browser page tab-1: ERR_ABORTED (-3)' }) + const preventUnload = wc.on.mock.calls.find(([event]) => event === 'will-prevent-unload')?.[1] + expect(wc.removeListener).toHaveBeenCalledWith('will-prevent-unload', preventUnload) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('fails closed when the navigation superseding an abort fails', async () => { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + wc.loadURL.mockRejectedValue( + Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED', errno: -3 }) + ) + webContentsFromIdMock.mockReturnValue(wc) + const getBrowserPageLoadError = vi.fn(() => ({ + code: -105, + description: 'Name not resolved', + validatedUrl: 'https://nxdomain.example/' + })) + const b = new AgentBrowserBridge( + mockBrowserManager(new Map([['tab-1', 100]]), undefined, { + getBrowserPageLoadError + }) + ) + b.setActiveTab(100) + + await expect(b.goto('https://example.com/redirect')).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to navigate browser page tab-1: Name not resolved (-105)' + }) + expect(execFileMock).not.toHaveBeenCalled() + }) + + it('bounds and cleans up a superseding navigation that never settles', async () => { + vi.useFakeTimers() + try { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + wc.isLoading.mockReturnValue(true) + wc.loadURL.mockRejectedValue( + Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED', errno: -3 }) + ) + webContentsFromIdMock.mockReturnValue(wc) + + const navigation = bridge.goto('https://example.com/redirect') + const rejection = expect(navigation).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to navigate browser page tab-1: Browser navigation timed out after 30000ms' + }) + await vi.advanceTimersByTimeAsync(30_000) + + await rejection + const stopLoading = wc.on.mock.calls.find(([event]) => event === 'did-stop-loading')?.[1] + const destroyed = wc.on.mock.calls.find(([event]) => event === 'destroyed')?.[1] + expect(wc.removeListener).toHaveBeenCalledWith('did-stop-loading', stopLoading) + expect(wc.removeListener).toHaveBeenCalledWith('destroyed', destroyed) + expect(vi.getTimerCount()).toBe(0) + expect( + (bridge as unknown as { commandQueues: Map }).commandQueues.size + ).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it('cleans up when the guest is destroyed while attaching the replacement wait', async () => { + vi.useFakeTimers() + try { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + let destroyed = false + wc.isDestroyed = () => destroyed + wc.isLoading.mockReturnValueOnce(true).mockImplementationOnce(() => { + destroyed = true + throw new Error('Object has been destroyed') + }) + wc.loadURL.mockRejectedValue( + Object.assign(new Error('ERR_ABORTED (-3)'), { code: 'ERR_ABORTED', errno: -3 }) + ) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('https://example.com/redirect')).rejects.toMatchObject({ + code: 'browser_tab_not_found', + message: 'Browser page tab-1 is no longer available' + }) + + const stopLoading = wc.on.mock.calls.find(([event]) => event === 'did-stop-loading')?.[1] + const destroyedListener = wc.on.mock.calls.find(([event]) => event === 'destroyed')?.[1] + expect(wc.removeListener).toHaveBeenCalledWith('did-stop-loading', stopLoading) + expect(wc.removeListener).toHaveBeenCalledWith('destroyed', destroyedListener) + expect(vi.getTimerCount()).toBe(0) + expect( + (bridge as unknown as { commandQueues: Map }).commandQueues.size + ).toBe(0) + } finally { + vi.useRealTimers() + } + }) + + it('fails closed when direct navigation fails for a non-abort reason', async () => { + const wc = mockWebContents(100, 'https://example.com/current', 'Example') + wc.loadURL.mockRejectedValue( + Object.assign(new Error('ERR_NAME_NOT_RESOLVED (-105)'), { + code: 'ERR_NAME_NOT_RESOLVED', + errno: -105 + }) + ) + webContentsFromIdMock.mockReturnValue(wc) + + await expect(bridge.goto('https://nxdomain.example')).rejects.toMatchObject({ + code: 'browser_error', + message: 'Failed to navigate browser page tab-1: ERR_NAME_NOT_RESOLVED (-105)' + }) expect(execFileMock).not.toHaveBeenCalled() }) diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 3011fd56b92..565352d3d50 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -264,6 +264,75 @@ function classifyErrorCode(message: string): string { return 'browser_error' } +function isAbortedNavigationError(error: unknown): boolean { + if (!error || typeof error !== 'object') { + return false + } + const { code, errno } = error as { code?: unknown; errno?: unknown } + return code === 'ERR_ABORTED' || errno === -3 +} + +function isWebContentsLoading(wc: WebContents): boolean { + try { + return wc.isLoading() + } catch { + // Why: destruction races are resolved against the authoritative page registration after the wait. + return false + } +} + +function waitForAbortedNavigationReplacement( + wc: WebContents, + browserPageId: string, + timeoutMs: number +): Promise { + if (!isWebContentsLoading(wc)) { + return Promise.resolve() + } + + return new Promise((resolve, reject) => { + let settled = false + let timeout: ReturnType | null = null + const finish = (error?: BrowserError): void => { + if (settled) { + return + } + settled = true + wc.removeListener('did-stop-loading', onDidStopLoading) + wc.removeListener('destroyed', onDestroyed) + if (timeout) { + clearTimeout(timeout) + } + if (error) { + reject(error) + } else { + resolve() + } + } + const onDidStopLoading = (): void => finish() + const onDestroyed = (): void => finish() + + wc.on('did-stop-loading', onDidStopLoading) + wc.on('destroyed', onDestroyed) + timeout = setTimeout( + () => + finish( + new BrowserError( + 'browser_error', + `Failed to navigate browser page ${browserPageId}: Browser navigation timed out after ${EMBEDDED_NAVIGATION_TIMEOUT_MS}ms` + ) + ), + timeoutMs + ) + timeout.unref?.() + + // Why: the replacement can finish between loadURL rejecting and listener attachment. + if (!isWebContentsLoading(wc)) { + finish() + } + }) +} + function isTabClosedTransportError(message: string): boolean { return /session destroyed while command|session destroyed while commands|connection refused|cdp discovery methods failed|websocket connect failed/i.test( message @@ -777,6 +846,15 @@ export class AgentBrowserBridge { if (!navigationUrl) { throw new BrowserError('invalid_argument', `Unsupported browser URL: ${url}`) } + const navigationState: { preventUnloadEvent: Electron.Event | null } = { + preventUnloadEvent: null + } + const onWillPreventUnload = (event: Electron.Event): void => { + navigationState.preventUnloadEvent = event + } + wc.on('will-prevent-unload', onWillPreventUnload) + let navigationAborted = false + const navigationDeadline = Date.now() + EMBEDDED_NAVIGATION_TIMEOUT_MS let navigationTimeout: ReturnType | null = null try { await Promise.race([ @@ -795,14 +873,33 @@ export class AgentBrowserBridge { }) ]) } catch (error) { + if (navigationTimeout) { + clearTimeout(navigationTimeout) + navigationTimeout = null + } if (!this.getWebContents(target.webContentsId)) { throw this.createPageUnavailableError(`orca-tab-${target.browserPageId}`) } - throw new BrowserError( - 'browser_error', - `Failed to navigate browser page ${target.browserPageId}: ${error instanceof Error ? error.message : String(error)}` + // Why: ERR_ABORTED also covers a page vetoing unload; that navigation did not succeed. + if ( + !isAbortedNavigationError(error) || + (navigationState.preventUnloadEvent !== null && + !navigationState.preventUnloadEvent.defaultPrevented) + ) { + throw new BrowserError( + 'browser_error', + `Failed to navigate browser page ${target.browserPageId}: ${error instanceof Error ? error.message : String(error)}` + ) + } + navigationAborted = true + // Why: a superseding navigation rejects the first load before its replacement has landed. + await waitForAbortedNavigationReplacement( + wc, + target.browserPageId, + Math.max(0, navigationDeadline - Date.now()) ) } finally { + wc.removeListener('will-prevent-unload', onWillPreventUnload) if (navigationTimeout) { clearTimeout(navigationTimeout) } @@ -811,6 +908,15 @@ export class AgentBrowserBridge { // Why: cross-process navigation can replace the guest while retaining the same authoritative page id. const navigatedTarget = this.resolveCommandTarget(worktreeId, target.browserPageId) const navigatedWebContents = this.requireTargetWebContents(navigatedTarget) + const loadError = navigationAborted + ? this.browserManager.getBrowserPageLoadError(target.browserPageId) + : null + if (loadError) { + throw new BrowserError( + 'browser_error', + `Failed to navigate browser page ${target.browserPageId}: ${loadError.description} (${loadError.code})` + ) + } return { url: navigatedWebContents.getURL(), title: navigatedWebContents.getTitle() } }, { ensureSession: false }