fix(browser): stop failing goto when a redirect or download aborts the load (#9822)

* 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
2026-10-18 16:03:17 +00:00 · 2026-07-21 21:00:57 -07:00
parent b25c298a2b
commit bc4cc20d21
2 changed files with 277 additions and 7 deletions
@@ -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<string, () => 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<string, unknown[]> }).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<string, unknown[]> }).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()
  })

@@ -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<void> {
+  if (!isWebContentsLoading(wc)) {
+    return Promise.resolve()
+  }
+
+  return new Promise((resolve, reject) => {
+    let settled = false
+    let timeout: ReturnType<typeof setTimeout> | 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<typeof setTimeout> | 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 }