From 972a15697ef835b68bcfacd23d143f67f7603ea3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:37:41 -0700 Subject: [PATCH] =?UTF-8?q?fix(browser):=20keep=20the=20dead-guest=20degra?= =?UTF-8?q?de=20honest=20=E2=80=94=20no=20spinner,=20no=20silent=20swallow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...tHostedBrowserPagePane.dead-guest.test.tsx | 39 ++++++++++++++++--- .../ClientHostedBrowserPagePane.tsx | 12 +++++- .../browser-client-page-guest-metadata.ts | 5 ++- .../browser-webview-load-failure.test.ts | 14 ++++++- .../navigate/browser-webview-load-failure.ts | 9 +++-- 5 files changed, 67 insertions(+), 12 deletions(-) diff --git a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.dead-guest.test.tsx b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.dead-guest.test.tsx index 70e3b7141ab..c4eac706f9a 100644 --- a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.dead-guest.test.tsx +++ b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.dead-guest.test.tsx @@ -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 { 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 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 ( @@ -97,6 +108,7 @@ let webview: ReturnType 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', () => { diff --git a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx index d996d6e7b23..49e6ad405c2 100644 --- a/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx +++ b/src/renderer/src/components/browser-pane/ClientHostedBrowserPagePane.tsx @@ -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 diff --git a/src/renderer/src/components/browser-pane/browser-client-page-guest-metadata.ts b/src/renderer/src/components/browser-pane/browser-client-page-guest-metadata.ts index c409b106e03..8638fc15e21 100644 --- a/src/renderer/src/components/browser-pane/browser-client-page-guest-metadata.ts +++ b/src/renderer/src/components/browser-pane/browser-client-page-guest-metadata.ts @@ -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 } } diff --git a/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.test.ts b/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.test.ts index 44e59fdbedd..1595b69f6aa 100644 --- a/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.test.ts +++ b/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.test.ts @@ -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: '' }) diff --git a/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.ts b/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.ts index c39c5afe53b..03228c3e1d6 100644 --- a/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.ts +++ b/src/renderer/src/components/browser-pane/navigate/browser-webview-load-failure.ts @@ -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') } }