diff --git a/src/main/browser/agent-browser-bridge-state-commands.ts b/src/main/browser/agent-browser-bridge-state-commands.ts index 4e03055da57..779c6930887 100644 --- a/src/main/browser/agent-browser-bridge-state-commands.ts +++ b/src/main/browser/agent-browser-bridge-state-commands.ts @@ -15,6 +15,7 @@ import type { import { BrowserError } from './cdp-bridge' import { parseShellArgs, stripAgentBrowserTargetArgs } from './agent-browser-bridge-process' import { AgentBrowserBridgeInteractionCommands } from './agent-browser-bridge-interaction-commands' +import { sendGuestCdpCommand } from './guest-cdp-command' export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridgeInteractionCommands { // ── Cookie commands ── @@ -101,16 +102,14 @@ export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridge } // Why: agent-browser's `set viewport` has no `mobile` flag, so apply the emulation directly via CDP to honor Orca's --mobile. - await dbg.sendCommand('Emulation.setDeviceMetricsOverride', { + await sendGuestCdpCommand(wc, 'Emulation.setDeviceMetricsOverride', { width, height, deviceScaleFactor: scale, mobile }) // Why: BrowserView's compositor can keep the old host size after a metrics-only resize, cropping remote screencast clients. - await Promise.resolve(dbg.sendCommand('Emulation.setVisibleSize', { width, height })).catch( - () => {} - ) + await sendGuestCdpCommand(wc, 'Emulation.setVisibleSize', { width, height }).catch(() => {}) return { width, diff --git a/src/main/browser/agent-browser-bridge-test-harness.ts b/src/main/browser/agent-browser-bridge-test-harness.ts index c4c28006e82..34e381838b2 100644 --- a/src/main/browser/agent-browser-bridge-test-harness.ts +++ b/src/main/browser/agent-browser-bridge-test-harness.ts @@ -56,6 +56,7 @@ export type MockWebContents = { on: Mock<(event: string, listener: MockEmitterListener) => void> removeListener: Mock<(event: string, listener: MockEmitterListener) => void> isDestroyed: () => boolean + isCrashed: () => boolean invalidate: Mock<() => void> focus: Mock<() => void> debugger: MockWebContentsDebugger @@ -78,6 +79,7 @@ export function mockWebContents( on: vi.fn(), removeListener: vi.fn(), isDestroyed: () => false, + isCrashed: () => false, invalidate: vi.fn(), focus: vi.fn(), debugger: { diff --git a/src/main/browser/browser-manager-viewport-crashed-guest.test.ts b/src/main/browser/browser-manager-viewport-crashed-guest.test.ts new file mode 100644 index 00000000000..696be1a474b --- /dev/null +++ b/src/main/browser/browser-manager-viewport-crashed-guest.test.ts @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + appGetPathMock: vi.fn(() => '/downloads'), + shellOpenExternalMock: vi.fn(), + browserWindowFromWebContentsMock: vi.fn(), + menuBuildFromTemplateMock: vi.fn(), + guestOffMock: vi.fn(), + guestOnMock: vi.fn(), + guestSetBackgroundThrottlingMock: vi.fn(), + guestSetWindowOpenHandlerMock: vi.fn(), + guestOpenDevToolsMock: vi.fn(), + webContentsFromIdMock: vi.fn(), + screenGetCursorScreenPointMock: vi.fn(() => ({ x: 0, y: 0 })), + openPopupWithOriginBarMock: vi.fn(), + processUserAgent: + 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/134.0.0.0 Safari/537.36' +})) + +vi.mock('electron', () => ({ + app: { getPath: mocks.appGetPathMock }, + BrowserWindow: { fromWebContents: mocks.browserWindowFromWebContentsMock }, + clipboard: { writeText: vi.fn() }, + shell: { openExternal: mocks.shellOpenExternalMock }, + Menu: { buildFromTemplate: mocks.menuBuildFromTemplateMock }, + screen: { getCursorScreenPoint: mocks.screenGetCursorScreenPointMock }, + webContents: { fromId: mocks.webContentsFromIdMock } +})) +vi.mock('./popup-origin-bar-window', () => ({ + openPopupWithOriginBar: mocks.openPopupWithOriginBarMock +})) +vi.mock('./browser-process-user-agent', () => ({ + getBrowserProcessUserAgentIdentity: () => ({ mode: 'clean', userAgent: mocks.processUserAgent }) +})) + +import { browserManager } from './browser-manager' +import { + rendererWebContentsId, + resetBrowserManagerMocks, + resetBrowserManagerState +} from './browser-manager-test-harness' +import { createViewportGuestFactory } from './browser-manager-viewport-test-fixtures' + +const makeGuest = createViewportGuestFactory(mocks) +const PHONE = { width: 375, height: 667, deviceScaleFactor: 2, mobile: true } as const + +const metricsWrites = (sendCommand: ReturnType): unknown[][] => + sendCommand.mock.calls.filter(([method]) => method === 'Emulation.setDeviceMetricsOverride') + +describe('browserManager viewport on a guest whose renderer crashed', () => { + beforeEach(() => { + resetBrowserManagerMocks(mocks) + resetBrowserManagerState() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + + it('never resizes the dead guest, and applies the preset once the page reloads', async () => { + const { guest, debuggerSendCommand, setRendererCrashed } = makeGuest(42501) + mocks.webContentsFromIdMock.mockReturnValue(guest) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the viewport fixture implements every WebContents member these paths touch. + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'tab-crashed', + webContentsId: 42501, + rendererWebContentsId + }) + + // Chromium would segfault the main process on this write (its view is gone with the renderer). + setRendererCrashed(true) + await expect(browserManager.setViewportOverride('tab-crashed', PHONE)).resolves.toBe(false) + expect(metricsWrites(debuggerSendCommand)).toEqual([]) + + // The reloaded page's dom-ready reapplies the store's preset. + setRendererCrashed(false) + await expect(browserManager.setViewportOverride('tab-crashed', PHONE)).resolves.toBe(true) + expect(metricsWrites(debuggerSendCommand)).toEqual([ + ['Emulation.setDeviceMetricsOverride', PHONE] + ]) + }) +}) diff --git a/src/main/browser/browser-manager-viewport-test-fixtures.ts b/src/main/browser/browser-manager-viewport-test-fixtures.ts index 959e958491c..805988e7702 100644 --- a/src/main/browser/browser-manager-viewport-test-fixtures.ts +++ b/src/main/browser/browser-manager-viewport-test-fixtures.ts @@ -35,6 +35,8 @@ export type ViewportGuestHandle = { debuggerSendCommand: ReturnType debuggerIsAttached: ReturnType debuggerAttach: ReturnType + /** Flips what isCrashed() reports, as a renderer death and its reload do. */ + setRendererCrashed: (crashed: boolean) => void setGuestUserAgent: (ua: string) => void commitNavigationTo: (nextUrl: string) => void webContentsUserAgent: () => string @@ -55,6 +57,7 @@ export function createViewportGuestFactory( const debuggerIsAttached = vi.fn(() => true) const debuggerAttach = vi.fn() let currentUa = mocks.processUserAgent ?? GUEST_ELECTRON_UA + let rendererCrashed = false // Why: getURL() reports the last COMMITTED url — it does not move at did-start-navigation. let committedUrl = url // Chromium applies CDP commands in issue order, so a later-issued write wins however they settle, @@ -81,6 +84,7 @@ export function createViewportGuestFactory( const guest = { id, isDestroyed: vi.fn(() => false), + isCrashed: vi.fn(() => rendererCrashed), getType: vi.fn(() => 'webview'), getURL: vi.fn(() => committedUrl), getUserAgent: vi.fn(() => currentUa), @@ -107,6 +111,9 @@ export function createViewportGuestFactory( debuggerSendCommand, debuggerIsAttached, debuggerAttach, + setRendererCrashed: (crashed: boolean) => { + rendererCrashed = crashed + }, setGuestUserAgent: (ua: string) => { currentUa = ua }, diff --git a/src/main/browser/browser-manager-viewport.ts b/src/main/browser/browser-manager-viewport.ts index 98dff80fabb..64e50e5ea48 100644 --- a/src/main/browser/browser-manager-viewport.ts +++ b/src/main/browser/browser-manager-viewport.ts @@ -6,6 +6,7 @@ import { } from '../../shared/browser-annotation-viewport-bridge' import type { BrowserViewportOverride } from '../../shared/browser-workspace-types' import { BrowserManagerDownloadLifecycle } from './browser-manager-download-lifecycle' +import { sendGuestCdpCommand } from './guest-cdp-command' // Why no maxTouchPoints: Chromium rejects values outside 1..16 even when disabling, which left // touch emulation (and no-hover media features) on after leaving a mobile preset (#22749). @@ -155,7 +156,7 @@ export abstract class BrowserManagerViewport extends BrowserManagerDownloadLifec 'device metrics', () => override - ? dbg.sendCommand('Emulation.setDeviceMetricsOverride', { + ? sendGuestCdpCommand(guest, 'Emulation.setDeviceMetricsOverride', { width: override.width, height: override.height, deviceScaleFactor: override.deviceScaleFactor, diff --git a/src/main/browser/browser-screencast-debugger-command.ts b/src/main/browser/browser-screencast-debugger-command.ts index 3a607e416ec..b32eef8d308 100644 --- a/src/main/browser/browser-screencast-debugger-command.ts +++ b/src/main/browser/browser-screencast-debugger-command.ts @@ -2,15 +2,22 @@ import type { WebContents } from 'electron' const DEBUGGER_COMMAND_TIMEOUT_MS = 8_000 -export async function sendDebuggerCommand( +export function sendDebuggerCommand( dbg: WebContents['debugger'], method: string, params: Record = {} +): Promise { + return runDebuggerCommandWithTimeout(method, () => dbg.sendCommand(method, params)) +} + +export async function runDebuggerCommandWithTimeout( + method: string, + send: () => Promise ): Promise { let timeout: ReturnType | null = null try { return await Promise.race([ - Promise.resolve().then(() => dbg.sendCommand(method, params)), + Promise.resolve().then(send), new Promise((_, reject) => { timeout = setTimeout(() => { reject(new Error(`Timed out while running ${method}.`)) diff --git a/src/main/browser/browser-screencast-device-metrics.ts b/src/main/browser/browser-screencast-device-metrics.ts index ff805d14604..96037bd53ae 100644 --- a/src/main/browser/browser-screencast-device-metrics.ts +++ b/src/main/browser/browser-screencast-device-metrics.ts @@ -1,5 +1,9 @@ import type { Debugger, WebContents } from 'electron' -import { sendDebuggerCommand } from './browser-screencast-debugger-command' +import { + runDebuggerCommandWithTimeout, + sendDebuggerCommand +} from './browser-screencast-debugger-command' +import { sendGuestCdpCommand } from './guest-cdp-command' import type { BrowserScreencastOptions } from './browser-screencast-stream-types' import { positiveInteger, positiveNumber } from './browser-screencast-viewport-fit' @@ -15,6 +19,8 @@ export function createBrowserScreencastDeviceMetrics( options: BrowserScreencastOptions ): BrowserScreencastDeviceMetrics { let deviceMetricsOverridden = false + const sendViewportCommand = (method: string, params: Record): Promise => + runDebuggerCommandWithTimeout(method, () => sendGuestCdpCommand(webContents, method, params)) const clearDeviceMetricsOverride = async (): Promise => { if (webContents.isDestroyed() || !dbg.isAttached()) { @@ -38,13 +44,13 @@ export function createBrowserScreencastDeviceMetrics( // Why: Back/Forward and cross-process navigations can drop emulation while // the screencast remains attached. Reapply before fallback captures so the // page lays out at the client pane size, not the host BrowserView size. - await sendDebuggerCommand(dbg, 'Emulation.setDeviceMetricsOverride', { + await sendViewportCommand('Emulation.setDeviceMetricsOverride', { width: viewportWidth, height: viewportHeight, deviceScaleFactor, mobile: options.mobile === true }) - await sendDebuggerCommand(dbg, 'Emulation.setVisibleSize', { + await sendViewportCommand('Emulation.setVisibleSize', { width: viewportWidth, height: viewportHeight }).catch(() => {}) diff --git a/src/main/browser/browser-screencast-lifecycle.test.ts b/src/main/browser/browser-screencast-lifecycle.test.ts index 251b53fd469..fdf586b0ef8 100644 --- a/src/main/browser/browser-screencast-lifecycle.test.ts +++ b/src/main/browser/browser-screencast-lifecycle.test.ts @@ -21,7 +21,7 @@ function createWebContents() { attached = false }) debuggerApi.sendCommand = vi.fn(async () => ({})) - return { isDestroyed: vi.fn(() => false), debugger: debuggerApi } + return { isDestroyed: vi.fn(() => false), isCrashed: vi.fn(() => false), debugger: debuggerApi } } describe('browser screencast lifecycle', () => { diff --git a/src/main/browser/browser-screencast-snapshot-scaling.test.ts b/src/main/browser/browser-screencast-snapshot-scaling.test.ts index 4358d618376..1398214f2dc 100644 --- a/src/main/browser/browser-screencast-snapshot-scaling.test.ts +++ b/src/main/browser/browser-screencast-snapshot-scaling.test.ts @@ -20,7 +20,12 @@ function createMockWebContents(capturePage: () => Promise) { attached = false }) dbg.sendCommand = vi.fn(async () => ({})) - return { isDestroyed: vi.fn(() => false), debugger: dbg, capturePage: vi.fn(capturePage) } + return { + isDestroyed: vi.fn(() => false), + isCrashed: vi.fn(() => false), + debugger: dbg, + capturePage: vi.fn(capturePage) + } } function createCapturedImage(width: number, height: number) { diff --git a/src/main/browser/browser-screencast-web-contents-test-double.ts b/src/main/browser/browser-screencast-web-contents-test-double.ts index 13811850c68..030eec63783 100644 --- a/src/main/browser/browser-screencast-web-contents-test-double.ts +++ b/src/main/browser/browser-screencast-web-contents-test-double.ts @@ -10,6 +10,7 @@ export type MockScreencastDebugger = EventEmitter & { export type MockScreencastWebContents = { isDestroyed: ReturnType + isCrashed: ReturnType debugger: MockScreencastDebugger } @@ -29,6 +30,7 @@ export function createMockScreencastWebContents(): MockScreencastWebContents { return { isDestroyed: vi.fn(() => false), + isCrashed: vi.fn(() => false), debugger: dbg } } diff --git a/src/main/browser/cdp-debugger-channel.ts b/src/main/browser/cdp-debugger-channel.ts index f1cddbe20f1..b0c790138e8 100644 --- a/src/main/browser/cdp-debugger-channel.ts +++ b/src/main/browser/cdp-debugger-channel.ts @@ -3,6 +3,7 @@ import type { WebContents } from 'electron' import { acquireElectronDebugger, type ElectronDebuggerLease } from './electron-debugger-lease' import type { CdpClientResponseWriter } from './cdp-client-response-writer' import type { CdpSyntheticSessionRegistry } from './cdp-synthetic-session-registry' +import { sendGuestCdpCommand } from './guest-cdp-command' /** * The IO boundary with webContents.debugger: lease-based attach, event fan-out to @@ -86,10 +87,9 @@ export class CdpDebuggerChannel { params: Record, sessionId?: string ): Promise { - const command = sessionId - ? this.webContents.debugger.sendCommand(method, params, sessionId) - : this.webContents.debugger.sendCommand(method, params) - return Promise.resolve(command) + return sessionId + ? sendGuestCdpCommand(this.webContents, method, params, sessionId) + : sendGuestCdpCommand(this.webContents, method, params) } forwardCommand( diff --git a/src/main/browser/cdp-debugger-lifecycle.ts b/src/main/browser/cdp-debugger-lifecycle.ts index 225eb689dba..0880f9926a4 100644 --- a/src/main/browser/cdp-debugger-lifecycle.ts +++ b/src/main/browser/cdp-debugger-lifecycle.ts @@ -4,6 +4,7 @@ import type { CdpTabState } from './cdp-auxiliary-commands' import type { CdpCommandSender } from './snapshot-engine' import type { CdpBridgeState } from './cdp-bridge-state' import { createCdpDebuggerMessageListener } from './cdp-debugger-events' +import { sendGuestCdpCommand } from './guest-cdp-command' export class CdpDebuggerLifecycle { constructor(private readonly bridgeState: CdpBridgeState) {} @@ -83,7 +84,7 @@ export class CdpDebuggerLifecycle { makeCdpSender(guest: WebContents, sessionId?: string): CdpCommandSender { return (method: string, params?: Record) => { - const command = guest.debugger.sendCommand(method, params, sessionId) as Promise + const command = sendGuestCdpCommand(guest, method, params, sessionId) // Why: Electron's CDP sendCommand can hang on a stale debugger session, so a 10s timeout bounds the RPC. let timer: ReturnType return Promise.race([ diff --git a/src/main/browser/cdp-ws-proxy-test-harness.ts b/src/main/browser/cdp-ws-proxy-test-harness.ts index c66a3cbf6b1..3f5765c18ab 100644 --- a/src/main/browser/cdp-ws-proxy-test-harness.ts +++ b/src/main/browser/cdp-ws-proxy-test-harness.ts @@ -25,6 +25,7 @@ export type MockWebContents = { webContents: { debugger: MockDebugger isDestroyed: () => boolean + isCrashed: () => boolean focus: Mock<() => void> printToPDF: Mock<() => Promise> reload: Mock<() => void> @@ -72,6 +73,7 @@ export function createMockWebContents(): MockWebContents { webContents: { debugger: debuggerObj, isDestroyed: () => destroyed, + isCrashed: () => false, focus: vi.fn(), printToPDF: vi.fn(async () => Buffer.from('%PDF-test')), reload: vi.fn(), diff --git a/src/main/browser/guest-cdp-command.test.ts b/src/main/browser/guest-cdp-command.test.ts new file mode 100644 index 00000000000..3ad71959681 --- /dev/null +++ b/src/main/browser/guest-cdp-command.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it, vi } from 'vitest' +import { BrowserError } from './browser-error' +import { sendGuestCdpCommand } from './guest-cdp-command' + +function makeGuest(state: { crashed?: boolean; destroyed?: boolean } = {}) { + const sendCommand = vi.fn(async () => ({ ok: true })) + const guest = { + isDestroyed: vi.fn(() => state.destroyed ?? false), + isCrashed: vi.fn(() => { + if (state.destroyed) { + throw new Error('Object has been destroyed') + } + return state.crashed ?? false + }), + debugger: { sendCommand } + } + return { guest, sendCommand } +} + +describe('sendGuestCdpCommand', () => { + it.each(['Emulation.setDeviceMetricsOverride', 'Emulation.setVisibleSize'])( + 'refuses %s while the renderer is gone instead of letting Chromium crash the app', + async (method) => { + const { guest, sendCommand } = makeGuest({ crashed: true }) + const sent = sendGuestCdpCommand(guest, method, { width: 375, height: 667 }) + await expect(sent).rejects.toBeInstanceOf(BrowserError) + await expect(sent).rejects.toMatchObject({ code: 'browser_cdp_error' }) + expect(sendCommand).not.toHaveBeenCalled() + } + ) + + it('refuses a resize on a destroyed guest without asking it whether it crashed', async () => { + const { guest, sendCommand } = makeGuest({ destroyed: true }) + await expect( + sendGuestCdpCommand(guest, 'Emulation.setDeviceMetricsOverride', { width: 1, height: 1 }) + ).rejects.toBeInstanceOf(BrowserError) + expect(sendCommand).not.toHaveBeenCalled() + }) + + it('still sends commands that do not resize the view to a crashed guest', async () => { + const { guest, sendCommand } = makeGuest({ crashed: true }) + await expect( + sendGuestCdpCommand(guest, 'Emulation.setUserAgentOverride', { userAgent: 'x' }) + ).resolves.toEqual({ ok: true }) + expect(sendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { userAgent: 'x' }) + }) + + it('sends a resize to a live guest, and passes a session id only when one is given', async () => { + const { guest, sendCommand } = makeGuest() + await sendGuestCdpCommand(guest, 'Emulation.setVisibleSize', { width: 2, height: 3 }) + await sendGuestCdpCommand(guest, 'DOM.enable', {}, 'iframe-session') + expect(sendCommand.mock.calls).toEqual([ + ['Emulation.setVisibleSize', { width: 2, height: 3 }], + ['DOM.enable', {}, 'iframe-session'] + ]) + }) +}) diff --git a/src/main/browser/guest-cdp-command.ts b/src/main/browser/guest-cdp-command.ts new file mode 100644 index 00000000000..ed7735df2d8 --- /dev/null +++ b/src/main/browser/guest-cdp-command.ts @@ -0,0 +1,42 @@ +import type { WebContents } from 'electron' +import { BrowserError } from './browser-error' + +// Why: Chromium resizes the page's view for these without checking the view still exists +// (WebContentsImpl::SetDeviceEmulationSize). A crashed renderer takes its view with it until the +// reload builds a new one, so one of these sent in that gap segfaults Orca's main process. +const VIEW_RESIZING_CDP_METHODS: ReadonlySet = new Set([ + 'Emulation.setDeviceMetricsOverride', + 'Emulation.setVisibleSize' +]) + +type GuestCdpTarget = Pick & { + debugger: Pick +} + +/** + * The gate for guest CDP commands: every viewport writer and every sender that forwards a caller's + * method (agent bridge, CDP proxy) goes through here, so none can hand Chromium a command that + * crashes the app while the guest's renderer is dead. + */ +export function sendGuestCdpCommand( + guest: GuestCdpTarget, + method: string, + params?: Record, + sessionId?: string +): Promise { + // Why same task as the send: isCrashed() flips in the same Chromium task that drops the view, + // so checking right before sendCommand leaves no gap for the renderer to die in between. + if (VIEW_RESIZING_CDP_METHODS.has(method) && (guest.isDestroyed() || guest.isCrashed())) { + return Promise.reject( + new BrowserError( + 'browser_cdp_error', + 'The page crashed; its viewport can be changed again once it reloads.' + ) + ) + } + return Promise.resolve( + sessionId === undefined + ? guest.debugger.sendCommand(method, params) + : guest.debugger.sendCommand(method, params, sessionId) + ) +}