mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
fix(browser): close the round-three parked-page reporting and reuse gaps
- Reusing a page id while its close was still tearing down let the new renderer register and the old close then unregister it, leaving a page the runtime owned but no command could resolve. A create now waits out an in-flight release for that id, which orders the whole reuse. - Closing a parked page told nobody: it has no WebContents teardown for the session-tabs publication to piggyback on, so a paired client kept showing a ghost tab until an operation against it failed. - A page waiting on a certificate decision was parkable. Its challenge id dies with the renderer, so parking discarded both the warning and the ability to approve it. Such a page is work waiting on an answer and stays resident. - The load helper resolves on its own 30s timeout, so `loading` stopped covering a navigation that was merely slow. The renderer is asked directly. - An iframe changing its own hash fires did-navigate-in-page too, and its address was being recorded as the tab's — waking would have landed the page on the iframe's document. Only main-frame navigations move the record. - `tab switch --index` reported the position the wake had renumbered the target to, which described neither the request nor the listing the caller read. Each mutation-checked. An ownership guard added alongside the reuse fix turned out to be unreachable once creates are ordered, so it is not here.
This commit is contained in:
@@ -289,8 +289,8 @@ targets it transparently rebuilds the renderer and reloads the address. What a
|
||||
park does _not_ preserve is in-page JavaScript state — cookies and local
|
||||
storage live in the profile partition and survive, but an unsubmitted form or
|
||||
an SPA's in-memory state does not. A page is never parked while a paired
|
||||
client is streaming it, while a command against it is in flight, or while its
|
||||
navigation has not committed.
|
||||
client is streaming it, while a command against it is in flight, while it is
|
||||
still navigating, or while it is waiting on a certificate decision.
|
||||
|
||||
Two evictors decide what stays resident, mirroring terminal pane parking. The
|
||||
cap is the primary one:
|
||||
|
||||
@@ -25,8 +25,9 @@ type FakeWindow = BrowserWindow & {
|
||||
__destroyed: boolean
|
||||
__url: string
|
||||
__destroyedListeners: (() => void)[]
|
||||
__navigationListeners: ((e: unknown, url: string) => void)[]
|
||||
navigateTo: (url: string) => void
|
||||
__navigationListeners: ((e: unknown, url: string, isMainFrame?: boolean) => void)[]
|
||||
navigateTo: (url: string, isMainFrame?: boolean) => void
|
||||
__loading: boolean
|
||||
}
|
||||
|
||||
let nextWebContentsId = 100
|
||||
@@ -38,12 +39,15 @@ function makeWindow(): FakeWindow {
|
||||
__destroyed: false,
|
||||
__url: 'about:blank',
|
||||
__destroyedListeners: [] as (() => void)[],
|
||||
__navigationListeners: [] as ((e: unknown, url: string) => void)[],
|
||||
__navigationListeners: [] as ((e: unknown, url: string, isMainFrame?: boolean) => void)[],
|
||||
__loading: false,
|
||||
isDestroyed: () => win.__destroyed,
|
||||
navigateTo: (url: string) => {
|
||||
win.__url = url
|
||||
navigateTo: (url: string, isMainFrame = true) => {
|
||||
if (isMainFrame) {
|
||||
win.__url = url
|
||||
}
|
||||
for (const listener of win.__navigationListeners) {
|
||||
listener(null, url)
|
||||
listener(null, url, isMainFrame)
|
||||
}
|
||||
},
|
||||
destroy: () => {
|
||||
@@ -56,14 +60,22 @@ function makeWindow(): FakeWindow {
|
||||
id,
|
||||
isDestroyed: () => win.__destroyed,
|
||||
getURL: () => win.__url,
|
||||
isLoading: () => win.__loading,
|
||||
getTitle: () => `title-${id}`,
|
||||
once: (event: string, listener: () => void) => {
|
||||
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') {
|
||||
on: (event: string, listener: (e: unknown, url: string, isMainFrame?: boolean) => void) => {
|
||||
if (event === 'did-navigate') {
|
||||
win.__navigationListeners.push((e, url, isMainFrame) => {
|
||||
if (isMainFrame !== false) {
|
||||
listener(e, url)
|
||||
}
|
||||
})
|
||||
}
|
||||
if (event === 'did-navigate-in-page') {
|
||||
win.__navigationListeners.push(listener)
|
||||
}
|
||||
}
|
||||
@@ -81,6 +93,8 @@ type Harness = {
|
||||
windows: FakeWindow[]
|
||||
clock: { value: number }
|
||||
activePageId: string | undefined
|
||||
pagesChanged: (string | undefined)[]
|
||||
certificateFailurePageIds: Set<string>
|
||||
}
|
||||
|
||||
function createHarness(
|
||||
@@ -88,9 +102,14 @@ function createHarness(
|
||||
pinned?: Set<string>
|
||||
activePageId?: string
|
||||
loadError?: { code: number; description: string; validatedUrl: string } | null
|
||||
certificateFailurePageIds?: string[]
|
||||
} = {}
|
||||
): Harness {
|
||||
const state = { activePageId: overrides.activePageId }
|
||||
const state = {
|
||||
activePageId: overrides.activePageId,
|
||||
certificateFailurePageIds: new Set<string>(overrides.certificateFailurePageIds ?? []),
|
||||
pagesChanged: [] as (string | undefined)[]
|
||||
}
|
||||
const order: string[] = []
|
||||
const registered = new Map<string, number>()
|
||||
const windows: FakeWindow[] = []
|
||||
@@ -118,7 +137,9 @@ function createHarness(
|
||||
registered.delete(browserPageId)
|
||||
},
|
||||
getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null,
|
||||
getBrowserPageLoadError: () => overrides.loadError ?? null
|
||||
getBrowserPageLoadError: () => overrides.loadError ?? null,
|
||||
getBrowserPageCertificateFailure: (browserPageId: string) =>
|
||||
state.certificateFailurePageIds.has(browserPageId) ? { challengeId: 'c' } : null
|
||||
} as unknown as BrowserManager
|
||||
|
||||
const bridge = {
|
||||
@@ -134,6 +155,7 @@ function createHarness(
|
||||
const backend = new OffscreenBrowserBackend(manager, {
|
||||
getAgentBrowserBridge: () => bridge,
|
||||
isPagePinned: (id) => overrides.pinned?.has(id) === true,
|
||||
onPagesChanged: (worktreeId) => state.pagesChanged.push(worktreeId),
|
||||
now: () => clock.value
|
||||
})
|
||||
|
||||
@@ -147,7 +169,11 @@ function createHarness(
|
||||
clock,
|
||||
set activePageId(value: string | undefined) {
|
||||
state.activePageId = value
|
||||
}
|
||||
},
|
||||
get pagesChanged(): (string | undefined)[] {
|
||||
return state.pagesChanged
|
||||
},
|
||||
certificateFailurePageIds: state.certificateFailurePageIds
|
||||
}
|
||||
}
|
||||
|
||||
@@ -353,6 +379,81 @@ describe('OffscreenBrowserBackend reclamation', () => {
|
||||
expect(await wake).toBe(true)
|
||||
})
|
||||
|
||||
it('does not let a slow close unregister a page that reused its id', async () => {
|
||||
// Why: closing drops the record before teardown finishes, so a caller
|
||||
// reusing the id could register a renderer the old close then unregisters,
|
||||
// leaving the new page unreachable by every command.
|
||||
let releaseTeardown = (): void => {}
|
||||
const h = createHarness()
|
||||
await h.backend.createTab({ url: 'https://a', browserPageId: 'a' })
|
||||
const bridge = h.bridge as unknown as { onTabClosed: ReturnType<typeof vi.fn> }
|
||||
bridge.onTabClosed.mockImplementationOnce(
|
||||
async () => new Promise<void>((resolve) => (releaseTeardown = resolve))
|
||||
)
|
||||
|
||||
const close = h.backend.closeTab('a')
|
||||
await Promise.resolve()
|
||||
const recreate = h.backend.createTab({ url: 'https://a2', browserPageId: 'a' })
|
||||
releaseTeardown()
|
||||
await close
|
||||
await recreate
|
||||
|
||||
expect(h.registered.get('a')).toBe(h.windows[1].webContents.id)
|
||||
expect(h.windows[0].isDestroyed()).toBe(true)
|
||||
expect(h.windows[1].isDestroyed()).toBe(false)
|
||||
})
|
||||
|
||||
it('tells the session snapshot when a parked page is closed', async () => {
|
||||
// Why: a parked close has no WebContents teardown to piggyback on, so
|
||||
// nothing else would tell paired clients the tab is gone.
|
||||
const h = createHarness()
|
||||
await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' })
|
||||
h.clock.value += 120_000
|
||||
await h.backend.reclaimIdlePages()
|
||||
h.pagesChanged.length = 0
|
||||
|
||||
await h.backend.closeTab('a')
|
||||
|
||||
expect(h.pagesChanged).toEqual(['wt-1'])
|
||||
})
|
||||
|
||||
it('does not park a page waiting on a certificate decision', async () => {
|
||||
// Why: the challenge id dies with the renderer, so parking would discard
|
||||
// both the warning and the ability to approve it.
|
||||
const h = createHarness({ certificateFailurePageIds: ['a'] })
|
||||
await h.backend.createTab({ url: 'https://a', browserPageId: 'a' })
|
||||
h.clock.value += 120_000
|
||||
|
||||
expect(await h.backend.reclaimIdlePages()).toEqual([])
|
||||
|
||||
h.certificateFailurePageIds.delete('a')
|
||||
expect(await h.backend.reclaimIdlePages()).toEqual(['a'])
|
||||
})
|
||||
|
||||
it('does not park a renderer whose navigation is still running after the load timeout', async () => {
|
||||
// Why: the load helper resolves on its own timeout, so `loading` stops
|
||||
// covering a navigation that is merely slow rather than finished.
|
||||
const h = createHarness()
|
||||
await h.backend.createTab({ url: 'https://a', browserPageId: 'a' })
|
||||
h.windows[0].__loading = true
|
||||
h.clock.value += 120_000
|
||||
|
||||
expect(await h.backend.reclaimIdlePages()).toEqual([])
|
||||
|
||||
h.windows[0].__loading = false
|
||||
expect(await h.backend.reclaimIdlePages()).toEqual(['a'])
|
||||
})
|
||||
|
||||
it('ignores a subframe in-page navigation when recording the address', async () => {
|
||||
const h = createHarness()
|
||||
await h.backend.createTab({ url: 'https://host.test/', browserPageId: 'a' })
|
||||
h.windows[0].navigateTo('https://frame.test/#x', false)
|
||||
h.clock.value += 120_000
|
||||
await h.backend.reclaimIdlePages()
|
||||
|
||||
expect(h.backend.listParkedPages()[0]?.url).toBe('https://host.test/')
|
||||
})
|
||||
|
||||
it('coalesces concurrent wakes into one renderer', async () => {
|
||||
const h = createHarness()
|
||||
await h.backend.createTab({ url: 'https://example.test/a' })
|
||||
|
||||
@@ -27,6 +27,8 @@ export type OffscreenBrowserBackendOptions = {
|
||||
getAgentBrowserBridge?: () => AgentBrowserBridge | null
|
||||
/** Pages a client is streaming or that have a command in flight. */
|
||||
isPagePinned?: (browserPageId: string) => boolean
|
||||
/** Called when the set of open pages changes, so paired clients republish. */
|
||||
onPagesChanged?: (worktreeId: string | undefined) => void
|
||||
now?: () => number
|
||||
}
|
||||
|
||||
@@ -50,6 +52,10 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
|
||||
async createTab(params: BrowserBackendCreateTab): Promise<{ browserPageId: string }> {
|
||||
const browserPageId = params.browserPageId ?? randomUUID()
|
||||
// Why: closing drops the record before its teardown finishes, so a caller
|
||||
// reusing the id can register a new renderer that the old close then
|
||||
// unregisters. Let the release finish before claiming the id again.
|
||||
await this.releasing.get(browserPageId)
|
||||
if (this.pages.has(browserPageId)) {
|
||||
throw new Error(`Browser page ${browserPageId} already exists`)
|
||||
}
|
||||
@@ -104,6 +110,12 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
if (this.pages.size === 0) {
|
||||
this.stopSweep()
|
||||
}
|
||||
// Why: closing a parked page has no WebContents teardown to piggyback on,
|
||||
// so nothing else tells paired clients the tab is gone — they would keep
|
||||
// showing a ghost until an operation against it failed.
|
||||
if (page) {
|
||||
this.options.onPagesChanged?.(page.worktreeId)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -250,11 +262,30 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
private isPagePinned(page: OffscreenBrowserPage): boolean {
|
||||
return (
|
||||
page.loading ||
|
||||
this.isNavigating(page) ||
|
||||
this.waking.has(page.browserPageId) ||
|
||||
// Why: a page blocked on a certificate decision is work waiting on an
|
||||
// answer. Its challenge id dies with the renderer, so parking it would
|
||||
// discard both the warning and the ability to approve it.
|
||||
this.browserManager.getBrowserPageCertificateFailure(page.browserPageId) !== null ||
|
||||
this.options.isPagePinned?.(page.browserPageId) === true
|
||||
)
|
||||
}
|
||||
|
||||
// Why: the load helper resolves on its own timeout, so `loading` stops
|
||||
// covering a navigation that is slow rather than finished. Ask the renderer.
|
||||
private isNavigating(page: OffscreenBrowserPage): boolean {
|
||||
const win = page.window
|
||||
if (!win || win.isDestroyed()) {
|
||||
return false
|
||||
}
|
||||
try {
|
||||
return win.webContents.isLoading()
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
private isSafeToReclaim(browserPageId: string): boolean {
|
||||
const page = this.pages.get(browserPageId)
|
||||
if (!page || this.releasing.has(browserPageId)) {
|
||||
@@ -352,13 +383,21 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
// 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 => {
|
||||
const recordAddress = (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)
|
||||
// did-navigate is main-frame only; did-frame-navigate covers subframes.
|
||||
win.webContents.on('did-navigate', (_event, url) => recordAddress(url))
|
||||
// Why: an iframe changing its own hash also fires did-navigate-in-page. A
|
||||
// subframe navigation is not a navigation of the tab, and adopting its
|
||||
// address would wake the page onto the iframe's document.
|
||||
win.webContents.on('did-navigate-in-page', (_event, url, isMainFrame) => {
|
||||
if (isMainFrame) {
|
||||
recordAddress(url)
|
||||
}
|
||||
})
|
||||
// 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.
|
||||
|
||||
+2
-1
@@ -3168,7 +3168,8 @@ void app.whenReady().then(async () => {
|
||||
runtime.setOffscreenBrowserBackend(
|
||||
new OffscreenBrowserBackend(browserManager, {
|
||||
getAgentBrowserBridge: () => runtime?.getAgentBrowserBridge() ?? null,
|
||||
isPagePinned: (browserPageId) => runtime?.isBrowserPagePinned(browserPageId) === true
|
||||
isPagePinned: (browserPageId) => runtime?.isBrowserPagePinned(browserPageId) === true,
|
||||
onPagesChanged: (worktreeId) => runtime?.notifyHeadlessBrowserPagesChanged(worktreeId)
|
||||
})
|
||||
)
|
||||
}
|
||||
|
||||
@@ -163,6 +163,19 @@ describe('headless parked-page targeting', () => {
|
||||
])
|
||||
})
|
||||
|
||||
it('reports back the index the caller passed, not the renumbered one', async () => {
|
||||
// Why: waking the target makes it live, which renumbers the bridge listing.
|
||||
// Reporting that position would describe neither the request nor the
|
||||
// listing the caller read.
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
const fake = createFakeHeadlessHost(['live-a'], ['parked-b', 'parked-c'])
|
||||
const commands = new RuntimeBrowserCommands(fake.host)
|
||||
|
||||
const result = await commands.browserTabSwitch({ index: 2, worktree: 'id:wt-1' })
|
||||
|
||||
expect(result).toMatchObject({ switched: 2, browserPageId: 'parked-c' })
|
||||
})
|
||||
|
||||
it('counts an indexed switch of a resident page as using it', async () => {
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
const fake = createFakeHeadlessHost(['live-a', 'live-b'], ['parked-c'])
|
||||
|
||||
@@ -757,11 +757,17 @@ export class RuntimeBrowserCommands {
|
||||
// or `tab switch --index 0` selects a different tab than the one it showed.
|
||||
const indexed = await this.resolveIndexedTargetBeforeWake(params)
|
||||
const target = indexed ?? (await this.resolveBrowserCommandTarget(params))
|
||||
const result = await bridge.tabSwitch(
|
||||
const switched = await bridge.tabSwitch(
|
||||
indexed ? undefined : params.index,
|
||||
target.worktreeId,
|
||||
target.browserPageId
|
||||
)
|
||||
// Why (STA-4341): waking the target makes it live, which renumbers the
|
||||
// bridge's own listing. Report the index the caller passed — the one the
|
||||
// listing they read actually names — rather than a position that describes
|
||||
// neither their request nor what they were looking at.
|
||||
const result =
|
||||
indexed && params.index !== undefined ? { ...switched, switched: params.index } : switched
|
||||
if (params.focus) {
|
||||
// Why: scope focus to the tab's owning worktree; the renderer never yanks the user across worktrees on this signal (see focusBrowserTabInWorktree).
|
||||
const worktreeId =
|
||||
|
||||
@@ -6608,6 +6608,14 @@ export class OrcaRuntimeService {
|
||||
return this.offscreenBrowserBackend
|
||||
}
|
||||
|
||||
// Why (STA-4341): closing a parked page has no WebContents teardown to
|
||||
// piggyback on, so the session snapshot only learns about it through here.
|
||||
notifyHeadlessBrowserPagesChanged(worktreeId: string | undefined): void {
|
||||
if (worktreeId) {
|
||||
this.notifyMobileSessionTabsChanged(worktreeId)
|
||||
}
|
||||
}
|
||||
|
||||
// Why (STA-4341): the headless reclaimer asks the runtime before parking a
|
||||
// page, so a streamed or mid-command page keeps its renderer.
|
||||
isBrowserPagePinned(browserPageId: string): boolean {
|
||||
|
||||
Reference in New Issue
Block a user