fix(browser): keep the dead-guest degrade honest — no spinner, no silent swallow

Review remediation for the guest guards.

- The attach bail now writes `loading: false` before setting
  browser_client_page_guest_unavailable, matching what retryGuestRecoveryRef already does on
  the pane's own route into that state. A page that died mid-load carries `loading: true` in
  the store, so without it the unavailable notice rendered beside a spinner nothing would
  ever stop. Uses the existing updatePageStateFromGuest effect event, not a new setter.
- The dead-guest condition is no longer silent: the metadata read logs the caught error under
  the subsystem's `[browser-client-page]` warn convention, and the attach bail records a
  `browser_client_page_guest_unavailable` crash breadcrumb via the existing
  recordRendererCrashBreadcrumb. Renderer diagnostics only capture window error/rejection, so
  the breadcrumb is what puts this on a channel crash reports actually carry — the registry
  liveness defect this change deliberately does not fix stays measurable, and a read failure
  that is not `Invalid guestInstanceId` is no longer indistinguishable from a dead guest.
- onFailLoad no longer pays five sync IPCs per discarded event: resolveBrowserWebviewLoadFailure
  accepts a lazy fallbackUrl and resolves it after the subframe/ERR_ABORTED filter. Covered by
  a new case in browser-webview-load-failure.test.ts.

Correction to the previous commit's narrative: only the metadata half reaches
browser_client_page_guest_unavailable. The focus half reaches no state at all — the guarded
BrowserPageGuestFocus wrapper returns false and the pane stays mounted over a webview the
registry already removed from the DOM, with no notice and no reopen-on-server escape. It stops
the crash; it does not diagnose the page.

Not changed, with reasons:
- The two sibling attach bails (renderer-unavailable, attach threw) omit the same `loading`
  write. That predates this branch and neither is reached by the dead-guest path; fixing them
  is a separate change.
- recordHistoryFromGuest still passes a raw `webview.getTitle()`. Substituting `metadata.title`
  is not behaviour-preserving: metadata.title falls back to the URL, so an untitled page would
  be filed in history under its URL instead of "New Tab". The call runs only after a five-read
  succeeded and sits in a DOM event listener, which cannot unwind a React commit.
This commit is contained in:
Neil
2026-09-02 21:37:41 -07:00
parent 37bc6a4577
commit 972a15697e
5 changed files with 67 additions and 12 deletions
@@ -3,11 +3,18 @@ import { act, cleanup, render, screen } from '@testing-library/react'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { BrowserPage } from '../../../../shared/browser-workspace-types'
const mocks = vi.hoisted(() => ({ attach: vi.fn(), detach: vi.fn() }))
const mocks = vi.hoisted(() => ({
attach: vi.fn(),
detach: vi.fn(),
recordBreadcrumb: vi.fn()
}))
vi.mock('./browser-client-page-renderer-installation', () => ({
attachBrowserClientPageToViewport: mocks.attach
}))
vi.mock('@/lib/crash-breadcrumb-recorder', () => ({
recordRendererCrashBreadcrumb: mocks.recordBreadcrumb
}))
vi.mock('sonner', () => ({
toast: { error: vi.fn(), success: vi.fn(), loading: vi.fn(), message: vi.fn() }
}))
@@ -33,7 +40,7 @@ function nullContentWindowFocus(): TypeError {
return new TypeError("Cannot read properties of null (reading 'focus')")
}
function page(): BrowserPage {
function page(overrides?: Partial<BrowserPage>): BrowserPage {
return {
id: 'page-a',
workspaceId: 'workspace-a',
@@ -45,7 +52,8 @@ function page(): BrowserPage {
canGoBack: false,
canGoForward: false,
loadError: null,
createdAt: 1
createdAt: 1,
...overrides
}
}
@@ -74,18 +82,21 @@ function createGuest(): Electron.WebviewTag & { getURL: ReturnType<typeof vi.fn>
return webview
}
function paneElement(isActive: boolean): React.JSX.Element {
function paneElement(
isActive: boolean,
options?: { browserTab?: BrowserPage; onUpdatePageState?: (id: string, state: unknown) => void }
): React.JSX.Element {
return (
<TooltipProvider>
<ClientHostedBrowserPagePane
browserTab={page()}
browserTab={options?.browserTab ?? page()}
workspaceId="workspace-a"
chromeShortcutScope="focused"
runtimeEnvironmentId="environment-a"
worktreeId="worktree-a"
placement={PLACEMENT}
isActive={isActive}
onUpdatePageState={vi.fn()}
onUpdatePageState={options?.onUpdatePageState ?? vi.fn()}
onSetUrl={vi.fn()}
/>
</TooltipProvider>
@@ -97,6 +108,7 @@ let webview: ReturnType<typeof createGuest>
beforeEach(() => {
mocks.attach.mockReset()
mocks.detach.mockReset()
mocks.recordBreadcrumb.mockReset()
installClientHostedPaneApi()
webview = createGuest()
})
@@ -115,6 +127,21 @@ describe('client-hosted browser pane over a dead guest', () => {
expect(() => render(paneElement(true))).not.toThrow()
expect(screen.getByText('Client-hosted browser unavailable')).toBeTruthy()
expect(mocks.detach).toHaveBeenCalled()
expect(mocks.recordBreadcrumb).toHaveBeenCalledWith('browser_client_page_guest_unavailable', {
browserPageId: 'page-a',
pageHostGeneration: PLACEMENT.pageHostGeneration
})
})
it('stops the spinner it inherited from a page that died mid-load', () => {
webview.getURL.mockImplementation(() => {
throw invalidGuestInstanceId()
})
const onUpdatePageState = vi.fn()
render(paneElement(true, { browserTab: page({ loading: true }), onUpdatePageState }))
expect(onUpdatePageState).toHaveBeenCalledWith('page-a', { loading: false })
})
it('survives activation focus after the retained tag left the DOM', () => {
@@ -1,5 +1,6 @@
import { useEffect, useEffectEvent, useLayoutEffect, useRef, useState } from 'react'
import { cn } from '@/lib/utils'
import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder'
import { useAppStore } from '@/store'
import type {
BrowserLoadError,
@@ -205,7 +206,14 @@ export function ClientHostedBrowserPagePane({
const attachedMetadata = readBrowserClientPageGuestMetadataIfLive(webview)
if (!attachedMetadata) {
attachment.detach()
// Why the loading write: nothing is left to report progress, and a spinner beside the
// unavailable notice is the one state this pane must not sit in (as at retryGuestRecovery).
updatePageStateFromGuest(browserTab.id, { loading: false })
setAttachmentError('browser_client_page_guest_unavailable')
recordRendererCrashBreadcrumb('browser_client_page_guest_unavailable', {
browserPageId: browserTab.id,
pageHostGeneration
})
return
}
const publisher = startBrowserClientPageMetadataPublisher({
@@ -269,7 +277,9 @@ export function ClientHostedBrowserPagePane({
}
const onFailLoad = (event: Event): void => {
const loadError = resolveBrowserWebviewLoadFailure(event as BrowserPageFailLoadEvent, {
fallbackUrl: readBrowserClientPageGuestMetadataIfLive(webview)?.url ?? null
// Lazy: the guest read is five sync IPCs, and ERR_ABORTED/subframe events are discarded
// before any fallback is needed.
fallbackUrl: () => readBrowserClientPageGuestMetadataIfLive(webview)?.url ?? null
})
if (!loadError) {
return
@@ -27,7 +27,10 @@ export function readBrowserClientPageGuestMetadataIfLive(
canGoBack: webview.canGoBack(),
canGoForward: webview.canGoForward()
}
} catch {
} catch (error) {
// Why logged: the tag reporting a live guest id for a destroyed guest is an unfixed defect,
// and this is the only place the field can see it happen (or see a different read failure).
console.warn('[browser-client-page] guest read failed, treating the page as gone:', error)
return null
}
}
@@ -1,4 +1,4 @@
import { describe, expect, it } from 'vitest'
import { describe, expect, it, vi } from 'vitest'
import { resolveBrowserWebviewLoadFailure } from './browser-webview-load-failure'
describe('resolveBrowserWebviewLoadFailure', () => {
@@ -49,6 +49,18 @@ describe('resolveBrowserWebviewLoadFailure', () => {
).toMatchObject({ validatedUrl: 'https://example.com/current' })
})
it('never reads a lazy fallback URL for an event it discards', () => {
const fallbackUrl = vi.fn(() => 'https://example.com/current')
expect(resolveBrowserWebviewLoadFailure({ errorCode: -3 }, { fallbackUrl })).toBeNull()
expect(fallbackUrl).not.toHaveBeenCalled()
expect(
resolveBrowserWebviewLoadFailure(
{ errorCode: -105, errorDescription: 'ERR_NAME_NOT_RESOLVED', validatedURL: '' },
{ fallbackUrl }
)
).toMatchObject({ validatedUrl: 'https://example.com/current' })
})
it('keeps a usable description when Chromium reports an empty one', () => {
expect(
resolveBrowserWebviewLoadFailure({ errorCode: -105, errorDescription: '' })
@@ -8,20 +8,23 @@ import type { BrowserPageFailLoadEvent } from '../describe-page/browser-page-typ
* cannot forget the ignore rules or build a differently-shaped BrowserLoadError.
*
* `fallbackUrl` covers failures that arrive without a validatedURL — pass the webview's
* current URL so the overlay names the page instead of about:blank.
* current URL so the overlay names the page instead of about:blank. Pass it as a function when
* reading it costs anything: discarded events never ask for it.
*/
export function resolveBrowserWebviewLoadFailure(
event: BrowserPageFailLoadEvent,
options: { fallbackUrl?: string | null } = {}
options: { fallbackUrl?: string | null | (() => string | null) } = {}
): BrowserLoadError | null {
// Why: Chromium reports redirect/cancel races as ERR_ABORTED (-3) even when the
// replacement navigation succeeds; subframe failures never blank the page.
if (event.isMainFrame === false || event.errorCode === -3) {
return null
}
const fallbackUrl =
typeof options.fallbackUrl === 'function' ? options.fallbackUrl() : options.fallbackUrl
return {
code: event.errorCode ?? -1,
description: event.errorDescription || 'Unknown load failure',
validatedUrl: redactKagiSessionToken(event.validatedURL || options.fallbackUrl || 'about:blank')
validatedUrl: redactKagiSessionToken(event.validatedURL || fallbackUrl || 'about:blank')
}
}