From 65e2b8c6dd862a6a058f049bd6ac3159fe32a00a Mon Sep 17 00:00:00 2001 From: Mike Lyons Date: Wed, 7 Oct 2026 14:55:02 -0400 Subject: [PATCH] fix(browser): capture the tab that was asked for, not the park window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit screenshot_tab intermittently returned another tab's pixels, or a frozen older frame, while get_tab_url / get_tab_clickables stayed correct for the same id in the same moment. Both paths resolve the tab the same way, so the bug was never in resolution — it was in the capture. The offscreen path asked CDP for `Page.captureScreenshot` with `fromSurface: false`. That routes through GetSnapshotFromBrowser, which on macOS grabs the *native window* and crops it to the view's rect. Every hidden tab is parked in one shared park window, so the crop returns whatever composites on top there — a sibling tab's live page, or a stale window frame. `fromSurface: true` copies the requesting view's own compositor surface instead, which is also what the on-screen path has always used. Captures are now serialized too: capturing a visible tab whose emulated viewport is taller than its pane detaches the view and parks it mid-shot, so two captures at once meant one reparenting the other's host. Every await inside a capture is bounded so a wedged one can't stall the queue. Measured in a real Electron run driving BrowserManager directly: one tab visible under a tall emulated viewport, a parked sibling, captured concurrently — the sibling came back holding the other tab's page 3/3 times before, 0/3 after, and a 20-assertion sweep of the reported sequence (two then three tabs, pane switches, mobile emulation, about:blank, hidden host window) goes from 2 failures to 0. Surface capture converts through the display profile, so parked-tab screenshots now have the same slight color shift on-screen captures already had. Co-Authored-By: Claude Opus 5 --- src/main/browser-manager-capture.test.ts | 311 +++++++++++++++++++++++ src/main/browser-manager.ts | 90 +++++-- 2 files changed, 379 insertions(+), 22 deletions(-) create mode 100644 src/main/browser-manager-capture.test.ts diff --git a/src/main/browser-manager-capture.test.ts b/src/main/browser-manager-capture.test.ts new file mode 100644 index 00000000..9d4650a5 --- /dev/null +++ b/src/main/browser-manager-capture.test.ts @@ -0,0 +1,311 @@ +// Regression tests for "screenshot_tab returned another tab's pixels". +// +// The fakes here model one specific piece of measured Chromium behaviour, and +// the tests are only as good as that model, so it's worth stating: +// +// Page.captureScreenshot with `fromSurface: true` copies the *requesting +// view's* compositor surface. With `fromSurface: false` it goes through +// RenderWidgetHostImpl::GetSnapshotFromBrowser, which grabs the *native +// window* and crops it to the view's rect — so it returns whatever is +// composited on top in that window, which for a parked tab is a sibling tab. +// +// Verified against a real Electron 41 run: two tabs parked in the shared park +// window, one visible under an emulated viewport taller than its pane (which +// makes capturing it park the view briefly), captured concurrently. With +// `fromSurface: false` the parked tab came back holding the other tab's page +// 3/3 times; with `fromSurface: true`, 0/3. `captureFromWindow` below encodes +// that, so flipping the flag back fails these tests instead of shipping. + +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('./debug', () => ({ log: () => {} })) + +interface FakeImageMeta { + marker: string + width: number + height: number +} + +/** Stand-in for a frame: carries which page's pixels it holds. */ +function encodeFrame(meta: FakeImageMeta): Buffer { + return Buffer.from(JSON.stringify(meta)) +} + +function decodeFrame(base64: string): FakeImageMeta { + return JSON.parse(Buffer.from(base64, 'base64').toString()) as FakeImageMeta +} + +function makeImage(buf: Buffer, size?: { width: number; height: number }) { + const meta = JSON.parse(buf.toString()) as FakeImageMeta + const dims = size ?? { width: meta.width, height: meta.height } + const img = { + getSize: () => dims, + isEmpty: () => false, + resize: (next: { width: number; height: number }) => makeImage(buf, next), + toPNG: () => buf, + toJPEG: () => buf + } + return img +} + +type Rect = { x: number; y: number; width: number; height: number } + +/** Captures that the test can hold open, to observe overlap. */ +const gate: { pending: Array<() => void>; hold: boolean; failCapture: boolean } = { + pending: [], + hold: false, + failCapture: false +} + +class FakeWebContents { + url = 'about:blank' + attached = false + destroyed = false + reloads = 0 + commands: string[] = [] + constructor(private view: FakeWebContentsView) {} + + getURL = (): string => this.url + getTitle = (): string => this.url + getUserAgent = (): string => 'fake-ua' + isDestroyed = (): boolean => this.destroyed + isCrashed = (): boolean => false + close = (): void => { + this.destroyed = true + } + on = (): void => {} + once = (): void => {} + removeAllListeners = (): void => {} + sendInputEvent = (): void => {} + isDevToolsOpened = (): boolean => false + openDevTools = (): void => {} + closeDevTools = (): void => {} + reload = (): void => { + this.reloads++ + } + navigationHistory = { + canGoBack: () => false, + canGoForward: () => false, + goBack: () => {}, + goForward: () => {} + } + loadURL = async (url: string): Promise => { + this.url = url + } + executeJavaScript = async (): Promise => null + + /** The on-screen path: a per-widget surface copy, always this view's own. */ + capturePage = async () => makeImage(encodeFrame(this.view.frame())) + + debugger = { + isAttached: (): boolean => this.attached, + attach: (): void => { + this.attached = true + }, + detach: (): void => { + this.attached = false + }, + sendCommand: async (method: string, params?: Record): Promise => { + this.commands.push(method) + if (method !== 'Page.captureScreenshot') return {} + if (gate.failCapture) throw new Error('UnknownVizError') + if (!this.view.host) throw new Error('no surface: view is not parented') + const fromSurface = params?.['fromSurface'] === true + const meta = fromSurface ? this.view.frame() : this.view.host.captureFromWindow(this.view) + if (gate.hold) { + await new Promise((resolve) => gate.pending.push(resolve)) + } + return { data: encodeFrame(meta).toString('base64') } + } + } +} + +class FakeWebContentsView { + webContents = new FakeWebContents(this) + host: FakeWindow | null = null + private bounds: Rect = { x: 0, y: 0, width: 0, height: 0 } + setBackgroundColor = (): void => {} + setBounds = (b: Rect): void => { + this.bounds = { ...b } + } + getBounds = (): Rect => ({ ...this.bounds }) + /** This view's own pixels. */ + frame(): FakeImageMeta { + return { + marker: this.webContents.url, + width: this.bounds.width, + height: this.bounds.height + } + } +} + +class FakeWindow { + static all: FakeWindow[] = [] + children: FakeWebContentsView[] = [] + private destroyed = false + private content = { width: 1280, height: 800 } + + constructor(_opts?: unknown) { + FakeWindow.all.push(this) + } + static getAllWindows = (): FakeWindow[] => FakeWindow.all.filter((w) => !w.isDestroyed()) + + contentView = { + addChildView: (view: FakeWebContentsView): void => { + if (view.host) view.host.children = view.host.children.filter((v) => v !== view) + this.children = this.children.filter((v) => v !== view) + // Last one added composites on top — which is the whole problem. + this.children.push(view) + view.host = this + }, + removeChildView: (view: FakeWebContentsView): void => { + this.children = this.children.filter((v) => v !== view) + if (view.host === this) view.host = null + } + } + + /** What an OS-level grab of this window, cropped to `view`'s rect, holds: + * the topmost child's pixels, not necessarily the requested view's. */ + captureFromWindow(view: FakeWebContentsView): FakeImageMeta { + const top = this.children[this.children.length - 1] ?? view + const own = view.frame() + return { marker: top.frame().marker, width: own.width, height: own.height } + } + + getContentBounds = (): { width: number; height: number } => ({ ...this.content }) + setContentSize = (width: number, height: number): void => { + this.content = { width, height } + } + isDestroyed = (): boolean => this.destroyed + destroy = (): void => { + this.destroyed = true + } + once = (): void => {} + on = (): void => {} + hide = (): void => {} + show = (): void => {} +} + +vi.mock('electron', () => ({ + WebContentsView: FakeWebContentsView, + BrowserWindow: FakeWindow, + app: { on: () => {} }, + session: { fromPartition: () => ({}) }, + nativeImage: { + createFromBuffer: (buf: Buffer) => makeImage(buf) + } +})) + +const { BrowserManager } = await import('./browser-manager') + +const WT = '/tmp/wt-a' +const RED = 'https://red.test/' +const BLUE = 'https://blue.test/' + +const tick = async (n = 4): Promise => { + for (let i = 0; i < n; i++) await new Promise((r) => setTimeout(r, 0)) +} + +async function markerOf( + bm: InstanceType, + tabId: string +): Promise { + const shot = await bm.capturePage(tabId, { format: 'png' }) + if (!shot?.data) throw new Error(`no image: ${shot?.error}`) + return decodeFrame(shot.data).marker +} + +describe('capturePage tab isolation', () => { + beforeEach(() => { + FakeWindow.all = [] + gate.pending = [] + gate.hold = false + gate.failCapture = false + }) + + it('captures a parked tab from its own surface, not from the shared park window', async () => { + const bm = new BrowserManager() + bm.create('red', WT, RED) + // Parked second, so it composites on top of red in the park window. + bm.create('blue', WT, BLUE) + + expect(await markerOf(bm, 'red')).toBe(RED) + expect(await markerOf(bm, 'blue')).toBe(BLUE) + }) + + it('keeps a parked tab clean while a sibling capture parks a visible view', async () => { + const bm = new BrowserManager() + bm.create('red', WT, RED) + bm.create('blue', WT, BLUE) + + const win = new FakeWindow() + bm.setBounds('blue', win as unknown as Electron.BrowserWindow, { + x: 0, + y: 0, + width: 800, + height: 600 + }) + // Emulated taller than the pane, so capturing blue has to park it first. + await bm.setViewport('blue', { + width: 1280, + height: 1600, + deviceScaleFactor: 0, + mobile: false + }) + + const [red, blue] = await Promise.all([markerOf(bm, 'red'), markerOf(bm, 'blue')]) + expect(red).toBe(RED) + expect(blue).toBe(BLUE) + + // And blue is handed back to its window afterwards. + expect(bm.getViewport('blue')).toMatchObject({ width: 1280, height: 1600 }) + expect(await markerOf(bm, 'blue')).toBe(BLUE) + }) + + it('serializes captures so one cannot reparent another mid-shot', async () => { + const bm = new BrowserManager() + bm.create('red', WT, RED) + bm.create('blue', WT, BLUE) + await tick() + + gate.hold = true + const first = bm.capturePage('red', { format: 'png' }) + const second = bm.capturePage('blue', { format: 'png' }) + await tick() + expect(gate.pending).toHaveLength(1) + + gate.pending.pop()?.() + await tick() + expect(gate.pending).toHaveLength(1) + gate.pending.pop()?.() + + const [a, b] = await Promise.all([first, second]) + expect(decodeFrame(a!.data!).marker).toBe(RED) + expect(decodeFrame(b!.data!).marker).toBe(BLUE) + }) + + it('reports the failure instead of falling back to a window-level grab', async () => { + const bm = new BrowserManager() + bm.create('red', WT, RED) + bm.create('blue', WT, BLUE) + await tick() + + gate.failCapture = true + const shot = await bm.capturePage('red', { format: 'png' }) + expect(shot?.data).toBeUndefined() + expect(shot?.error).toMatch(/capture failed/) + }) + + it('re-parks and still captures the right tab after the park window is gone', async () => { + const bm = new BrowserManager() + bm.create('red', WT, RED) + bm.create('blue', WT, BLUE) + await tick() + + // macOS closes every window without quitting the app. + for (const win of [...FakeWindow.all]) win.destroy() + + expect(await markerOf(bm, 'red')).toBe(RED) + expect(await markerOf(bm, 'blue')).toBe(BLUE) + }) +}) diff --git a/src/main/browser-manager.ts b/src/main/browser-manager.ts index f9bf4417..599df8ef 100644 --- a/src/main/browser-manager.ts +++ b/src/main/browser-manager.ts @@ -62,6 +62,26 @@ function sanitizePartition(worktreePath: string): string { return worktreePath.replace(/[^a-zA-Z0-9._-]/g, '_').slice(0, 120) } +/** Captures run one at a time (see `capturePage`), so every await inside one + * has to be bounded — a capture that never settles would otherwise wedge the + * queue and take every later screenshot down with it. */ +async function withCaptureTimeout(work: Promise, what: string): Promise { + let timer: NodeJS.Timeout | undefined + try { + return await Promise.race([ + work, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error(`${what} timed out after ${CAPTURE_TIMEOUT_MS}ms`)), + CAPTURE_TIMEOUT_MS + ) + }) + ]) + } finally { + clearTimeout(timer) + } +} + const SPECIAL_KEYS: Record = { enter: 'Return', return: 'Return', @@ -219,6 +239,8 @@ export class BrowserManager implements BrowserManagerLike { private store: Store | null = null private parkWindow: BrowserWindow | null = null private quitGuardInstalled = false + /** Tail of the serialized capture chain — see `capturePage`. */ + private captureQueue: Promise = Promise.resolve() setStore(store: Store): void { this.store = store @@ -869,35 +891,39 @@ export class BrowserManager implements BrowserManagerLike { /** Render a view that isn't on screen. * - * `webContents.capturePage()` reads the compositor surface, which a hidden - * view doesn't have — it throws, or worse returns the last frame from before - * the tab was hidden. CDP's Page.captureScreenshot with `fromSurface: false` - * renders in the renderer process instead, so it stays correct for a parked - * tab. It does still need the view to be parented, hence the park window. + * `webContents.capturePage()` on a *parentless* view has nothing to read — + * it reports 0×0 and throws — which is what the park window fixes. Once the + * view is parented, CDP's Page.captureScreenshot gets us a frame without the + * tab ever being displayed. + * + * `fromSurface` must stay **true**. It reads the requesting view's own + * compositor surface. `fromSurface: false` instead routes through + * `RenderWidgetHostImpl::GetSnapshotFromBrowser`, which on macOS grabs the + * *native window* and crops it to the view's rect — so a capture of tab A + * can come back holding tab B's pixels whenever B is also composited into + * the shared park window. That's not theoretical: with one tab visible under + * an emulated viewport taller than its pane (so capturing it parks it + * briefly) and a concurrent capture of a parked sibling, the sibling + * returned the visible tab's page every time. Same root cause as captures + * coming back as a frozen older frame — what the window held, not what the + * tab was showing. Surface capture is also what the on-screen path + * (`capturePage()`) uses, so both paths now agree. */ private async captureOffscreen(inst: BrowserInstance): Promise { const dbg = inst.view.webContents.debugger const wasAttached = dbg.isAttached() if (!wasAttached) dbg.attach('1.3') - let timer: NodeJS.Timeout | undefined try { - const shot = dbg.sendCommand('Page.captureScreenshot', { - format: 'png', - fromSurface: false, - captureBeyondViewport: false - }) as Promise<{ data?: string }> - const result = await Promise.race([ - shot, - new Promise((_, reject) => { - timer = setTimeout( - () => reject(new Error(`Page.captureScreenshot timed out after ${CAPTURE_TIMEOUT_MS}ms`)), - CAPTURE_TIMEOUT_MS - ) - }) - ]) + const result = await withCaptureTimeout( + dbg.sendCommand('Page.captureScreenshot', { + format: 'png', + fromSurface: true, + captureBeyondViewport: false + }) as Promise<{ data?: string }>, + 'Page.captureScreenshot' + ) return nativeImage.createFromBuffer(Buffer.from(result?.data ?? '', 'base64')) } finally { - clearTimeout(timer) // Leaving the debugger attached would block DevTools on this tab. if (!wasAttached && dbg.isAttached()) { try { @@ -909,9 +935,26 @@ export class BrowserManager implements BrowserManagerLike { } } + /** Capturing a tab can move its view between windows (see `restoreTo` + * below), so two captures in flight at once means one is reparenting the + * other's host mid-shot. Run them one at a time: a screenshot is ~100ms and + * agents rarely want two at once, which is a cheap price for every capture + * seeing a settled view tree. */ async capturePage( tabId: string, opts?: { format?: 'jpeg' | 'png'; quality?: number } + ): Promise { + const turn = this.captureQueue.then( + () => this.captureNow(tabId, opts), + () => this.captureNow(tabId, opts) + ) + this.captureQueue = turn.catch(() => {}) + return turn + } + + private async captureNow( + tabId: string, + opts?: { format?: 'jpeg' | 'png'; quality?: number } ): Promise { const inst = this.instances.get(tabId) if (!inst) return null @@ -941,7 +984,10 @@ export class BrowserManager implements BrowserManagerLike { let image: NativeImage | null = null if (inst.visible) { try { - image = await inst.view.webContents.capturePage() + image = await withCaptureTimeout( + inst.view.webContents.capturePage(), + 'capturePage' + ) } catch (err) { // Minimized / occluded windows lose their surface too; the offscreen // path below still works because the view is parented.