fix(browser): a crashed page no longer crashes Orca when its viewport is resized (#23852)

* fix(browser): never resize a browser tab whose page crashed, which segfaulted Orca

Chromium's Emulation.setDeviceMetricsOverride and Emulation.setVisibleSize
resize the page's view without checking it still exists
(WebContentsImpl::SetDeviceEmulationSize). A crashed page renderer takes
its view with it until a reload builds a new one, so either command sent
in that gap killed Orca's whole main process with SIGSEGV.

One gate, sendGuestCdpCommand, now refuses those two commands while the
tab's renderer is gone. The viewport preset writer, the agent viewport
command, the agent bridge's command sender, the CDP proxy and the
screencast device metrics all send through it. The check runs in the same
task as the send, so the renderer cannot die in between. The page's own
reload reapplies the chosen preset on dom-ready, as it already did.

* test(browser): type the CDP gate's target so its test double needs no cast

CI's changed-lines gate rejects new type assertions. The gate only uses
isDestroyed, isCrashed and debugger.sendCommand, so it now takes exactly
that shape; the viewport test keeps the one fixture cast its siblings use,
with a line-specific SAFETY note.
This commit is contained in:
Brennan Benson
2026-09-29 15:12:32 -07:00
committed by GitHub
parent 0fd098e519
commit 4ebec19f46
15 changed files with 228 additions and 17 deletions
@@ -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,
@@ -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: {
@@ -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<typeof vi.fn>): 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]
])
})
})
@@ -35,6 +35,8 @@ export type ViewportGuestHandle = {
debuggerSendCommand: ReturnType<typeof vi.fn>
debuggerIsAttached: ReturnType<typeof vi.fn>
debuggerAttach: ReturnType<typeof vi.fn>
/** 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
},
+2 -1
View File
@@ -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,
@@ -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<string, unknown> = {}
): Promise<unknown> {
return runDebuggerCommandWithTimeout(method, () => dbg.sendCommand(method, params))
}
export async function runDebuggerCommandWithTimeout(
method: string,
send: () => Promise<unknown>
): Promise<unknown> {
let timeout: ReturnType<typeof setTimeout> | null = null
try {
return await Promise.race([
Promise.resolve().then(() => dbg.sendCommand(method, params)),
Promise.resolve().then(send),
new Promise<never>((_, reject) => {
timeout = setTimeout(() => {
reject(new Error(`Timed out while running ${method}.`))
@@ -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<string, unknown>): Promise<unknown> =>
runDebuggerCommandWithTimeout(method, () => sendGuestCdpCommand(webContents, method, params))
const clearDeviceMetricsOverride = async (): Promise<void> => {
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(() => {})
@@ -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', () => {
@@ -20,7 +20,12 @@ function createMockWebContents(capturePage: () => Promise<unknown>) {
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) {
@@ -10,6 +10,7 @@ export type MockScreencastDebugger = EventEmitter & {
export type MockScreencastWebContents = {
isDestroyed: ReturnType<typeof vi.fn>
isCrashed: ReturnType<typeof vi.fn>
debugger: MockScreencastDebugger
}
@@ -29,6 +30,7 @@ export function createMockScreencastWebContents(): MockScreencastWebContents {
return {
isDestroyed: vi.fn(() => false),
isCrashed: vi.fn(() => false),
debugger: dbg
}
}
+4 -4
View File
@@ -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<string, unknown>,
sessionId?: string
): Promise<unknown> {
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(
+2 -1
View File
@@ -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<string, unknown>) => {
const command = guest.debugger.sendCommand(method, params, sessionId) as Promise<unknown>
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<typeof setTimeout>
return Promise.race([
@@ -25,6 +25,7 @@ export type MockWebContents = {
webContents: {
debugger: MockDebugger
isDestroyed: () => boolean
isCrashed: () => boolean
focus: Mock<() => void>
printToPDF: Mock<() => Promise<Buffer>>
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(),
@@ -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']
])
})
})
+42
View File
@@ -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<string> = new Set([
'Emulation.setDeviceMetricsOverride',
'Emulation.setVisibleSize'
])
type GuestCdpTarget = Pick<WebContents, 'isDestroyed' | 'isCrashed'> & {
debugger: Pick<WebContents['debugger'], 'sendCommand'>
}
/**
* 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<string, unknown>,
sessionId?: string
): Promise<unknown> {
// 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)
)
}