diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index b93f75db589..30adfe83f75 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -2902,6 +2902,14 @@ describe('browserManager', () => { const GUEST_CLEAN_UA = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/134.0.0.0 Safari/537.36' + // Why: viewport UA writes are queued on the per-tab chain, so draining it takes more than one + // microtask hop; loop until the chain is empty rather than guessing a tick count. + async function flushViewportOps(): Promise { + for (let i = 0; i < 20; i++) { + await Promise.resolve() + } + } + function makeGuest( id: number, url = 'https://example.com/' @@ -2911,16 +2919,19 @@ describe('browserManager', () => { debuggerIsAttached: ReturnType debuggerAttach: ReturnType setGuestUserAgent: (ua: string) => void + commitNavigationTo: (nextUrl: string) => void } { const debuggerSendCommand = vi.fn().mockResolvedValue(undefined) const debuggerIsAttached = vi.fn(() => true) const debuggerAttach = vi.fn() let currentUa = GUEST_ELECTRON_UA + // Why: getURL() reports the last COMMITTED url — it does not move at did-start-navigation. + let committedUrl = url const guest = { id, isDestroyed: vi.fn(() => false), getType: vi.fn(() => 'webview'), - getURL: vi.fn(() => url), + getURL: vi.fn(() => committedUrl), getUserAgent: vi.fn(() => currentUa), setUserAgent: vi.fn((ua: string) => { currentUa = ua @@ -2945,6 +2956,9 @@ describe('browserManager', () => { debuggerAttach, setGuestUserAgent: (ua: string) => { currentUa = ua + }, + commitNavigationTo: (nextUrl: string) => { + committedUrl = nextUrl } } } @@ -3091,7 +3105,7 @@ describe('browserManager', () => { // Navigating to the auth host must move the standing override to the Firefox identity. debuggerSendCommand.mockClear() didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) - await Promise.resolve() + await flushViewportOps() expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { userAgent: googleAuthUserAgent() }) @@ -3099,12 +3113,198 @@ describe('browserManager', () => { // Leaving the auth host restores the clean Chrome-shaped preset UA. debuggerSendCommand.mockClear() didStartNavigation(null, 'https://example.com/', false, true) - await Promise.resolve() + await flushViewportOps() expect(debuggerSendCommand).toHaveBeenCalledWith('Emulation.setUserAgentOverride', { userAgent: GUEST_CLEAN_UA }) }) + // Why: setViewportOverride and the navigation path are two independent writers of the same CDP + // command. Verified against real Electron 43.1.0: did-start-navigation fires while an awaited + // sendCommand is still pending, and getURL() keeps reporting the OUTGOING page until commit — + // so a preset resuming mid-navigation resolves the wrong host unless both writers share a chain + // and a single URL source. + function lastUserAgentOverride( + debuggerSendCommand: ReturnType + ): Record | undefined { + const calls = debuggerSendCommand.mock.calls.filter( + (call) => call[0] === 'Emulation.setUserAgentOverride' + ) + return calls.at(-1)?.[1] as Record | undefined + } + + // Why mobile: on the desktop branch the break is masked by coincidence — applyGoogleAuthUserAgent + // has already switched the WebContents UA to Firefox, and cleanElectronUserAgent passes a Firefox + // UA through untouched, so the stale-URL desktop path happens to emit Firefox anyway. The mobile + // branch derives a Chrome-shaped iPhone UA from that same base and exposes the real defect. + it('does not leave the Chrome preset UA standing when a mobile preset lands mid-navigation onto an auth host', async () => { + const { guest, debuggerSendCommand } = makeGuest(4251, 'https://example.com/') + // Hold the preset's first CDP command open so the navigation lands inside its await window. + let releaseMetrics = (): void => {} + const metricsGate = new Promise((resolve) => { + releaseMetrics = () => resolve() + }) + debuggerSendCommand.mockImplementation((method: string) => + method === 'Emulation.setDeviceMetricsOverride' ? metricsGate : Promise.resolve(undefined) + ) + webContentsFromIdMock.mockReturnValue(guest) + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'tab-race-onto-auth', + webContentsId: guest.id as number, + rendererWebContentsId + }) + const didStartNavigation = guestOnMock.mock.calls.find( + ([event]) => event === 'did-start-navigation' + )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void + + const presetDone = browserManager.setViewportOverride('tab-race-onto-auth', { + width: 375, + height: 667, + deviceScaleFactor: 2, + mobile: true + }) + await flushViewportOps() + + // The tab navigates to the auth host while the preset is still awaiting its first command. + didStartNavigation(null, 'https://accounts.google.com/v3/signin/identifier', false, true) + releaseMetrics() + await presetDone + await flushViewportOps() + + // Without a shared chain + shared URL source, the preset resumes, reads getURL() as the + // pre-navigation host, and pins the tab to the Chrome-shaped UA while it is on the auth host. + expect(lastUserAgentOverride(debuggerSendCommand)).toEqual({ + userAgent: googleAuthUserAgent() + }) + }) + + it('does not leave the Firefox UA standing when a preset lands mid-navigation off an auth host', async () => { + const { guest, debuggerSendCommand } = makeGuest(4252, 'https://accounts.google.com/') + webContentsFromIdMock.mockReturnValue(guest) + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'tab-race-off-auth', + webContentsId: guest.id as number, + rendererWebContentsId + }) + const didStartNavigation = guestOnMock.mock.calls.find( + ([event]) => event === 'did-start-navigation' + )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void + + // Establish a standing preset while the tab really is on the auth host. + await browserManager.setViewportOverride('tab-race-off-auth', { + width: 1024, + height: 768, + deviceScaleFactor: 1, + mobile: false + }) + await flushViewportOps() + expect(lastUserAgentOverride(debuggerSendCommand)).toEqual({ + userAgent: googleAuthUserAgent() + }) + + // A second preset change is now in flight when the tab leaves the auth host. + let releaseMetrics = (): void => {} + const metricsGate = new Promise((resolve) => { + releaseMetrics = () => resolve() + }) + debuggerSendCommand.mockImplementation((method: string) => + method === 'Emulation.setDeviceMetricsOverride' ? metricsGate : Promise.resolve(undefined) + ) + const presetDone = browserManager.setViewportOverride('tab-race-off-auth', { + width: 1440, + height: 900, + deviceScaleFactor: 2, + mobile: false + }) + await flushViewportOps() + + didStartNavigation(null, 'https://example.com/', false, true) + releaseMetrics() + await presetDone + await flushViewportOps() + + // Without the fix the resuming preset re-reads getURL() as the auth host and clobbers the + // navigation's correct write, stranding the Firefox UA on a non-auth page. + expect(lastUserAgentOverride(debuggerSendCommand)).toEqual({ userAgent: GUEST_CLEAN_UA }) + }) + + it('falls back to the committed URL once a navigation commits or fails', async () => { + const { guest, debuggerSendCommand, commitNavigationTo } = makeGuest( + 4253, + 'https://example.com/' + ) + webContentsFromIdMock.mockReturnValue(guest) + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'tab-pending-cleared', + webContentsId: guest.id as number, + rendererWebContentsId + }) + const didStartNavigation = guestOnMock.mock.calls.find( + ([event]) => event === 'did-start-navigation' + )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void + const didFailLoad = guestOnMock.mock.calls.find( + ([event]) => event === 'did-fail-load' + )?.[1] as ( + event: unknown, + errorCode: number, + errorDescription: string, + validatedURL: string, + isMainFrame: boolean + ) => void + const didNavigate = guestOnMock.mock.calls.find( + ([event]) => event === 'did-navigate' + )?.[1] as (event: unknown, url: string) => void + + await browserManager.setViewportOverride('tab-pending-cleared', { + width: 1024, + height: 768, + deviceScaleFactor: 1, + mobile: false + }) + await flushViewportOps() + + // An aborted navigation to the auth host must not leave that host standing as the tab's + // identity: the tab never went there, so a later preset must resolve the committed URL. + didStartNavigation(null, 'https://accounts.google.com/', false, true) + didFailLoad(null, -3, 'Aborted', 'https://accounts.google.com/', true) + await flushViewportOps() + + // A mobile preset discriminates cleanly here: the auth branch would emit the Firefox UA, the + // non-auth branch an iPhone UA. Asserting the desktop branch instead would conflate this with + // applyGoogleAuthUserAgent leaving the WebContents UA on Firefox after an aborted auth nav, + // which is pre-existing behavior and out of scope for the ordering fix. + debuggerSendCommand.mockClear() + await browserManager.setViewportOverride('tab-pending-cleared', { + width: 375, + height: 667, + deviceScaleFactor: 2, + mobile: true + }) + await flushViewportOps() + expect(lastUserAgentOverride(debuggerSendCommand)?.userAgent).toContain('iPhone') + + // Same after a successful commit: the committed URL takes over from the pending target. + didStartNavigation(null, 'https://accounts.google.com/signin', false, true) + commitNavigationTo('https://accounts.google.com/signin') + didNavigate(null, 'https://accounts.google.com/signin') + await flushViewportOps() + + debuggerSendCommand.mockClear() + await browserManager.setViewportOverride('tab-pending-cleared', { + width: 1024, + height: 768, + deviceScaleFactor: 1, + mobile: false + }) + await flushViewportOps() + expect(lastUserAgentOverride(debuggerSendCommand)).toEqual({ + userAgent: googleAuthUserAgent() + }) + }) + it('does not touch the UA override on navigation when no preset is standing', async () => { const { guest, debuggerSendCommand } = makeGuest(4248) webContentsFromIdMock.mockReturnValue(guest) @@ -3119,7 +3319,7 @@ describe('browserManager', () => { )?.[1] as (event: unknown, url: string, isInPlace: boolean, isMainFrame: boolean) => void didStartNavigation(null, 'https://accounts.google.com/', false, true) - await Promise.resolve() + await flushViewportOps() expect(debuggerSendCommand).not.toHaveBeenCalledWith( 'Emulation.setUserAgentOverride', expect.anything() @@ -3149,7 +3349,7 @@ describe('browserManager', () => { debuggerSendCommand.mockClear() didStartNavigation(null, 'https://accounts.google.com/', false, true) - await Promise.resolve() + await flushViewportOps() expect(debuggerSendCommand).not.toHaveBeenCalledWith( 'Emulation.setUserAgentOverride', expect.anything() @@ -3179,7 +3379,7 @@ describe('browserManager', () => { }) debuggerSendCommand.mockClear() didStartNavigation(null, 'https://accounts.google.com/', false, true) - await Promise.resolve() + await flushViewportOps() expect(debuggerSendCommand).not.toHaveBeenCalledWith( 'Emulation.setUserAgentOverride', expect.anything() diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index b522eaa1deb..54bc447ce82 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -226,6 +226,9 @@ export class BrowserManager { // Why: presence means a CDP UA override is standing for the tab (value = the preset's mobile flag), // so navigation can re-issue it against the target URL's identity. private readonly viewportUaOverrideMobileByTabId = new Map() + // Why: the in-flight main-frame navigation target, held only until commit or failure — getURL() + // still reports the outgoing page until then. See resolveTabNavigationUrl. + private readonly pendingNavigationUrlByTabId = new Map() private readonly contextMenuCleanupByTabId = new Map void>() private readonly grabShortcutCleanupByTabId = new Map void>() private readonly shortcutForwardingCleanupByTabId = new Map void>() @@ -802,6 +805,8 @@ export class BrowserManager { if (!isMainFrame) { return } + // Why: a nav that never committed must not leave its target standing as the tab's host. + this.clearPendingNavigationUrl(guest.id) const browserPageId = this.tabIdByWebContentsId.get(guest.id) const certificateFailure = browserPageId ? this.certificateTrustController?.getFailure(browserPageId) @@ -845,6 +850,12 @@ export class BrowserManager { if (!isMainFrame || isChromiumInternalErrorUrl(url)) { return } + const navigatingTabId = this.tabIdByWebContentsId.get(guest.id) + if (navigatingTabId) { + // Why: getURL() still reports the previous committed URL until this navigation commits, so + // every UA writer must read the in-flight target or they disagree about the tab's host. + this.pendingNavigationUrlByTabId.set(navigatingTabId, url) + } this.applyGoogleAuthUserAgent(guest, url) this.certificateTrustController?.onMainFrameNavigationStarted(guest.id) // Why: a pre-registration failure belongs only to its own nav; a replacement nav must not replay it. @@ -861,6 +872,8 @@ export class BrowserManager { } const didNavigateHandler = (_event: Electron.Event, url: string): void => { + // Why: once committed, getURL() reports this url, so the pending target is redundant. + this.clearPendingNavigationUrl(guest.id) // Why: a committed nav makes the did-start-navigation stash obsolete; drop it so a later ERR_ABORTED can't restore an error over it. this.clearedLoadErrorsByGuestId.delete(guest.id) this.certificateTrustController?.onMainFrameNavigationCommitted(guest.id, url) @@ -932,39 +945,78 @@ export class BrowserManager { guest.setUserAgent(guest.session.getUserAgent()) } if (browserPageId) { - this.reapplyViewportUserAgentOverride(guest, browserPageId, url) + this.reapplyViewportUserAgentOverride(guest, browserPageId) } } + private clearPendingNavigationUrl(guestId: number): void { + const browserTabId = this.tabIdByWebContentsId.get(guestId) + if (browserTabId) { + this.pendingNavigationUrlByTabId.delete(browserTabId) + } + } + + // Why: webContents.getURL() reports the last COMMITTED url, so mid-navigation it names the host + // the tab is leaving, not the one it is entering. Every UA writer must resolve the host through + // here or two writers racing the same navigation will pick opposite identities. + private resolveTabNavigationUrl(guest: Electron.WebContents, browserTabId: string): string { + return this.pendingNavigationUrlByTabId.get(browserTabId) ?? guest.getURL() + } + + // Why: setViewportOverride and the navigation path both write Emulation.setUserAgentOverride for + // the same tab. Sharing one chain makes the last-enqueued write the one that lands; without it the + // two interleave and the tab can keep the Firefox UA off the auth hosts, or the Chrome-shaped + // preset UA while on them — the exact mismatch this scope exists to remove. + private enqueueViewportOp(browserTabId: string, op: () => Promise): Promise { + const prev = this.viewportOpsByTabId.get(browserTabId) ?? Promise.resolve() + const next = prev.catch(() => {}).then(op) + this.viewportOpsByTabId.set(browserTabId, next) + // Why: only clear if we're still the tail; a later call may have replaced the entry, and deleting would break serialization. + void next + .catch(() => {}) + .then(() => { + if (this.viewportOpsByTabId.get(browserTabId) === next) { + this.viewportOpsByTabId.delete(browserTabId) + } + }) + return next + } + // Why: Emulation.setUserAgentOverride is set once and stands across every later navigation, // outranking setUserAgent for navigator.userAgent. A viewport preset applied before reaching an // auth host would otherwise pin navigator.userAgent to the Chrome-shaped preset UA while the // request header says Firefox — the two-layer disagreement this scope exists to remove. private reapplyViewportUserAgentOverride( guest: Electron.WebContents, - browserTabId: string, - url: string + browserTabId: string ): void { + // Why: enqueue unconditionally — a setViewportOverride already queued ahead may be about to + // install (or clear) the override, so the standing-preset check belongs inside the chain. + void this.enqueueViewportOp(browserTabId, () => + this.sendViewportUserAgentOverride(guest, browserTabId) + ).catch(() => {}) + } + + private async sendViewportUserAgentOverride( + guest: Electron.WebContents, + browserTabId: string + ): Promise { const mobile = this.viewportUaOverrideMobileByTabId.get(browserTabId) if (mobile === undefined) { return } try { - if (!guest.debugger.isAttached()) { + if (guest.isDestroyed() || !guest.debugger.isAttached()) { return } - // Why: navigator.userAgent is read by page JS after commit, so landing this override - // asynchronously still beats every reader; the header layer already forces Firefox on the wire. - void guest.debugger - .sendCommand( - 'Emulation.setUserAgentOverride', - buildViewportUserAgentOverride({ - url, - mobile, - baseUserAgent: cleanElectronUserAgent(guest.getUserAgent()) - }) - ) - .catch(() => {}) + await guest.debugger.sendCommand( + 'Emulation.setUserAgentOverride', + buildViewportUserAgentOverride({ + url: this.resolveTabNavigationUrl(guest, browserTabId), + mobile, + baseUserAgent: cleanElectronUserAgent(guest.getUserAgent()) + }) + ) } catch { // guest may be mid-teardown or the debugger taken over by DevTools } @@ -1152,6 +1204,7 @@ export class BrowserManager { // Why: drop the viewport-op chain so the Map doesn't retain a promise keyed to a destroyed guest. this.viewportOpsByTabId.delete(browserTabId) this.viewportUaOverrideMobileByTabId.delete(browserTabId) + this.pendingNavigationUrlByTabId.delete(browserTabId) this.annotationViewportBridgeOpsByTabId.delete(browserTabId) } @@ -1218,6 +1271,7 @@ export class BrowserManager { this.sessionProfileIdByPageId.clear() this.userAgentModeByPageId.clear() this.viewportUaOverrideMobileByTabId.clear() + this.pendingNavigationUrlByTabId.clear() this.pendingLoadFailuresByGuestId.clear() this.loadErrorsByGuestId.clear() this.clearedLoadErrorsByGuestId.clear() @@ -1505,20 +1559,11 @@ export class BrowserManager { browserTabId: string, override: BrowserViewportOverride | null ): Promise { - // Why: chain per-tab so rapid toggles don't interleave CDP commands and the last-requested override wins. - const prev = this.viewportOpsByTabId.get(browserTabId) ?? Promise.resolve() - const next = prev - .catch(() => {}) - .then(() => this.doSetViewportOverrideImpl(browserTabId, override)) - this.viewportOpsByTabId.set(browserTabId, next) - try { - return await next - } finally { - // Why: only clear if we're still the tail; a later call may have replaced the entry, and deleting would break serialization. - if (this.viewportOpsByTabId.get(browserTabId) === next) { - this.viewportOpsByTabId.delete(browserTabId) - } - } + // Why: chain per-tab so rapid toggles — and navigations racing them — don't interleave CDP + // commands; the last-enqueued write wins. + return this.enqueueViewportOp(browserTabId, () => + this.doSetViewportOverrideImpl(browserTabId, override) + ) } async setAnnotationViewportBridge( @@ -1614,14 +1659,8 @@ export class BrowserManager { // Why: remember the standing override so later navigations can re-issue it; it outranks // setUserAgent and would otherwise pin the auth-host identity to whatever was set here. this.viewportUaOverrideMobileByTabId.set(browserTabId, override.mobile) - await dbg.sendCommand( - 'Emulation.setUserAgentOverride', - buildViewportUserAgentOverride({ - url: guest.getURL(), - mobile: override.mobile, - baseUserAgent: cleanElectronUserAgent(guest.getUserAgent()) - }) - ) + // Why: same sender as the navigation path, so both resolve the tab's host identically. + await this.sendViewportUserAgentOverride(guest, browserTabId) } } else { this.viewportUaOverrideMobileByTabId.delete(browserTabId)