fix(browser): correct what a parked page reports and which one a page-less command means

Round-two review of the merged listing. Four defects, all in what parking
projects rather than in the reclamation itself:

- A page-less command picked the most recently used parked page. Recency is not
  activeness: an explicit `--page b` makes b the most recently used while a is
  still the active tab, and a page-less command has always meant "the active
  tab". The retained active page now wins, with recency only breaking the tie.
- Parking the active page promotes another live tab to active, which then parks
  claiming the flag too, so a listing could report two active tabs. Only the
  newest claim holds it, and the merge enforces at most one either way.
- A parked page reported no load failure, showing a page that could not load as
  healthy at its address. The failure becomes unreadable once the guest is
  unregistered, so it is captured onto the record. Certificate failures are
  deliberately not carried: their challenge id dies with the renderer, so
  surfacing one would offer an action that cannot work.
- Parking sniffed the committed address, which cannot tell an intentional
  navigation to about:blank from the blank page a failed load left behind. The
  record now follows did-navigate, so waking returns the page to where it
  actually is; chrome-error addresses are the failure, not a destination, and
  never replace the address that produced them.

Each has a mutation-checked regression test.
This commit is contained in:
Jinwoo-H
2026-08-19 19:07:11 -04:00
parent 9f77906067
commit 77a4c3693c
8 changed files with 197 additions and 37 deletions
+8 -3
View File
@@ -6,6 +6,8 @@
// WebContents uniformly regardless of how the page was created. This interface
// isolates the only step that actually differs: tab creation and teardown.
import type { BrowserLoadError } from '../../shared/browser-workspace-types'
export type BrowserBackendCreateTab = {
url: string
worktreeId?: string
@@ -22,6 +24,9 @@ export type ParkedBrowserPage = {
title: string
/** Whether the page was its worktree's active tab when it parked. */
active?: boolean
/** The failure the page last reported. Reclaiming a renderer does not make a
* page that failed to load healthy again. */
loadError?: BrowserLoadError | null
}
export type BrowserBackend = {
@@ -40,7 +45,7 @@ export type BrowserBackend = {
wakeTab?(browserPageId: string): Promise<boolean>
/** Pages this backend still owns whose renderer is currently reclaimed. */
listParkedPages?(worktreeId?: string): ParkedBrowserPage[]
/** Most-recently-used parked page, for commands that target a worktree
* rather than a page id. */
getMostRecentlyUsedParkedPageId?(worktreeId?: string): string | null
/** The parked page a page-less command should target: the one that was
* active when it parked, else the most recently used. */
getParkedPageIdForImplicitTarget?(worktreeId?: string): string | null
}
@@ -60,4 +60,17 @@ describe('mergeParkedBrowserTabs', () => {
parked: true
})
})
it('reports at most one active tab when several parked pages claim it', () => {
// Why: `active` is a single selection. Parking the active page promotes
// another live tab, which can later park claiming the flag as well.
const merged = mergeParkedBrowserTabs([], [parked('b', true), parked('c', true)])
expect(merged.filter((tab) => tab.active).map((tab) => tab.browserPageId)).toEqual(['b'])
})
it('carries a parked page load failure into the listing', () => {
const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' }
const [tab] = mergeParkedBrowserTabs([], [{ ...parked('b'), loadError }])
expect(tab.loadError).toEqual(loadError)
})
})
@@ -16,17 +16,23 @@ export function mergeParkedBrowserTabs(
}
// Why: parking clears the bridge's active pointer, which then lands on a
// surviving live tab. A parked page may only claim active when nothing live
// does, or a listing can report two active tabs.
const liveClaimsActive = live.some((tab) => tab.active)
// does, and at most one may — `active` is a single selection, so a listing
// that reports two is not something any caller can act on.
let activeClaimed = live.some((tab) => tab.active)
const merged = [...live]
for (const page of parked) {
const active = !activeClaimed && page.active === true
activeClaimed = activeClaimed || active
merged.push({
browserPageId: page.browserPageId,
index: merged.length,
url: page.url,
title: page.title,
active: !liveClaimsActive && page.active === true,
active,
parked: true,
// Why: a page that failed to load is still failed while parked; hiding
// that would show a broken page as healthy at its address.
loadError: page.loadError ?? null,
worktreeId: page.worktreeId ?? null,
profileId: page.profileId ?? null
})
@@ -25,6 +25,8 @@ type FakeWindow = BrowserWindow & {
__destroyed: boolean
__url: string
__destroyedListeners: (() => void)[]
__navigationListeners: ((e: unknown, url: string) => void)[]
navigateTo: (url: string) => void
}
let nextWebContentsId = 100
@@ -36,7 +38,14 @@ function makeWindow(): FakeWindow {
__destroyed: false,
__url: 'about:blank',
__destroyedListeners: [] as (() => void)[],
__navigationListeners: [] as ((e: unknown, url: string) => void)[],
isDestroyed: () => win.__destroyed,
navigateTo: (url: string) => {
win.__url = url
for (const listener of win.__navigationListeners) {
listener(null, url)
}
},
destroy: () => {
win.__destroyed = true
for (const listener of win.__destroyedListeners) {
@@ -52,6 +61,11 @@ function makeWindow(): FakeWindow {
if (event === 'destroyed') {
win.__destroyedListeners.push(listener)
}
},
on: (event: string, listener: (e: unknown, url: string) => void) => {
if (event === 'did-navigate' || event === 'did-navigate-in-page') {
win.__navigationListeners.push(listener)
}
}
}
} as unknown as FakeWindow
@@ -66,9 +80,17 @@ type Harness = {
registered: Map<string, number>
windows: FakeWindow[]
clock: { value: number }
activePageId: string | undefined
}
function createHarness(overrides: { pinned?: Set<string>; activePageId?: string } = {}): Harness {
function createHarness(
overrides: {
pinned?: Set<string>
activePageId?: string
loadError?: { code: number; description: string; validatedUrl: string } | null
} = {}
): Harness {
const state = { activePageId: overrides.activePageId }
const order: string[] = []
const registered = new Map<string, number>()
const windows: FakeWindow[] = []
@@ -95,7 +117,8 @@ function createHarness(overrides: { pinned?: Set<string>; activePageId?: string
order.push(`unregister:${browserPageId}`)
registered.delete(browserPageId)
},
getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null
getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null,
getBrowserPageLoadError: () => overrides.loadError ?? null
} as unknown as BrowserManager
const bridge = {
@@ -105,7 +128,7 @@ function createHarness(overrides: { pinned?: Set<string>; activePageId?: string
onProcessSwap: vi.fn(async (browserPageId: string, webContentsId: number) => {
order.push(`process-swap:${browserPageId}:${webContentsId}`)
}),
isActiveBrowserPage: (browserPageId: string) => overrides.activePageId === browserPageId
isActiveBrowserPage: (browserPageId: string) => state.activePageId === browserPageId
} as unknown as AgentBrowserBridge
const backend = new OffscreenBrowserBackend(manager, {
@@ -114,7 +137,18 @@ function createHarness(overrides: { pinned?: Set<string>; activePageId?: string
now: () => clock.value
})
return { backend, manager, bridge, order, registered, windows, clock }
return {
backend,
manager,
bridge,
order,
registered,
windows,
clock,
set activePageId(value: string | undefined) {
state.activePageId = value
}
}
}
beforeEach(() => {
@@ -145,7 +179,8 @@ describe('OffscreenBrowserBackend reclamation', () => {
profileId: 'default',
url: 'https://example.test/a',
title: `title-${h.windows[0].webContents.id}`,
active: false
active: false,
loadError: null
}
])
})
@@ -186,7 +221,7 @@ describe('OffscreenBrowserBackend reclamation', () => {
const h = createHarness()
await h.backend.createTab({ url: 'https://example.test/a' })
const [pageId] = [...h.registered.keys()]
h.windows[0].__url = 'https://example.test/moved'
h.windows[0].navigateTo('https://example.test/moved')
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
@@ -204,11 +239,10 @@ describe('OffscreenBrowserBackend reclamation', () => {
})
it('keeps the requested address when a page parks before committing one', async () => {
// Why: an uncommitted window still reports about:blank, and adopting that
// would send the wake to a blank page instead of the agent's URL.
// Why: a page whose load never committed must still wake to the address the
// agent asked for, not to the blank page the window started on.
const h = createHarness()
await h.backend.createTab({ url: 'https://example.test/slow', browserPageId: 'a' })
h.windows[0].__url = 'about:blank'
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
@@ -298,6 +332,72 @@ describe('OffscreenBrowserBackend reclamation', () => {
expect(h.backend.listParkedPages().map((page) => page.browserPageId)).toEqual(['b'])
})
it('keeps an intentional navigation to about:blank across a park', async () => {
// Why: an in-page `location.href = "about:blank"` is a real destination.
// Sniffing the address at park time could not tell it apart from the blank
// page a window starts on, so the record follows navigation instead.
const h = createHarness()
await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' })
h.windows[0].navigateTo('about:blank')
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
expect(h.backend.listParkedPages()[0]?.url).toBe('about:blank')
})
it('ignores a chrome-error address so a wake retries the real one', async () => {
const h = createHarness()
await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' })
h.windows[0].navigateTo('chrome-error://chromewebdata/')
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
expect(h.backend.listParkedPages()[0]?.url).toBe('https://example.test/a')
})
it('carries a load failure onto the parked record', async () => {
// Why: reclaiming a renderer does not make a page that failed to load
// healthy, and the failure is unreadable once the guest is unregistered.
const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' }
const h = createHarness({ loadError })
await h.backend.createTab({ url: 'https://nope', browserPageId: 'a' })
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
expect(h.backend.listParkedPages()[0]?.loadError).toEqual(loadError)
})
it('lets only the newest claim hold the parked active flag', async () => {
// Why: parking the active page promotes another live tab to active, which
// then parks claiming the flag too. `active` is a single selection.
const h = createHarness({ activePageId: 'a' })
await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' })
await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' })
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
// Simulate the promotion: b is now the active page and parks claiming it.
h.activePageId = 'b'
await h.backend.wakeTab('b')
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
const claimed = h.backend.listParkedPages().filter((page) => page.active)
expect(claimed.map((page) => page.browserPageId)).toEqual(['b'])
})
it('targets the page that was active, not merely the most recently used', async () => {
// Why: an explicit `--page b` command makes b the most recently used while
// a is still the active tab, and a page-less command means "the active tab".
const h = createHarness({ activePageId: 'a' })
await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' })
h.clock.value += 1_000
await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' })
h.clock.value += 120_000
await h.backend.reclaimIdlePages()
expect(h.backend.getParkedPageIdForImplicitTarget('wt-1')).toBe('a')
})
it('reports the most recently used parked page for implicit targeting', async () => {
const h = createHarness()
for (const id of ['a', 'b', 'c', 'd']) {
@@ -307,7 +407,7 @@ describe('OffscreenBrowserBackend reclamation', () => {
h.clock.value += 10_000
await h.backend.reclaimIdlePages()
expect(h.backend.getMostRecentlyUsedParkedPageId()).toBe('b')
expect(h.backend.getParkedPageIdForImplicitTarget()).toBe('b')
})
it('restarts the reclaim clock when a resident page is used', async () => {
@@ -395,7 +495,7 @@ describe('OffscreenBrowserBackend reclamation', () => {
await h.backend.reclaimIdlePages()
expect(h.backend.listParkedPages('wt-1').map((page) => page.browserPageId)).toEqual(['a'])
expect(h.backend.getMostRecentlyUsedParkedPageId('wt-2')).toBe('b')
expect(h.backend.getParkedPageIdForImplicitTarget('wt-2')).toBe('b')
})
it('destroys every page it owns on shutdown', async () => {
+28 -14
View File
@@ -68,6 +68,7 @@ export class OffscreenBrowserBackend implements BrowserBackend {
window: null,
activeWhenParked: false,
loading: false,
loadError: null,
lastActivityAt: this.now()
}
this.pages.add(page)
@@ -151,9 +152,9 @@ export class OffscreenBrowserBackend implements BrowserBackend {
return this.pages.listParked(worktreeId)
}
/** Most-recently-used parked page in a worktree, for implicit targeting. */
getMostRecentlyUsedParkedPageId(worktreeId?: string): string | null {
return this.pages.mostRecentlyUsedParkedId(worktreeId)
/** The parked page a page-less command should target in this worktree. */
getParkedPageIdForImplicitTarget(worktreeId?: string): string | null {
return this.pages.parkedIdForImplicitTarget(worktreeId)
}
destroyAll(): void {
@@ -217,23 +218,25 @@ export class OffscreenBrowserBackend implements BrowserBackend {
if (!page?.window || page.window.isDestroyed()) {
return
}
// Why: capture the committed address before teardown so a woken page
// returns to where the agent left it, not to its original create URL. A
// page that never committed still reports the empty or blank address the
// window started on; adopting that would send the wake to the wrong page.
const wc = page.window.webContents
const committed = wc.getURL()
const uncommitted = !committed || committed === 'about:blank'
if (!uncommitted && !committed.startsWith('chrome-error://')) {
page.url = committed
}
page.title = wc.getTitle() ?? page.title
// Why: the record's address is kept current by did-navigate, so parking
// never has to guess it from a WebContents that may be sitting on the blank
// page a failed load left behind.
page.title = page.window.webContents.getTitle() ?? page.title
// Why: a reclaimed renderer does not make a page that failed to load
// healthy, and the failure becomes unreadable once the guest is
// unregistered — so carry it onto the record.
page.loadError = this.browserManager.getBrowserPageLoadError(browserPageId)
page.activeWhenParked =
this.options
.getAgentBrowserBridge?.()
?.isActiveBrowserPage(browserPageId, page.worktreeId) === true
await this.releaseRenderer(page, browserPageId)
page.window = null
if (page.activeWhenParked) {
// Why: teardown promotes another live tab to active, which then parks
// claiming the flag too. Only the newest claim may hold it.
this.pages.claimParkedActive(browserPageId, page.worktreeId)
}
}
// Why: teardown order matters — the bridge must destroy the helper session and
@@ -281,6 +284,17 @@ export class OffscreenBrowserBackend implements BrowserBackend {
// the throw would escape the 'destroyed' listener into the main process.
// Capture the id while it is still safe to read.
const webContentsId = win.webContents.id
// Why: the record's address must follow the page, not the create call — an
// agent that navigates with `goto` or in-page script has to be woken back
// to where it actually is. A chrome-error address is the failure, not a
// destination, so it never replaces the address that produced it.
const trackNavigation = (_event: unknown, url: string): void => {
if (page.window === win && url && !url.startsWith('chrome-error://')) {
page.url = url
}
}
win.webContents.on('did-navigate', trackNavigation)
win.webContents.on('did-navigate-in-page', trackNavigation)
// Why: if the window is destroyed out from under us (crash, app teardown),
// drop the page so commands fail cleanly instead of resolving a dead
// WebContents. Parking destroys it deliberately, so it opts out here.
@@ -1,4 +1,5 @@
import type { BrowserWindow } from 'electron'
import type { BrowserLoadError } from '../../shared/browser-workspace-types'
import type { ParkedBrowserPage } from './browser-backend'
// Why (STA-4341): a headless browser page and the renderer behind it have
@@ -20,6 +21,8 @@ export type OffscreenBrowserPage = {
activeWhenParked: boolean
/** True while the initial or post-wake navigation is still in flight. */
loading: boolean
/** The failure the page reported when its renderer was reclaimed. */
loadError: BrowserLoadError | null
lastActivityAt: number
}
@@ -77,18 +80,37 @@ export class OffscreenBrowserOpenPages {
profileId: page.profileId,
url: page.url,
title: page.title,
active: page.activeWhenParked
active: page.activeWhenParked,
loadError: page.loadError
}))
}
/** Most-recently-used parked page, for commands that name a worktree. */
mostRecentlyUsedParkedId(worktreeId?: string): string | null {
/**
* The parked page a page-less command should target. Activity order alone is
* wrong: an explicit `--page B` command makes B the most recently used while
* A is still the active tab, and a page-less command has always meant "the
* active tab". So the retained active page wins, and recency only breaks the
* tie when none is recorded.
*/
parkedIdForImplicitTarget(worktreeId?: string): string | null {
let best: OffscreenBrowserPage | null = null
for (const page of this.parked(worktreeId)) {
if (page.activeWhenParked) {
return page.browserPageId
}
if (!best || page.lastActivityAt > best.lastActivityAt) {
best = page
}
}
return best?.browserPageId ?? null
}
/** Only one page per worktree may carry the active flag across a park. */
claimParkedActive(browserPageId: string, worktreeId?: string): void {
for (const page of this.parked(worktreeId)) {
if (page.browserPageId !== browserPageId) {
page.activeWhenParked = false
}
}
}
}
@@ -109,7 +109,7 @@ function createFakeHeadlessHost(live: string[], parked: string[]): FakeHeadlessH
url: `https://example.test/${browserPageId}`,
title: browserPageId
})),
getMostRecentlyUsedParkedPageId: () => state.parkedPageIds.at(-1) ?? null
getParkedPageIdForImplicitTarget: () => state.parkedPageIds.at(-1) ?? null
} as unknown as BrowserBackend
state.host = {
+2 -2
View File
@@ -327,7 +327,7 @@ export class RuntimeBrowserCommands {
// Read-only callers opt out — listing tabs must never cost a renderer.
if (options.wakeParkedPage !== false && !this.host.getAvailableAuthoritativeWindow()) {
const offscreen = this.host.getOffscreenBrowserBackend()
const parkedPageId = offscreen?.getMostRecentlyUsedParkedPageId?.(worktreeId)
const parkedPageId = offscreen?.getParkedPageIdForImplicitTarget?.(worktreeId)
if (parkedPageId && (await offscreen?.wakeTab?.(parkedPageId))) {
return
}
@@ -1768,7 +1768,7 @@ export class RuntimeBrowserCommands {
const resolvedTabId =
tabId ??
bridge.getActivePageId(worktreeId) ??
offscreen.getMostRecentlyUsedParkedPageId?.(worktreeId) ??
offscreen.getParkedPageIdForImplicitTarget?.(worktreeId) ??
null
if (!resolvedTabId) {
return { closed: false }