fix(browser): scope same-document settlement to the navigation that owns it

Re-review of the previous commit found two defects it introduced, neither
covered by its guards.

The in-page handler deleted pendingNavigationByGuestId unconditionally. Since
an in-place start no longer creates that record, anything present belongs to an
overlapping full navigation -- whose target resolveTabNavigationUrl and
failPendingNavigation still resolve against for UA and failure handling. An
in-page commit from the outgoing document could therefore erase the incoming
navigation's target. The isInPlace start-path change alone fixes the leak it
was meant to fix, so the delete is gone rather than made conditional.

The isInPlace early return also skipped more than pending-target creation: it
skipped load-error and certificate settlement. A failed load followed by an
in-page route left loadErrorsByGuestId set, and AgentBrowserBridge.tabList
publishes loadError.validatedUrl ahead of getURL(), so the tab kept publishing
the failed target as its url. Settlement now happens on the same-document
commit, matching the local webview precedent, but only when no full navigation
is in flight -- guest-global clearing would settle the wrong transaction.

Guarded both directions, and both ablate: making the settle unconditional
reddens the overlap case, removing it reddens the standalone case. The
detachment test now also asserts the in-page listener is among those attached,
so it can no longer pass vacuously.
This commit is contained in:
Merge Sim
2026-09-12 01:03:47 -07:00
parent cda685e89f
commit 1837bb024a
2 changed files with 83 additions and 5 deletions
@@ -132,16 +132,26 @@ export abstract class BrowserManagerGuestNavigationPolicy extends BrowserManager
const didNavigateInPageHandler = (
_event: Electron.Event,
_url: string,
url: string,
isMainFrame: boolean
): void => {
if (!isMainFrame) {
return
}
// Why: Electron does not emit 'did-navigate' for a same-document commit, so without this a
// pushed host snapshot keeps the pre-navigation url while a pull reads the new one — the same
// stale-push class this policy's committed-navigation notify fixes for full navigations.
this.pendingNavigationByGuestId.delete(guest.id)
// Why the pending record is left alone: an in-place start never creates one, so anything
// here belongs to an overlapping full navigation whose target the UA and failure paths still
// resolve against. Deleting it would settle the wrong navigation.
if (!this.pendingNavigationByGuestId.has(guest.id)) {
// Why settle only with no full navigation in flight: a same-document commit replaces this
// document's own failure state, and tabList publishes loadError.validatedUrl ahead of
// getURL(), so a stale failure would keep republishing the failed target as the tab's url.
this.loadErrorsByGuestId.delete(guest.id)
this.clearedLoadErrorsByGuestId.delete(guest.id)
this.certificateTrustController?.onMainFrameNavigationCommitted(guest.id, url)
}
// Why notify regardless: Electron emits no 'did-navigate' for a same-document commit, so
// otherwise the pushed host snapshot keeps the pre-navigation url while a pull reads the new
// one — the same stale-push class the committed-navigation notify fixes for full navigations.
this.notifyBrowserGuestStateChanged(guest.id)
}
@@ -123,6 +123,71 @@ describe('browserManager', () => {
expect(stateChanged).toHaveBeenCalledWith('remote-worktree')
})
it('settles a stale failure on a standalone same-document commit, but not across a full navigation', () => {
const mk = (id: number) => ({
id,
isDestroyed: vi.fn(() => false),
getType: vi.fn(() => 'window'),
setBackgroundThrottling: vi.fn(),
setWindowOpenHandler: vi.fn(),
on: vi.fn(),
off: vi.fn(),
getURL: vi.fn(() => 'https://spa.test/app'),
getUserAgent: vi.fn(() => 'Mozilla/5.0 Chrome/140.0.0.0'),
setUserAgent: vi.fn()
})
const handlers = (guest: ReturnType<typeof mk>) => ({
fail: guest.on.mock.calls.find(([e]) => e === 'did-fail-load')?.[1] as (
...a: unknown[]
) => void,
start: guest.on.mock.calls.find(([e]) => e === 'did-start-navigation')?.[1] as (
...a: unknown[]
) => void,
inPage: guest.on.mock.calls.find(([e]) => e === 'did-navigate-in-page')?.[1] as (
...a: unknown[]
) => void
})
// Standalone: the surviving document routes in place, so its own stale failure must go —
// tabList publishes loadError.validatedUrl ahead of getURL() and would republish the failure.
const alone = mk(701)
webContentsFromIdMock.mockReturnValue(alone)
browserManager.registerOffscreenGuest({
browserPageId: 'spa-alone',
worktreeId: 'remote-worktree',
webContentsId: alone.id
})
const a = handlers(alone)
a.fail(null, -105, 'Name not resolved', 'https://missing.test/', true)
expect(browserManager.getBrowserPageLoadError('spa-alone')).not.toBeNull()
a.inPage(null, 'https://spa.test/app#route', true)
expect(browserManager.getBrowserPageLoadError('spa-alone')).toBeNull()
// Overlapping: a full navigation is in flight and owns the transaction. An in-page commit from
// the still-live old document must not settle it, or the wrong navigation gets resolved.
const overlap = mk(702)
webContentsFromIdMock.mockReturnValue(overlap)
browserManager.registerOffscreenGuest({
browserPageId: 'spa-overlap',
worktreeId: 'remote-worktree',
webContentsId: overlap.id
})
const o = handlers(overlap)
o.fail(null, -105, 'Name not resolved', 'https://missing.test/', true)
// A full navigation start stashes the visible error so an abort can restore it.
o.start(null, 'https://elsewhere.test/', false, true)
expect(browserManager.getBrowserPageLoadError('spa-overlap')).toBeNull()
o.inPage(null, 'https://spa.test/app#route', true)
// The stash must survive: the in-page commit belongs to the outgoing document, not to the
// full navigation that owns it. Aborting the full navigation then restores the real error.
o.fail(null, -3, 'Aborted', 'https://elsewhere.test/', true)
expect(browserManager.getBrowserPageLoadError('spa-overlap')).toEqual({
code: -105,
description: 'Name not resolved',
validatedUrl: 'https://missing.test/'
})
})
it('detaches the same-document navigation listener with the rest of the policy', () => {
const guest = {
id: 608,
@@ -146,6 +211,9 @@ describe('browserManager', () => {
// Why assert the pairing: a listener added without a matching off() outlives its guest.
const attached = guest.on.mock.calls.map(([event]) => event).filter((e) => e !== 'destroyed')
const detached = new Set(guest.off.mock.calls.map(([event]) => event))
// Why assert membership too: iterating an empty or in-page-less list would pass vacuously and
// stop protecting the very listener this commit adds.
expect(attached).toContain('did-navigate-in-page')
for (const event of attached) {
expect(detached.has(event), `${event} was attached but never detached`).toBe(true)
}