diff --git a/src/main/browser/browser-manager-guest-navigation-policy.ts b/src/main/browser/browser-manager-guest-navigation-policy.ts index d34892206b1..1974dc2975c 100644 --- a/src/main/browser/browser-manager-guest-navigation-policy.ts +++ b/src/main/browser/browser-manager-guest-navigation-policy.ts @@ -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) } diff --git a/src/main/browser/browser-manager-load-failure-replay.test.ts b/src/main/browser/browser-manager-load-failure-replay.test.ts index 9cb281a83e2..555e63274f1 100644 --- a/src/main/browser/browser-manager-load-failure-replay.test.ts +++ b/src/main/browser/browser-manager-load-failure-replay.test.ts @@ -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) => ({ + 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) }