From 6deae967162159b42e816ca49aaee7c040a0c636 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sat, 12 Sep 2026 15:19:41 -0700 Subject: [PATCH 1/7] browser: preserve popup state across IPC handoff Reconcile native navigation state after dynamic event subscription and reject stale snapshots or already-included events. Preserve newer per-field updates, editor identity, and closure coupling, with deterministic model and production workbench/IPC coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browserView/common/browserView.ts | 12 +- .../browserView/electron-main/browserView.ts | 12 +- .../contrib/browserView/common/browserView.ts | 170 ++++++--- .../browserViewWorkbenchService.ts | 2 +- .../test/common/browserView.test.ts | 3 + .../test/common/browserViewModelState.test.ts | 346 +++++++++++++++++ .../browserEditorInput.test.ts | 1 + .../electron-browser/browserViewModel.test.ts | 2 + .../browserViewWorkbenchService.test.ts | 355 ++++++++++++++++++ .../browserAutoReloadFeatures.test.ts | 2 + 10 files changed, 846 insertions(+), 59 deletions(-) create mode 100644 src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts create mode 100644 src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts diff --git a/src/vs/platform/browserView/common/browserView.ts b/src/vs/platform/browserView/common/browserView.ts index d9505fe64fcabd..2fc25d06474140 100644 --- a/src/vs/platform/browserView/common/browserView.ts +++ b/src/vs/platform/browserView/common/browserView.ts @@ -324,6 +324,8 @@ export interface IBrowserViewStorageKeys { } export interface IBrowserViewState { + /** Monotonic version shared by navigation, title, loading, and favicon updates. */ + navigationStateVersion: number; url: string; title: string; canGoBack: boolean; @@ -348,6 +350,7 @@ export interface IBrowserViewState { } export interface IBrowserViewNavigationEvent { + navigationStateVersion: number; url: string; title: string; canGoBack: boolean; @@ -356,6 +359,7 @@ export interface IBrowserViewNavigationEvent { } export interface IBrowserViewLoadingEvent { + navigationStateVersion: number; loading: boolean; error?: IBrowserViewLoadError; } @@ -403,10 +407,12 @@ export interface IBrowserViewKeyDownEvent { } export interface IBrowserViewTitleChangeEvent { + navigationStateVersion: number; title: string; } export interface IBrowserViewFaviconChangeEvent { + navigationStateVersion: number; favicon: string | undefined; } @@ -560,10 +566,8 @@ export interface IBrowserViewService { setOwner(id: string, owner: IBrowserViewOwner): Promise; /** - * Get the state of an existing browser view by ID, or throw if it doesn't exist - * @param id The browser view identifier - * @return The state of the browser view for the given ID - * @throws If no browser view exists for the given ID + * Get the current state, or throw if the view doesn't exist. + * Subscribe before reading and compare navigationStateVersion when reconciling navigation-related events. */ getState(id: string): Promise; diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index 6831bae3194932..9a74729ce33aa0 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -41,6 +41,7 @@ export class BrowserView extends Disposable { private _lastScreenshot: VSBuffer | undefined = undefined; private _lastFavicon: string | undefined = undefined; private _lastError: IBrowserViewLoadError | undefined = undefined; + private _navigationStateVersion = 0; private _lastUserGestureTimestamp: number = -Infinity; private _browserZoomIndex: number = browserZoomDefaultIndex; @@ -309,7 +310,7 @@ export class BrowserView extends Disposable { try { this._lastFavicon = await this._faviconRequestCache.get(url)!; - this._onDidChangeFavicon.fire({ favicon: this._lastFavicon }); + this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); this._currentHistoryHandle?.update({ favicon: this._lastFavicon }); // On success, stop searching return; @@ -321,7 +322,7 @@ export class BrowserView extends Disposable { // If we searched all favicons and none worked, clear the favicon if (this._lastFavicon) { this._lastFavicon = undefined; - this._onDidChangeFavicon.fire({ favicon: this._lastFavicon }); + this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); this._currentHistoryHandle?.update({ favicon: null }); } }); @@ -345,12 +346,13 @@ export class BrowserView extends Disposable { // Title events webContents.on('page-title-updated', (_event, title) => { - this._onDidChangeTitle.fire({ title }); + this._onDidChangeTitle.fire({ navigationStateVersion: ++this._navigationStateVersion, title }); this._currentHistoryHandle?.update({ title }); }); const fireNavigationEvent = (url: string) => { this._onDidNavigate.fire({ + navigationStateVersion: ++this._navigationStateVersion, url, title: webContents.getTitle(), canGoBack: webContents.navigationHistory.canGoBack(), @@ -361,7 +363,7 @@ export class BrowserView extends Disposable { }; const fireLoadingEvent = (loading: boolean) => { - this._onDidChangeLoadingState.fire({ loading, error: this._lastError }); + this._onDidChangeLoadingState.fire({ navigationStateVersion: ++this._navigationStateVersion, loading, error: this._lastError }); }; // Loading state events @@ -392,6 +394,7 @@ export class BrowserView extends Disposable { fireLoadingEvent(false); this._onDidNavigate.fire({ + navigationStateVersion: ++this._navigationStateVersion, url: validatedURL, title: '', canGoBack: webContents.navigationHistory.canGoBack(), @@ -617,6 +620,7 @@ export class BrowserView extends Disposable { const url = webContents.getURL(); return { + navigationStateVersion: this._navigationStateVersion, url, title: webContents.getTitle(), canGoBack: webContents.navigationHistory.canGoBack(), diff --git a/src/vs/workbench/contrib/browserView/common/browserView.ts b/src/vs/workbench/contrib/browserView/common/browserView.ts index 232e8b3f213863..8dc66abd6fa950 100644 --- a/src/vs/workbench/contrib/browserView/common/browserView.ts +++ b/src/vs/workbench/contrib/browserView/common/browserView.ts @@ -468,6 +468,11 @@ export interface IBrowserViewModel extends IDisposable { setDevice(device: IBrowserDeviceProfile | undefined): Promise; } +/** A snapshot already includes events at its version; only snapshots may replace equal-version state. */ +function shouldApplyNavigationState(version: number, currentVersion: number, isSnapshot: boolean): boolean { + return version > currentVersion || (isSnapshot && version === currentVersion); +} + export class BrowserViewModel extends Disposable implements IBrowserViewModel { private _url: string = ''; private _owner: IBrowserViewOwner; @@ -492,10 +497,23 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { private _elementSelectionState: IBrowserElementSelectionState = { active: false, options: {} }; private _isAreaSelectionActive: boolean = false; private _device: IBrowserDeviceProfile | undefined; + private readonly _navigationStateVersions: { navigation: number; title: number; loading: number; favicon: number }; readonly history = this._register(new BrowserHistoryStore()); readonly permissions = this._register(new BrowserPermissionStore()); + private readonly _onDidNavigate = this._register(new Emitter()); + readonly onDidNavigate: Event = this._onDidNavigate.event; + + private readonly _onDidChangeTitle = this._register(new Emitter()); + readonly onDidChangeTitle: Event = this._onDidChangeTitle.event; + + private readonly _onDidChangeLoadingState = this._register(new Emitter()); + readonly onDidChangeLoadingState: Event = this._onDidChangeLoadingState.event; + + private readonly _onDidChangeFavicon = this._register(new Emitter()); + readonly onDidChangeFavicon: Event = this._onDidChangeFavicon.event; + private readonly _onDidChangeDevice = this._register(new Emitter()); readonly onDidChangeDevice: Event = this._onDidChangeDevice.event; @@ -528,6 +546,12 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { ) { super(); this._owner = owner; + this._navigationStateVersions = { + navigation: initialState.navigationStateVersion, + title: initialState.navigationStateVersion, + loading: initialState.navigationStateVersion, + favicon: initialState.navigationStateVersion, + }; // Initialize state this._url = initialState.url; @@ -593,45 +617,15 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { } })); - this._register(this.onDidNavigate(e => { - // Clear favicon on navigation to a different host - if (URL.parse(e.url)?.host !== URL.parse(this._url)?.host) { - this._favicon = undefined; - } - - this._zoomHost = parseZoomHost(e.url); - this._url = e.url; - this._title = e.title; - this._canGoBack = e.canGoBack; - this._canGoForward = e.canGoForward; - this._certificateError = e.certificateError; - this._updateSharingState(); - - // Always forceApply because Chromium resets zoom on cross-origin navigation, - // and an origin change may not correspond to a host change (e.g. http→https). - void this.setBrowserZoomIndex( - this.zoomService.getEffectiveZoomIndex(this._zoomHost, this._isInMemory), - true - ); - })); - - this._register(this.onDidChangeLoadingState(e => { - this._loading = e.loading; - this._error = e.error; - })); + this._register(this.browserViewService.onDynamicDidNavigate(this.id)(e => this._updateNavigation(e))); + this._register(this.browserViewService.onDynamicDidChangeLoadingState(this.id)(e => this._updateLoadingState(e))); + this._register(this.browserViewService.onDynamicDidChangeTitle(this.id)(e => this._updateTitle(e))); + this._register(this.browserViewService.onDynamicDidChangeFavicon(this.id)(e => this._updateFavicon(e))); this._register(this.onDidChangeDevToolsState(e => { this._isDevToolsOpen = e.isDevToolsOpen; })); - this._register(this.onDidChangeTitle(e => { - this._title = e.title; - })); - - this._register(this.onDidChangeFavicon(e => { - this._favicon = e.favicon; - })); - this._register(this.onDidChangeOwner(owner => { this._owner = owner; })); @@ -676,6 +670,98 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { this._register(this.onDidChangeRemoteStatus(isRemoteSession => { this._isRemoteSession = isRemoteSession; })); + + // Subscribe before reading back state: a native popup can finish loading before model creation. + void this._synchronizeInitialNavigationState().catch(error => { + this.logService.error('[BrowserViewModel] Failed to synchronize initial navigation state.', error); + }); + } + + private async _synchronizeInitialNavigationState(): Promise { + const state = await this.browserViewService.getState(this.id); + if (this._store.isDisposed) { + return; + } + + if (state.url) { + this._updateNavigation(state, true); + } + this._updateTitle({ navigationStateVersion: state.navigationStateVersion, title: state.title }, true); + this._updateLoadingState({ navigationStateVersion: state.navigationStateVersion, loading: state.loading, error: state.lastError }, true); + this._updateFavicon({ navigationStateVersion: state.navigationStateVersion, favicon: state.lastFavicon }, true); + } + + private _updateNavigation(event: IBrowserViewNavigationEvent, isSnapshot = false): void { + if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.navigation, isSnapshot)) { + return; + } + const didChange = event.url !== this._url + || event.canGoBack !== this._canGoBack + || event.canGoForward !== this._canGoForward + || !structuralEquals(event.certificateError, this._certificateError) + || (shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.title, isSnapshot) && event.title !== this._title); + this._navigationStateVersions.navigation = event.navigationStateVersion; + + if (shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.favicon, isSnapshot) && URL.parse(event.url)?.host !== URL.parse(this._url)?.host) { + this._favicon = undefined; + this._navigationStateVersions.favicon = event.navigationStateVersion; + } + if (shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.title, isSnapshot)) { + this._title = event.title; + this._navigationStateVersions.title = event.navigationStateVersion; + } + this._zoomHost = parseZoomHost(event.url); + this._url = event.url; + this._canGoBack = event.canGoBack; + this._canGoForward = event.canGoForward; + this._certificateError = event.certificateError; + this._updateSharingState(); + + if (isSnapshot && !didChange) { + return; + } + + // Chromium resets zoom on cross-origin navigation, even when the host is unchanged. + void this.setBrowserZoomIndex( + this.zoomService.getEffectiveZoomIndex(this._zoomHost, this._isInMemory), + true + ).catch(error => this.logService.warn('[BrowserViewModel] Failed to update zoom after navigation.', error)); + this._onDidNavigate.fire({ + navigationStateVersion: event.navigationStateVersion, + url: this._url, + title: this._title, + canGoBack: this._canGoBack, + canGoForward: this._canGoForward, + certificateError: this._certificateError, + }); + } + + private _updateTitle(event: IBrowserViewTitleChangeEvent, isSnapshot = false): void { + if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.title, isSnapshot)) { + return; + } + this._navigationStateVersions.title = event.navigationStateVersion; + this._title = event.title; + this._onDidChangeTitle.fire(event); + } + + private _updateLoadingState(event: IBrowserViewLoadingEvent, isSnapshot = false): void { + if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.loading, isSnapshot)) { + return; + } + this._navigationStateVersions.loading = event.navigationStateVersion; + this._loading = event.loading; + this._error = event.error; + this._onDidChangeLoadingState.fire(event); + } + + private _updateFavicon(event: IBrowserViewFaviconChangeEvent, isSnapshot = false): void { + if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.favicon, isSnapshot)) { + return; + } + this._navigationStateVersions.favicon = event.navigationStateVersion; + this._favicon = event.favicon; + this._onDidChangeFavicon.fire(event); } get url(): string { return this._url; } @@ -715,14 +801,6 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { get isAreaSelectionActive(): boolean { return this._isAreaSelectionActive; } get device(): IBrowserDeviceProfile | undefined { return this._device; } - get onDidNavigate(): Event { - return this.browserViewService.onDynamicDidNavigate(this.id); - } - - get onDidChangeLoadingState(): Event { - return this.browserViewService.onDynamicDidChangeLoadingState(this.id); - } - get onDidChangeFocus(): Event { return this.browserViewService.onDynamicDidChangeFocus(this.id); } @@ -735,14 +813,6 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { return this.browserViewService.onDynamicDidKeyCommand(this.id); } - get onDidChangeTitle(): Event { - return this.browserViewService.onDynamicDidChangeTitle(this.id); - } - - get onDidChangeFavicon(): Event { - return this.browserViewService.onDynamicDidChangeFavicon(this.id); - } - get onDidChangeOwner(): Event { return this.browserViewService.onDynamicDidChangeOwner(this.id); } diff --git a/src/vs/workbench/contrib/browserView/electron-browser/browserViewWorkbenchService.ts b/src/vs/workbench/contrib/browserView/electron-browser/browserViewWorkbenchService.ts index b2a51971fcfa14..e56e85a83dacf7 100644 --- a/src/vs/workbench/contrib/browserView/electron-browser/browserViewWorkbenchService.ts +++ b/src/vs/workbench/contrib/browserView/electron-browser/browserViewWorkbenchService.ts @@ -387,7 +387,7 @@ export class BrowserViewWorkbenchService extends Disposable implements IBrowserV ); return this._createModel(info); }); - input.onWillDispose(() => { + Event.once(input.onWillDispose)(() => { this._known.delete(id); this._onDidChangeBrowserViews.fire(); }); diff --git a/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts b/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts index 3577ee7f3710d9..d3fb4229beb908 100644 --- a/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts +++ b/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts @@ -22,6 +22,8 @@ suite('BrowserViewModel', () => { test('only blocks disallowed pages that cannot be shared directly', () => { const browserViewService = upcastPartial({ destroyBrowserView: async () => { }, + getState: async () => createInitialState(BrowserViewStorageScope.Ephemeral, []), + setBrowserZoomIndex: async () => { }, onDynamicDidChangePermissions: () => Event.None, onDynamicDidNavigate: () => Event.None, onDynamicDidChangeLoadingState: () => Event.None, @@ -81,6 +83,7 @@ suite('BrowserViewModel', () => { function createInitialState(storageScope: BrowserViewStorageScope, audiences: IBrowserViewAudience[]): IBrowserViewState { return { + navigationStateVersion: 0, url: 'https://blocked.example.com/', title: '', canGoBack: false, diff --git a/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts new file mode 100644 index 00000000000000..115acc8fad151e --- /dev/null +++ b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts @@ -0,0 +1,346 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { DeferredPromise } from '../../../../../base/common/async.js'; +import { Emitter, Event, Relay } from '../../../../../base/common/event.js'; +import { upcastPartial } from '../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { browserZoomDefaultIndex, BrowserViewStorageScope, IBrowserViewFaviconChangeEvent, IBrowserViewLoadingEvent, IBrowserViewNavigationEvent, IBrowserViewService, IBrowserViewState, IBrowserViewTitleChangeEvent } from '../../../../../platform/browserView/common/browserView.js'; +import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; +import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js'; +import { NullLogService } from '../../../../../platform/log/common/log.js'; +import { IAgentNetworkFilterService } from '../../../../../platform/networkFilter/common/networkFilterService.js'; +import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { NullTelemetryService } from '../../../../../platform/telemetry/common/telemetryUtils.js'; +import { IThemeService } from '../../../../../platform/theme/common/themeService.js'; +import { BrowserEditorInput } from '../../common/browserEditorInput.js'; +import { BrowserViewModel, IBrowserViewWorkbenchService } from '../../common/browserView.js'; +import { IBrowserZoomService } from '../../common/browserZoomService.js'; + +suite('BrowserViewModel initial state handoff', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + const childUrl = 'http://localhost/popup-child'; + const childTitle = 'Browser Smoke Popup Child'; + + function createPopup(delaySubscriptions = false) { + const trace: string[] = []; + const navigation = store.add(new Emitter()); + const title = store.add(new Emitter()); + const loading = store.add(new Emitter()); + const favicon = store.add(new Emitter()); + const navigationRelay = store.add(new Relay()); + const titleRelay = store.add(new Relay()); + const loadingRelay = store.add(new Relay()); + const faviconRelay = store.add(new Relay()); + const close = store.add(new Emitter()); + const snapshot = new DeferredPromise(); + const errors: (string | Error)[] = []; + const errorLogged = new DeferredPromise(); + const logService = new class extends NullLogService { + override error(message: string | Error): void { + errors.push(message); + void errorLogged.complete(); + } + }; + const connectSubscriptions = () => { + navigationRelay.input = navigation.event; + titleRelay.input = title.event; + loadingRelay.input = loading.event; + faviconRelay.input = favicon.event; + }; + if (!delaySubscriptions) { + connectSubscriptions(); + } + const initialState: IBrowserViewState = { + navigationStateVersion: 0, + url: '', + title: '', + canGoBack: false, + canGoForward: false, + loading: true, + focused: false, + visible: false, + isDevToolsOpen: false, + lastScreenshot: undefined, + lastFavicon: undefined, + lastError: undefined, + certificateError: undefined, + storageScope: BrowserViewStorageScope.Ephemeral, + storageKeys: {}, + permissions: { origins: {} }, + browserZoomIndex: browserZoomDefaultIndex, + elementSelectionState: { active: false, options: {} }, + isRemoteSession: false, + isAreaSelectionActive: false, + device: undefined, + audiences: [], + }; + let state = initialState; + const destroyed: string[] = []; + const service = upcastPartial({ + onDynamicDidNavigate: id => (listener, thisArgs, disposables) => { + trace.push(`subscribe navigation ${id}`); + return navigationRelay.event(listener, thisArgs, disposables); + }, + onDynamicDidChangeTitle: id => (listener, thisArgs, disposables) => { + trace.push(`subscribe title ${id}`); + return titleRelay.event(listener, thisArgs, disposables); + }, + onDynamicDidChangeLoadingState: () => loadingRelay.event, + onDynamicDidClose: () => close.event, + onDynamicDidChangePermissions: () => Event.None, + onDynamicDidChangeDevToolsState: () => Event.None, + onDynamicDidChangeFavicon: () => faviconRelay.event, + onDynamicDidChangeOwner: () => Event.None, + onDynamicDidChangeFocus: () => Event.None, + onDynamicDidChangeVisibility: () => Event.None, + onDynamicDidChangeDeviceEmulation: () => Event.None, + onDynamicDidChangeElementSelectionState: () => Event.None, + onDynamicDidChangeAreaSelectionActive: () => Event.None, + onDynamicDidChangeAudiences: () => Event.None, + onDynamicDidChangeRemoteStatus: () => Event.None, + getState: id => { + trace.push(`snapshot requested ${id}`); + return snapshot.p; + }, + setBrowserZoomIndex: async () => { }, + destroyBrowserView: async id => { destroyed.push(id); }, + loadURL: async () => { assert.fail('Adopting a popup must not navigate it again'); }, + }); + const workbenchService = upcastPartial({ + isSharingAvailable: false, + onDidChangeSharingAvailable: Event.None, + }); + + const setTitle = (pageTitle: string) => { + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, title: pageTitle }; + trace.push(`native title ${pageTitle}`); + title.fire({ navigationStateVersion: state.navigationStateVersion, title: pageTitle }); + }; + const commit = (url = childUrl, pageTitle = childTitle) => { + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, canGoBack: !!state.url, url, title: '', lastFavicon: undefined }; + trace.push(`native navigation ${url}`); + navigation.fire(state); + setTitle(pageTitle); + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, loading: false }; + loading.fire({ navigationStateVersion: state.navigationStateVersion, loading: false }); + }; + + const adopt = (creationState = initialState) => { + trace.push('create model child'); + const model = store.add(new BrowserViewModel( + 'child', { windowId: 1 }, { type: 'user' }, undefined, creationState, service, workbenchService, + NullTelemetryService, upcastPartial({}), upcastPartial({}), + upcastPartial({ getEffectiveZoomIndex: () => browserZoomDefaultIndex, onDidChangeZoom: Event.None }), + upcastPartial({ onDidChange: Event.None }), + logService, + )); + trace.push('create editor child'); + const input = store.add(new BrowserEditorInput( + { id: 'child', url: childUrl }, async () => model, + upcastPartial({}), upcastPartial({}), NullTelemetryService, workbenchService, + )); + input.model = model; + store.add(input.onDidChangeLabel(() => trace.push(`label ${input.getName()}`))); + return { model, input }; + }; + + return { trace, initialState, get state() { return state; }, snapshot, commit, setTitle, adopt, connectSubscriptions, navigation, title, loading, favicon, close, destroyed, errors, errorLogged }; + } + + test('reconciles a popup that finished loading before model subscription', async () => { + const popup = createPopup(); + popup.commit(); + const { model, input } = popup.adopt(); + await popup.snapshot.complete(popup.state); + const resolved = await input.resolve(); + + assert.deepStrictEqual({ + id: input.id, + sameModel: resolved === model, + nativeUrl: popup.state.url, + nativeTitle: popup.state.title, + modelUrl: model.url, + modelTitle: model.title, + loading: model.loading, + label: input.getName(), + subscribedBeforeSnapshot: popup.trace.indexOf('subscribe title child') < popup.trace.indexOf('snapshot requested child'), + }, { + id: 'child', + sameModel: true, + nativeUrl: childUrl, + nativeTitle: childTitle, + modelUrl: childUrl, + modelTitle: childTitle, + loading: false, + label: childTitle, + subscribedBeforeSnapshot: true, + }, JSON.stringify(popup.trace)); + }); + + test('recovers updates while remote subscription delivery is delayed', async () => { + const popup = createPopup(true); + const { model, input } = popup.adopt(); + popup.commit(); + popup.connectSubscriptions(); + await popup.snapshot.complete(popup.state); + + assert.deepStrictEqual({ url: model.url, title: model.title, loading: model.loading, label: input.getName() }, { + url: childUrl, + title: childTitle, + loading: false, + label: childTitle, + }); + }); + + test('does not report a new navigation for an unchanged snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(popup.state); + const navigations: string[] = []; + store.add(model.onDidNavigate(event => navigations.push(event.url))); + await popup.snapshot.complete(popup.state); + + assert.deepStrictEqual({ title: model.title, navigations }, { title: childTitle, navigations: [] }); + }); + + test('logs an initial snapshot failure', async () => { + const popup = createPopup(); + popup.adopt(); + await popup.snapshot.error(new Error('Snapshot unavailable')); + await popup.errorLogged.p; + + assert.deepStrictEqual(popup.errors, ['[BrowserViewModel] Failed to synchronize initial navigation state.']); + }); + + for (const snapshotFirst of [false, true]) { + test(`preserves later navigation when the snapshot arrives ${snapshotFirst ? 'before' : 'after'} the events`, async () => { + const popup = createPopup(); + popup.commit(); + const snapshot = popup.state; + const { model, input } = popup.adopt(); + const labels: string[] = []; + store.add(input.onDidChangeLabel(() => labels.push(input.getName()))); + + if (snapshotFirst) { + await popup.snapshot.complete(snapshot); + } + popup.commit('http://later.example/child', 'Later child'); + labels.length = 0; + if (!snapshotFirst) { + await popup.snapshot.complete(snapshot); + } + + assert.deepStrictEqual({ + url: model.url, + title: model.title, + canGoBack: model.canGoBack, + loading: model.loading, + label: input.getName(), + staleLabels: labels.filter(label => label !== 'Later child'), + }, { + url: 'http://later.example/child', + title: 'Later child', + canGoBack: true, + loading: false, + label: 'Later child', + staleLabels: [], + }, JSON.stringify(popup.trace)); + }); + } + + test('recovers missed navigation without overwriting a newer title event', async () => { + const popup = createPopup(); + popup.commit(); + const snapshot = popup.state; + const { model, input } = popup.adopt(); + popup.setTitle('Updated child title'); + await popup.snapshot.complete(snapshot); + + assert.deepStrictEqual({ url: model.url, title: model.title, label: input.getName() }, { + url: childUrl, + title: 'Updated child title', + label: 'Updated child title', + }, JSON.stringify(popup.trace)); + }); + + test('ignores older events delivered after a newer snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const olderState = popup.state; + popup.commit('http://later.example/child', 'Later child'); + const { model, input } = popup.adopt(); + await popup.snapshot.complete(popup.state); + popup.navigation.fire(olderState); + popup.title.fire({ navigationStateVersion: olderState.navigationStateVersion, title: 'Stale title' }); + popup.loading.fire({ navigationStateVersion: olderState.navigationStateVersion, loading: true }); + popup.favicon.fire({ navigationStateVersion: olderState.navigationStateVersion, favicon: 'https://old.example/icon.png' }); + + assert.deepStrictEqual({ url: model.url, title: model.title, loading: model.loading, favicon: model.favicon, label: input.getName() }, { + url: 'http://later.example/child', + title: 'Later child', + loading: false, + favicon: undefined, + label: 'Later child', + }); + }); + + test('does not replace a newer favicon while recovering missed navigation', async () => { + const popup = createPopup(); + popup.commit(); + const snapshot = popup.state; + const { model } = popup.adopt(); + popup.favicon.fire({ navigationStateVersion: snapshot.navigationStateVersion + 1, favicon: 'https://new.example/icon.png' }); + await popup.snapshot.complete(snapshot); + + assert.deepStrictEqual({ url: model.url, favicon: model.favicon }, { + url: childUrl, + favicon: 'https://new.example/icon.png', + }); + }); + + test('does not replay a loading event already included in the snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(); + const snapshot = { ...popup.state, loading: true }; + await popup.snapshot.complete(snapshot); + + // An aborted load can report false while a competing native navigation is still loading. + popup.loading.fire({ navigationStateVersion: snapshot.navigationStateVersion, loading: false }); + + assert.deepStrictEqual({ title: model.title, loading: model.loading }, { title: childTitle, loading: true }); + }); + + for (const closeNative of [true, false]) { + test(`preserves ${closeNative ? 'native child' : 'editor'} closure coupling during a pending snapshot`, async () => { + const popup = createPopup(); + const { model, input } = popup.adopt(); + const resolved = await Promise.all([input.resolve(), input.resolve()]); + if (closeNative) { + popup.close.fire(); + } else { + input.dispose(); + } + popup.commit(); + await popup.snapshot.complete(popup.state); + + assert.deepStrictEqual({ + resolvedSameModel: resolved.every(candidate => candidate === model), + editorDisposed: input.isDisposed(), + modelDetached: input.model === undefined, + destroyed: popup.destroyed, + lateTitleApplied: model.title !== '', + }, { + resolvedSameModel: true, + editorDisposed: true, + modelDetached: true, + destroyed: ['child'], + lateTitleApplied: false, + }); + }); + } +}); diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/browserEditorInput.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/browserEditorInput.test.ts index ed42294685ff49..0bd06a5f18100c 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-browser/browserEditorInput.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/browserEditorInput.test.ts @@ -269,6 +269,7 @@ suite('BrowserEditorInput', () => { url = 'https://loaded.example/'; title = ''; onDidNavigate.fire({ + navigationStateVersion: 1, url, title, canGoBack: false, diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts index d8448ff3270f1a..ef0fa1e8090d93 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts @@ -43,6 +43,7 @@ suite('BrowserViewModel', () => { onDynamicDidChangeRemoteStatus: () => Event.None, onDynamicDidChangeAudiences: () => Event.None, destroyBrowserView: async () => { }, + getState: async () => initialState, }); const browserViewWorkbenchService = upcastPartial({ isSharingAvailable: true, @@ -59,6 +60,7 @@ suite('BrowserViewModel', () => { getEffectiveZoomIndex: () => browserZoomDefaultIndex, }); const initialState: IBrowserViewState = { + navigationStateVersion: 0, url: '', title: '', canGoBack: false, diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts new file mode 100644 index 00000000000000..40aa4460d4a9f7 --- /dev/null +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts @@ -0,0 +1,355 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import sinon from 'sinon'; +import { mainWindow } from '../../../../../base/browser/window.js'; +import { DeferredPromise } from '../../../../../base/common/async.js'; +import { VSBuffer } from '../../../../../base/common/buffer.js'; +import { Emitter, Event } from '../../../../../base/common/event.js'; +import { Disposable, DisposableStore } from '../../../../../base/common/lifecycle.js'; +import { URI } from '../../../../../base/common/uri.js'; +import { ChannelClient, ChannelServer, IMessagePassingProtocol, ProxyChannel } from '../../../../../base/parts/ipc/common/ipc.js'; +import { mock, upcastPartial } from '../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { nextMacrotask, realTimeApi } from '../../../../../base/test/common/virtualScheduling/index.js'; +import { IAccessibilityService } from '../../../../../platform/accessibility/common/accessibility.js'; +import { browserZoomDefaultIndex, BrowserViewStorageScope, IBrowserViewCreatedEvent, IBrowserViewInfo, IBrowserViewLoadingEvent, IBrowserViewNavigationEvent, IBrowserViewService, IBrowserViewState, IBrowserViewTitleChangeEvent, ipcBrowserViewChannelName } from '../../../../../platform/browserView/common/browserView.js'; +import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js'; +import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js'; +import { IContextKeyService } from '../../../../../platform/contextkey/common/contextkey.js'; +import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; +import { IEditorOptions } from '../../../../../platform/editor/common/editor.js'; +import { InstantiationService } from '../../../../../platform/instantiation/common/instantiationService.js'; +import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; +import { IMainProcessService } from '../../../../../platform/ipc/common/mainProcessService.js'; +import { IKeybindingService } from '../../../../../platform/keybinding/common/keybinding.js'; +import { MockContextKeyService, MockKeybindingService } from '../../../../../platform/keybinding/test/common/mockKeybindingService.js'; +import { ILogService, NullLogService } from '../../../../../platform/log/common/log.js'; +import { IAgentNetworkFilterService } from '../../../../../platform/networkFilter/common/networkFilterService.js'; +import { INotificationService } from '../../../../../platform/notification/common/notification.js'; +import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { ITelemetryService } from '../../../../../platform/telemetry/common/telemetry.js'; +import { NullTelemetryService } from '../../../../../platform/telemetry/common/telemetryUtils.js'; +import { IThemeService } from '../../../../../platform/theme/common/themeService.js'; +import { TestColorTheme } from '../../../../../platform/theme/test/common/testThemeService.js'; +import { IWorkspaceContextService, WorkbenchState } from '../../../../../platform/workspace/common/workspace.js'; +import { IWorkspaceTrustEnablementService, IWorkspaceTrustManagementService } from '../../../../../platform/workspace/common/workspaceTrust.js'; +import { TestWorkspace } from '../../../../../platform/workspace/test/common/testWorkspace.js'; +import { IUntypedEditorInput } from '../../../../common/editor.js'; +import { EditorInput } from '../../../../common/editor/editorInput.js'; +import { IEditorGroup, IEditorGroupsService } from '../../../../services/editor/common/editorGroupsService.js'; +import { IEditorService, PreferredGroup } from '../../../../services/editor/common/editorService.js'; +import { INativeWorkbenchEnvironmentService } from '../../../../services/environment/electron-browser/environmentService.js'; +import { IChatWidgetService } from '../../../chat/browser/chat.js'; +import { BrowserEditorInput } from '../../common/browserEditorInput.js'; +import { BrowserViewModel, IBrowserViewWorkbenchService } from '../../common/browserView.js'; +import { IBrowserZoomService } from '../../common/browserZoomService.js'; +import { BrowserViewWorkbenchService } from '../../electron-browser/browserViewWorkbenchService.js'; + +class ControlledProtocol extends Disposable implements IMessagePassingProtocol { + private readonly _onMessage = this._register(new Emitter()); + readonly onMessage = this._onMessage.event; + private readonly pending: VSBuffer[] = []; + paused = true; + other!: ControlledProtocol; + + send(message: VSBuffer): void { + if (this.other.paused) { + this.other.pending.push(message); + } else { + this.other._onMessage.fire(message); + } + } + + resume(): void { + this.paused = false; + for (const message of this.pending.splice(0)) { + this._onMessage.fire(message); + } + } +} + +suite('BrowserViewWorkbenchService popup handoff', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + teardown(() => sinon.restore()); + + const childId = 'popup-child'; + const childUrl = 'http://localhost/popup-child'; + const childTitle = 'Browser Smoke Popup Child'; + + async function createWorkbench() { + const inputs = store.add(new DisposableStore()); + const services = store.add(new DisposableStore()); + const ipc = store.add(new DisposableStore()); + const events = store.add(new DisposableStore()); + const created = events.add(new Emitter()); + const navigation = events.add(new Emitter()); + const title = events.add(new Emitter()); + const loading = events.add(new Emitter()); + const closed = events.add(new Emitter()); + const listedViews = new DeferredPromise(); + const snapshot = new DeferredPromise(); + const trace: string[] = []; + const errors: (string | Error)[] = []; + const openCalls: BrowserEditorInput[] = []; + const groupEditors: BrowserEditorInput[] = []; + const destroyed: string[] = []; + const initialState = createInitialState(); + let state = initialState; + let nativeCreateCalls = 0; + let navigationCalls = 0; + const info = (): IBrowserViewInfo => ({ + id: childId, host: { windowId: mainWindow.vscodeWindowId }, owner: { type: 'user' }, state, + }); + const browserService = new class extends mock() { + override readonly onDidCreateBrowserView = created.event; + override getBrowserViews() { return listedViews.p; } + override getState(id: string) { + trace.push(`snapshot ${id}`); + return snapshot.p; + } + override async getOrCreateBrowserView() { + nativeCreateCalls++; + return info(); + } + override async loadURL() { navigationCalls++; } + override async updateWindowConfiguration() { } + override async setBrowserZoomIndex() { } + override async destroyBrowserView(id: string) { destroyed.push(id); } + override onDynamicDidNavigate(id: string) { + trace.push(`subscribe navigation ${id}`); + return navigation.event; + } + override onDynamicDidChangeTitle(id: string) { + trace.push(`subscribe title ${id}`); + return title.event; + } + override onDynamicDidChangeLoadingState() { return loading.event; } + override onDynamicDidClose() { return closed.event; } + override onDynamicDidChangePermissions() { return Event.None; } + override onDynamicDidChangeDevToolsState() { return Event.None; } + override onDynamicDidChangeFavicon() { return Event.None; } + override onDynamicDidChangeOwner() { return Event.None; } + override onDynamicDidChangeFocus() { return Event.None; } + override onDynamicDidChangeVisibility() { return Event.None; } + override onDynamicDidChangeDeviceEmulation() { return Event.None; } + override onDynamicDidChangeElementSelectionState() { return Event.None; } + override onDynamicDidChangeAreaSelectionActive() { return Event.None; } + override onDynamicDidChangeAudiences() { return Event.None; } + override onDynamicDidChangeRemoteStatus() { return Event.None; } + }(); + const mainProtocol = ipc.add(new ControlledProtocol()); + const rendererProtocol = ipc.add(new ControlledProtocol()); + mainProtocol.other = rendererProtocol; + rendererProtocol.other = mainProtocol; + const client = ipc.add(new ChannelClient(rendererProtocol)); + const server = ipc.add(new ChannelServer(mainProtocol, 'popup-test')); + server.registerChannel(ipcBrowserViewChannelName, ProxyChannel.fromService(browserService, events)); + rendererProtocol.resume(); + mainProtocol.resume(); + + const group = upcastPartial({ id: 1, editors: groupEditors, isLocked: false }); + const configuration = new TestConfigurationService(); + const collection = new ServiceCollection( + [IMainProcessService, upcastPartial({ getChannel: name => client.getChannel(name) })], + [IConfigurationService, configuration], + [IWorkspaceContextService, upcastPartial({ + getWorkspace: () => TestWorkspace, getWorkbenchState: () => WorkbenchState.EMPTY, onDidChangeWorkspaceFolders: Event.None, + })], + [IWorkspaceTrustManagementService, upcastPartial({ + workspaceTrustInitialized: Promise.resolve(), isWorkspaceTrusted: () => true, getTrustedUris: () => [], + onDidChangeTrustedFolders: Event.None, onDidChangeTrust: Event.None, + })], + [IWorkspaceTrustEnablementService, upcastPartial({ isWorkspaceTrustEnabled: () => true })], + [IKeybindingService, new MockKeybindingService()], + [IContextKeyService, new MockContextKeyService()], + [IThemeService, upcastPartial({ getColorTheme: () => new TestColorTheme(), onDidColorThemeChange: Event.None })], + [IAccessibilityService, upcastPartial({ isMotionReduced: () => false, onDidChangeReducedMotion: Event.None })], + [INativeWorkbenchEnvironmentService, upcastPartial({ userHome: URI.file('/popup-test') })], + [ILogService, new class extends NullLogService { + override error(message: string | Error): void { errors.push(message); } + }()], + [INotificationService, upcastPartial({})], + [IChatWidgetService, upcastPartial({})], + [IDialogService, upcastPartial({})], + [IStorageService, upcastPartial({})], + [ITelemetryService, NullTelemetryService], + [IBrowserZoomService, upcastPartial({ getEffectiveZoomIndex: () => browserZoomDefaultIndex, onDidChangeZoom: Event.None })], + [IAgentNetworkFilterService, upcastPartial({ onDidChange: Event.None })], + [IEditorGroupsService, upcastPartial({ + groups: [group], activeGroup: group, getGroup: id => id === group.id ? group : undefined, + })], + [IEditorService, upcastPartial({ + openEditor: async (editor: EditorInput | IUntypedEditorInput, _options?: IEditorOptions | PreferredGroup, targetGroup?: PreferredGroup) => { + assert.ok(editor instanceof BrowserEditorInput); + assert.strictEqual(targetGroup, group); + inputs.add(editor); + openCalls.push(editor); + groupEditors.push(editor); + trace.push(`open editor ${editor.id}`); + inputs.add(editor.onWillDispose(() => groupEditors.splice(groupEditors.indexOf(editor), 1))); + return undefined; + }, + })], + ); + const instantiationService = services.add(new InstantiationService(collection, true)); + const workbench = services.add(instantiationService.createInstance(BrowserViewWorkbenchService)); + collection.set(IBrowserViewWorkbenchService, workbench); + const initialized = Event.toPromise(client.getChannel(ipcBrowserViewChannelName).listen('onDidCreateBrowserView')); + created.fire({ info: { ...info(), id: 'other-window', host: { windowId: mainWindow.vscodeWindowId + 1 } } }); + await initialized; + const parent = inputs.add(workbench.getOrCreateLazy({ id: 'parent', url: 'http://localhost/lifecycle' })); + groupEditors.push(parent); + const instances = sinon.spy(instantiationService, 'createInstance'); + + const commit = (url = childUrl, pageTitle = childTitle) => { + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, url, title: '', canGoBack: !!state.url }; + trace.push(`navigate ${url}`); + navigation.fire(state); + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, title: pageTitle }; + trace.push(`title ${pageTitle}`); + title.fire(state); + state = { ...state, navigationStateVersion: state.navigationStateVersion + 1, loading: false }; + loading.fire(state); + }; + const publish = () => { + trace.push(`creation ${childId}`); + created.fire({ info: info(), initialUrl: childUrl, editorOpenRequest: { parentViewId: parent.id, pinned: true } }); + }; + const settle = () => new Promise(resolve => nextMacrotask(realTimeApi, resolve)); + const child = () => { + const input = workbench.getKnownBrowserViews().get(childId); + assert.ok(input); + return input; + }; + return { + workbench, initialState, info, publish, commit, settle, child, trace, openCalls, groupEditors, destroyed, closed, + mainProtocol, rendererProtocol, listedViews, snapshot, instances, errors, + get state() { return state; }, get nativeCreateCalls() { return nativeCreateCalls; }, get navigationCalls() { return navigationCalls; }, + }; + } + + for (const delay of ['creation', 'subscriptions'] as const) { + test(`recovers a loaded child with delayed ${delay} through the production workbench and IPC channel`, async () => { + const testCase = await createWorkbench(); + const delayedProtocol = delay === 'creation' ? testCase.rendererProtocol : testCase.mainProtocol; + delayedProtocol.paused = true; + testCase.publish(); + testCase.commit(); + delayedProtocol.resume(); + await testCase.snapshot.complete(testCase.state); + await testCase.listedViews.complete([testCase.info()]); + await testCase.settle(); + const input = testCase.child(); + const model = await input.resolve(); + + assert.deepStrictEqual({ + url: model.url, title: model.title, label: input.getName(), loading: model.loading, + openCalls: testCase.openCalls.map(editor => editor.id), + groupEditors: testCase.groupEditors.map(editor => editor.id), + known: [...testCase.workbench.getKnownBrowserViews().keys()], + inputCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserEditorInput).length, + modelCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserViewModel).length, + nativeCreateCalls: testCase.nativeCreateCalls, navigationCalls: testCase.navigationCalls, errors: testCase.errors, + trace: testCase.trace, + }, { + url: childUrl, title: childTitle, label: childTitle, loading: false, + openCalls: [childId], groupEditors: ['parent', childId], known: ['parent', childId], + inputCreations: 1, modelCreations: 1, nativeCreateCalls: 0, navigationCalls: 0, errors: [], + trace: [ + `creation ${childId}`, `navigate ${childUrl}`, `title ${childTitle}`, + `subscribe navigation ${childId}`, `subscribe title ${childId}`, `snapshot ${childId}`, `open editor ${childId}`, + ], + }, JSON.stringify(testCase.trace)); + }); + } + + for (const snapshotFirst of [true, false]) { + test(`does not roll back or recreate the editor when the snapshot is delivered ${snapshotFirst ? 'before' : 'after'} later events`, async () => { + const testCase = await createWorkbench(); + testCase.rendererProtocol.paused = true; + testCase.publish(); + const staleInfo = testCase.info(); + testCase.commit(); + const snapshot = testCase.state; + testCase.rendererProtocol.resume(); + const input = testCase.child(); + const model = await input.resolve(); + const labels: string[] = []; + store.add(input.onDidChangeLabel(() => labels.push(input.getName()))); + if (snapshotFirst) { + await testCase.snapshot.complete(snapshot); + await testCase.settle(); + } + testCase.commit('http://later.example/child', 'Later child'); + labels.length = 0; + if (!snapshotFirst) { + await testCase.snapshot.complete(snapshot); + } + await testCase.listedViews.complete([staleInfo]); + await testCase.settle(); + const sameInput = testCase.workbench.getOrCreateLazy({ id: childId, url: childUrl }); + + assert.deepStrictEqual({ + url: model.url, title: model.title, label: input.getName(), canGoBack: model.canGoBack, + sameInput: sameInput === input, sameModel: await sameInput.resolve() === model, + openCalls: testCase.openCalls.length, childEditors: testCase.groupEditors.filter(editor => editor.id === childId).length, + inputCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserEditorInput).length, + modelCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserViewModel).length, + staleLabels: labels.filter(label => label !== 'Later child'), + nativeCreateCalls: testCase.nativeCreateCalls, navigationCalls: testCase.navigationCalls, errors: testCase.errors, + }, { + url: 'http://later.example/child', title: 'Later child', label: 'Later child', canGoBack: true, + sameInput: true, sameModel: true, openCalls: 1, childEditors: 1, inputCreations: 1, modelCreations: 1, + staleLabels: [], nativeCreateCalls: 0, navigationCalls: 0, errors: [], + }, JSON.stringify(testCase.trace)); + }); + } + + for (const nativeClose of [true, false]) { + test(`keeps the child closed when ${nativeClose ? 'the native close event' : 'editor disposal'} precedes a delayed snapshot`, async () => { + const testCase = await createWorkbench(); + await testCase.listedViews.complete([]); + testCase.publish(); + testCase.commit(); + await testCase.settle(); + const input = testCase.child(); + if (nativeClose) { + testCase.closed.fire(); + } else { + input.dispose(); + } + await testCase.snapshot.complete(testCase.state); + await testCase.settle(); + + assert.deepStrictEqual({ + editorDisposed: input.isDisposed(), + groupEditors: testCase.groupEditors.map(editor => editor.id), + known: [...testCase.workbench.getKnownBrowserViews().keys()], + destroyed: testCase.destroyed, + inputCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserEditorInput).length, + modelCreations: testCase.instances.getCalls().filter(call => call.args[0] === BrowserViewModel).length, + openCalls: testCase.openCalls.length, + errors: testCase.errors, + }, { + editorDisposed: true, groupEditors: ['parent'], known: ['parent'], destroyed: [childId], + inputCreations: 1, modelCreations: 1, openCalls: 1, errors: [], + }); + }); + } +}); + +function createInitialState(): IBrowserViewState { + return { + navigationStateVersion: 0, + url: '', title: '', canGoBack: false, canGoForward: false, loading: true, + focused: false, visible: false, isDevToolsOpen: false, + lastScreenshot: undefined, lastFavicon: undefined, lastError: undefined, certificateError: undefined, + storageScope: BrowserViewStorageScope.Ephemeral, storageKeys: {}, permissions: { origins: {} }, + browserZoomIndex: browserZoomDefaultIndex, elementSelectionState: { active: false, options: {} }, + isRemoteSession: false, isAreaSelectionActive: false, device: undefined, audiences: [], + }; +} diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/features/browserAutoReloadFeatures.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/features/browserAutoReloadFeatures.test.ts index cc1fea417a13c5..5805a37b6a3660 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-browser/features/browserAutoReloadFeatures.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/features/browserAutoReloadFeatures.test.ts @@ -203,6 +203,7 @@ class TestBrowserViewModel extends Disposable { private readonly _onDidChangeVisibility = this._register(new Emitter()); private readonly _onWillDispose = this._register(new Emitter()); private _url: string; + private _navigationStateVersion = 0; private _visible = true; reloadCount = 0; @@ -227,6 +228,7 @@ class TestBrowserViewModel extends Disposable { navigate(url: string): void { this._url = url; this._onDidNavigate.fire({ + navigationStateVersion: ++this._navigationStateVersion, url, title: '', canGoBack: false, From c638055e6553546ede91bbd6dd3c93efe97f4cc2 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sat, 12 Sep 2026 15:40:38 -0700 Subject: [PATCH 2/7] browser: make popup reconciliation lightweight and race-safe Guard asynchronous favicon application against newer requests and navigation. Use navigation-only snapshots and suppress unchanged loading notifications from snapshots while preserving native events. Add deterministic regressions for the review feedback on #335987. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/browserFaviconLoader.ts | 52 +++++++++ .../browserView/common/browserView.ts | 16 ++- .../browserView/electron-main/browserView.ts | 53 +++++---- .../electron-main/browserViewMainService.ts | 6 +- .../test/common/browserFaviconLoader.test.ts | 102 ++++++++++++++++++ .../contrib/browserView/common/browserView.ts | 7 +- .../test/common/browserView.test.ts | 2 +- .../test/common/browserViewModelState.test.ts | 44 +++++++- .../electron-browser/browserViewModel.test.ts | 2 +- .../browserViewWorkbenchService.test.ts | 5 +- 10 files changed, 255 insertions(+), 34 deletions(-) create mode 100644 src/vs/platform/browserView/common/browserFaviconLoader.ts create mode 100644 src/vs/platform/browserView/test/common/browserFaviconLoader.test.ts diff --git a/src/vs/platform/browserView/common/browserFaviconLoader.ts b/src/vs/platform/browserView/common/browserFaviconLoader.ts new file mode 100644 index 00000000000000..56ddf840702325 --- /dev/null +++ b/src/vs/platform/browserView/common/browserFaviconLoader.ts @@ -0,0 +1,52 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { Disposable } from '../../../base/common/lifecycle.js'; +import { ILogService } from '../../log/common/log.js'; + +export class BrowserFaviconLoader extends Disposable { + private _requestId = 0; + + constructor( + private readonly fetchFavicon: (url: string) => Promise, + private readonly applyFavicon: (favicon: string | undefined) => void, + @ILogService private readonly logService: ILogService, + ) { + super(); + } + + invalidate(): void { + this._requestId++; + } + + async load(urls: readonly string[]): Promise { + if (this._store.isDisposed) { + return; + } + const requestId = ++this._requestId; + for (const url of urls) { + let favicon: string | undefined; + try { + favicon = await this.fetchFavicon(url); + } catch (error) { + this.logService.trace('[BrowserFaviconLoader] Failed to fetch favicon, trying the next candidate.', error); + } + if (!this.isCurrent(requestId)) { + return; + } + if (favicon !== undefined) { + this.applyFavicon(favicon); + return; + } + } + if (this.isCurrent(requestId)) { + this.applyFavicon(undefined); + } + } + + private isCurrent(requestId: number): boolean { + return requestId === this._requestId && !this._store.isDisposed; + } +} diff --git a/src/vs/platform/browserView/common/browserView.ts b/src/vs/platform/browserView/common/browserView.ts index 2fc25d06474140..61c276785ac0c0 100644 --- a/src/vs/platform/browserView/common/browserView.ts +++ b/src/vs/platform/browserView/common/browserView.ts @@ -323,7 +323,8 @@ export interface IBrowserViewStorageKeys { readonly permissions?: string; } -export interface IBrowserViewState { +/** Lightweight state used to reconcile navigation without transferring screenshots or session data. */ +export interface IBrowserViewNavigationState { /** Monotonic version shared by navigation, title, loading, and favicon updates. */ navigationStateVersion: number; url: string; @@ -331,13 +332,16 @@ export interface IBrowserViewState { canGoBack: boolean; canGoForward: boolean; loading: boolean; + lastFavicon: string | undefined; + lastError: IBrowserViewLoadError | undefined; + certificateError: IBrowserViewCertificateError | undefined; +} + +export interface IBrowserViewState extends IBrowserViewNavigationState { focused: boolean; visible: boolean; isDevToolsOpen: boolean; lastScreenshot: VSBuffer | undefined; - lastFavicon: string | undefined; - lastError: IBrowserViewLoadError | undefined; - certificateError: IBrowserViewCertificateError | undefined; storageScope: BrowserViewStorageScope; storageKeys: IBrowserViewStorageKeys; permissions: ISerializedBrowserPermissionsSnapshot; @@ -567,10 +571,12 @@ export interface IBrowserViewService { /** * Get the current state, or throw if the view doesn't exist. - * Subscribe before reading and compare navigationStateVersion when reconciling navigation-related events. */ getState(id: string): Promise; + /** Subscribe before reading this snapshot and use its version to reconcile navigation-related events. */ + getNavigationState(id: string): Promise; + /** * Adds an audience or, when disabled, removes every audience matching it. */ diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index 9a74729ce33aa0..776fbcbd476e16 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -7,7 +7,8 @@ import { screen, WebContentsView, webContents } from 'electron'; import { Disposable } from '../../../base/common/lifecycle.js'; import { Emitter, Event } from '../../../base/common/event.js'; import { VSBuffer } from '../../../base/common/buffer.js'; -import { IBrowserViewAudience, IBrowserViewBounds, IBrowserViewDevToolsStateEvent, IBrowserViewFocusEvent, IBrowserViewKeyDownEvent, IBrowserViewState, IBrowserViewNavigationEvent, IBrowserViewLoadingEvent, IBrowserViewLoadError, IBrowserViewTitleChangeEvent, IBrowserViewFaviconChangeEvent, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, IBrowserViewFindInPageResult, IBrowserViewVisibilityEvent, browserViewIsolatedWorldId, browserZoomFactors, browserZoomDefaultIndex, IBrowserViewOwner, IBrowserViewEditorOpenOptions, IBrowserViewPermissionRequestEvent, equalsBrowserViewAudience, isBrowserViewAssociatedResourceNavigation, matchesBrowserViewAudience, IBrowserViewHost } from '../common/browserView.js'; +import { IBrowserViewAudience, IBrowserViewBounds, IBrowserViewDevToolsStateEvent, IBrowserViewFocusEvent, IBrowserViewKeyDownEvent, IBrowserViewState, IBrowserViewNavigationState, IBrowserViewNavigationEvent, IBrowserViewLoadingEvent, IBrowserViewLoadError, IBrowserViewTitleChangeEvent, IBrowserViewFaviconChangeEvent, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, IBrowserViewFindInPageResult, IBrowserViewVisibilityEvent, browserViewIsolatedWorldId, browserZoomFactors, browserZoomDefaultIndex, IBrowserViewOwner, IBrowserViewEditorOpenOptions, IBrowserViewPermissionRequestEvent, equalsBrowserViewAudience, isBrowserViewAssociatedResourceNavigation, matchesBrowserViewAudience, IBrowserViewHost } from '../common/browserView.js'; +import { BrowserFaviconLoader } from '../common/browserFaviconLoader.js'; import { BrowserViewEmulator } from './browserViewEmulator.js'; import { BrowserViewInspector } from './browserViewInspector.js'; import { IWindowsMainService } from '../../windows/electron-main/windows.js'; @@ -284,9 +285,8 @@ export class BrowserView extends Disposable { }); // Favicon events - webContents.on('page-favicon-updated', async (_event, favicons) => { - // try each url in order until one works - for (const url of favicons) { + const faviconLoader = this._register(new BrowserFaviconLoader( + url => { if (!this._faviconRequestCache.has(url)) { this._faviconRequestCache.set(url, (async () => { if (url.startsWith('data:image/')) { @@ -308,22 +308,24 @@ export class BrowserView extends Disposable { })()); } - try { - this._lastFavicon = await this._faviconRequestCache.get(url)!; - this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); - this._currentHistoryHandle?.update({ favicon: this._lastFavicon }); - // On success, stop searching + return this._faviconRequestCache.get(url)!; + }, + favicon => { + if (favicon === undefined && !this._lastFavicon) { return; - } catch (e) { - // On failure, just try the next one } - } - - // If we searched all favicons and none worked, clear the favicon - if (this._lastFavicon) { - this._lastFavicon = undefined; + this._lastFavicon = favicon; this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); - this._currentHistoryHandle?.update({ favicon: null }); + this._currentHistoryHandle?.update({ favicon: favicon ?? null }); + }, + this.logService, + )); + webContents.on('page-favicon-updated', (_event, favicons) => { + void faviconLoader.load(favicons).catch(error => this.logService.warn('[BrowserView] Failed to update favicon.', error)); + }); + webContents.on('did-start-navigation', (_event, _url, isInPlace, isMainFrame) => { + if (isMainFrame && !isInPlace) { + faviconLoader.invalidate(); } }); webContents.on('will-navigate', (event) => { @@ -613,9 +615,9 @@ export class BrowserView extends Disposable { } /** - * Get the current state of this browser view + * Get navigation state without screenshots or session data. */ - getState(): IBrowserViewState { + getNavigationState(): IBrowserViewNavigationState { const webContents = this._view.webContents; const url = webContents.getURL(); @@ -626,13 +628,20 @@ export class BrowserView extends Disposable { canGoBack: webContents.navigationHistory.canGoBack(), canGoForward: webContents.navigationHistory.canGoForward(), loading: webContents.isLoading(), + lastFavicon: this._lastFavicon, + lastError: this._lastError, + certificateError: this.session.trust.getCertificateError(url), + }; + } + + getState(): IBrowserViewState { + const webContents = this._view.webContents; + return { + ...this.getNavigationState(), focused: webContents.isFocused(), visible: this._view.getVisible(), isDevToolsOpen: webContents.isDevToolsOpened(), lastScreenshot: this._lastScreenshot, - lastFavicon: this._lastFavicon, - lastError: this._lastError, - certificateError: this.session.trust.getCertificateError(url), storageScope: this.session.storageScope, storageKeys: { ...this.session.history.storageKeys, ...this.session.permissions.storageKeys }, permissions: this.session.permissions.serialize(), diff --git a/src/vs/platform/browserView/electron-main/browserViewMainService.ts b/src/vs/platform/browserView/electron-main/browserViewMainService.ts index b929bfa8cb9361..2bb1c5a35bfa88 100644 --- a/src/vs/platform/browserView/electron-main/browserViewMainService.ts +++ b/src/vs/platform/browserView/electron-main/browserViewMainService.ts @@ -6,7 +6,7 @@ import { Emitter, Event } from '../../../base/common/event.js'; import { Disposable, DisposableMap } from '../../../base/common/lifecycle.js'; import { VSBuffer } from '../../../base/common/buffer.js'; -import { BrowserViewSessionSelector, BrowserViewStorageScope, isBrowserViewStorageScopeShareableWithAgent, IBrowserElementCommentsUpdate, IBrowserElementSelectionOptions, IBrowserViewAudience, IBrowserViewBounds, IBrowserViewState, IBrowserViewService, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, BrowserViewCommandId, IBrowserViewOwner, IBrowserViewInfo, IBrowserViewCreatedEvent, IBrowserViewEditorOpenOptions, IBrowserViewCreateOptions, IBrowserViewCreationContext, IBrowserViewWindowConfiguration, IBrowserDeviceProfile } from '../common/browserView.js'; +import { BrowserViewSessionSelector, BrowserViewStorageScope, isBrowserViewStorageScopeShareableWithAgent, IBrowserElementCommentsUpdate, IBrowserElementSelectionOptions, IBrowserViewAudience, IBrowserViewBounds, IBrowserViewState, IBrowserViewNavigationState, IBrowserViewService, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, BrowserViewCommandId, IBrowserViewOwner, IBrowserViewInfo, IBrowserViewCreatedEvent, IBrowserViewEditorOpenOptions, IBrowserViewCreateOptions, IBrowserViewCreationContext, IBrowserViewWindowConfiguration, IBrowserDeviceProfile } from '../common/browserView.js'; import { clipboard, Menu, MenuItem } from 'electron'; import { IEnvironmentMainService } from '../../environment/electron-main/environmentMainService.js'; import { createDecorator, IInstantiationService } from '../../instantiation/common/instantiation.js'; @@ -246,6 +246,10 @@ export class BrowserViewMainService extends Disposable implements IBrowserViewMa return this._getBrowserView(id).getState(); } + async getNavigationState(id: string): Promise { + return this._getBrowserView(id).getNavigationState(); + } + async setAudience(id: string, audience: IBrowserViewAudience, enabled: boolean): Promise { const view = this._getBrowserView(id); if (enabled && audience.type === 'agent') { diff --git a/src/vs/platform/browserView/test/common/browserFaviconLoader.test.ts b/src/vs/platform/browserView/test/common/browserFaviconLoader.test.ts new file mode 100644 index 00000000000000..83db3054ed929a --- /dev/null +++ b/src/vs/platform/browserView/test/common/browserFaviconLoader.test.ts @@ -0,0 +1,102 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { DeferredPromise } from '../../../../base/common/async.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { NullLogService } from '../../../log/common/log.js'; +import { BrowserFaviconLoader } from '../../common/browserFaviconLoader.js'; + +suite('BrowserFaviconLoader', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + function createLoader() { + const requests = new Map>(); + const applied: (string | undefined)[] = []; + const loader = store.add(new BrowserFaviconLoader(url => { + const request = new DeferredPromise(); + requests.set(url, request); + return request.p; + }, favicon => applied.push(favicon), new NullLogService())); + return { loader, requests, applied }; + } + + test('does not publish an older request that completes after the current request', async () => { + const { loader, requests, applied } = createLoader(); + const older = loader.load(['older']); + const current = loader.load(['current']); + await requests.get('current')!.complete('current-icon'); + await current; + await requests.get('older')!.complete('older-icon'); + await older; + + assert.deepStrictEqual(applied, ['current-icon']); + }); + + test('does not publish after a navigation invalidates the request', async () => { + const { loader, requests, applied } = createLoader(); + const pending = loader.load(['older']); + loader.invalidate(); + await requests.get('older')!.complete('older-icon'); + await pending; + + assert.deepStrictEqual(applied, []); + }); + + test('does not clear the current icon or fetch fallbacks after an older request fails', async () => { + const { loader, requests, applied } = createLoader(); + const older = loader.load(['older', 'fallback']); + loader.invalidate(); + const current = loader.load(['current']); + await requests.get('current')!.complete('current-icon'); + await current; + await requests.get('older')!.error(new Error('Older favicon unavailable')); + await older; + + assert.deepStrictEqual({ requested: [...requests.keys()], applied }, { + requested: ['older', 'current'], applied: ['current-icon'], + }); + }); + + test('tries the next favicon after a current request fails', async () => { + const { loader, requests, applied } = createLoader(); + const pending = loader.load(['missing', 'fallback']); + await requests.get('missing')!.error(new Error('Favicon unavailable')); + await requests.get('fallback')!.complete('fallback-icon'); + await pending; + + assert.deepStrictEqual(applied, ['fallback-icon']); + }); + + test('clears the favicon when all current candidates fail', async () => { + const { loader, requests, applied } = createLoader(); + const pending = loader.load(['missing']); + await requests.get('missing')!.error(new Error('Favicon unavailable')); + await pending; + + assert.deepStrictEqual(applied, [undefined]); + }); + + test('an empty favicon update supersedes an earlier request', async () => { + const { loader, requests, applied } = createLoader(); + const pending = loader.load(['older']); + await loader.load([]); + await requests.get('older')!.complete('older-icon'); + await pending; + + assert.deepStrictEqual(applied, [undefined]); + }); + + test('disposal prevents pending completions and new requests', async () => { + const { loader, requests, applied } = createLoader(); + const pending = loader.load(['older']); + loader.dispose(); + await requests.get('older')!.complete('older-icon'); + await pending; + await loader.load(['after-disposal']); + + assert.deepStrictEqual({ requested: [...requests.keys()], applied }, { requested: ['older'], applied: [] }); + }); +}); diff --git a/src/vs/workbench/contrib/browserView/common/browserView.ts b/src/vs/workbench/contrib/browserView/common/browserView.ts index 8dc66abd6fa950..b90929b782efe6 100644 --- a/src/vs/workbench/contrib/browserView/common/browserView.ts +++ b/src/vs/workbench/contrib/browserView/common/browserView.ts @@ -678,7 +678,7 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { } private async _synchronizeInitialNavigationState(): Promise { - const state = await this.browserViewService.getState(this.id); + const state = await this.browserViewService.getNavigationState(this.id); if (this._store.isDisposed) { return; } @@ -749,10 +749,13 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.loading, isSnapshot)) { return; } + const didChange = event.loading !== this._loading || !structuralEquals(event.error, this._error); this._navigationStateVersions.loading = event.navigationStateVersion; this._loading = event.loading; this._error = event.error; - this._onDidChangeLoadingState.fire(event); + if (!isSnapshot || didChange) { + this._onDidChangeLoadingState.fire(event); + } } private _updateFavicon(event: IBrowserViewFaviconChangeEvent, isSnapshot = false): void { diff --git a/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts b/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts index d3fb4229beb908..64aaf82052b674 100644 --- a/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts +++ b/src/vs/workbench/contrib/browserView/test/common/browserView.test.ts @@ -22,7 +22,7 @@ suite('BrowserViewModel', () => { test('only blocks disallowed pages that cannot be shared directly', () => { const browserViewService = upcastPartial({ destroyBrowserView: async () => { }, - getState: async () => createInitialState(BrowserViewStorageScope.Ephemeral, []), + getNavigationState: async () => createInitialState(BrowserViewStorageScope.Ephemeral, []), setBrowserZoomIndex: async () => { }, onDynamicDidChangePermissions: () => Event.None, onDynamicDidNavigate: () => Event.None, diff --git a/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts index 115acc8fad151e..57fa8e7f6e9c09 100644 --- a/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts +++ b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts @@ -102,7 +102,8 @@ suite('BrowserViewModel initial state handoff', () => { onDynamicDidChangeAreaSelectionActive: () => Event.None, onDynamicDidChangeAudiences: () => Event.None, onDynamicDidChangeRemoteStatus: () => Event.None, - getState: id => { + getState: async () => assert.fail('Reconciliation must not request screenshots or session data'), + getNavigationState: id => { trace.push(`snapshot requested ${id}`); return snapshot.p; }, @@ -207,6 +208,47 @@ suite('BrowserViewModel initial state handoff', () => { assert.deepStrictEqual({ title: model.title, navigations }, { title: childTitle, navigations: [] }); }); + test('does not emit loading changes for an unchanged snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(popup.state); + const loading: IBrowserViewLoadingEvent[] = []; + store.add(model.onDidChangeLoadingState(event => loading.push(event))); + await popup.snapshot.complete(popup.state); + + assert.deepStrictEqual({ loading: model.loading, events: loading }, { loading: false, events: [] }); + }); + + test('advances the version of an unchanged loading snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(popup.state); + const loading: boolean[] = []; + store.add(model.onDidChangeLoadingState(event => loading.push(event.loading))); + const snapshot = { ...popup.state, navigationStateVersion: popup.state.navigationStateVersion + 2 }; + await popup.snapshot.complete(snapshot); + popup.loading.fire({ navigationStateVersion: snapshot.navigationStateVersion - 1, loading: true }); + + assert.deepStrictEqual({ loading: model.loading, events: loading }, { loading: false, events: [] }); + }); + + test('emits loading changes for changed snapshot errors and subsequent native events', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(popup.state); + const loading: IBrowserViewLoadingEvent[] = []; + store.add(model.onDidChangeLoadingState(event => loading.push(event))); + const snapshot = { ...popup.state, lastError: { url: childUrl, errorCode: -105, errorDescription: 'ERR_NAME_NOT_RESOLVED' } }; + await popup.snapshot.complete(snapshot); + const nativeEvent = { navigationStateVersion: snapshot.navigationStateVersion + 1, loading: false, error: snapshot.lastError }; + popup.loading.fire(nativeEvent); + + assert.deepStrictEqual(loading, [ + { navigationStateVersion: snapshot.navigationStateVersion, loading: false, error: snapshot.lastError }, + nativeEvent, + ]); + }); + test('logs an initial snapshot failure', async () => { const popup = createPopup(); popup.adopt(); diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts index ef0fa1e8090d93..5c6f74973427b2 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewModel.test.ts @@ -43,7 +43,7 @@ suite('BrowserViewModel', () => { onDynamicDidChangeRemoteStatus: () => Event.None, onDynamicDidChangeAudiences: () => Event.None, destroyBrowserView: async () => { }, - getState: async () => initialState, + getNavigationState: async () => initialState, }); const browserViewWorkbenchService = upcastPartial({ isSharingAvailable: true, diff --git a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts index 40aa4460d4a9f7..737af4000eb18b 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-browser/browserViewWorkbenchService.test.ts @@ -107,7 +107,10 @@ suite('BrowserViewWorkbenchService popup handoff', () => { const browserService = new class extends mock() { override readonly onDidCreateBrowserView = created.event; override getBrowserViews() { return listedViews.p; } - override getState(id: string) { + override async getState(): Promise { + assert.fail('Reconciliation must not request the full browser snapshot'); + } + override getNavigationState(id: string) { trace.push(`snapshot ${id}`); return snapshot.p; } From 21526d09e87bf6cceaaa056a4e04297a1aba33bb Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sat, 12 Sep 2026 16:28:47 -0700 Subject: [PATCH 3/7] browser: clear favicon state across cross-host redirects Use the pending navigation URL for redirect chains and invalidate favicon work before clearing authoritative state. Cover snapshots, history, late completions, same-host and subframe redirects, and rejected navigations through actual BrowserView handlers with an injectable native view factory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browserView/electron-main/browserView.ts | 26 ++- .../electron-main/browserViewMainService.ts | 3 +- .../electron-main/browserViewFavicon.test.ts | 185 ++++++++++++++++++ 3 files changed, 205 insertions(+), 9 deletions(-) create mode 100644 src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index 776fbcbd476e16..d8858f9525dbab 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -132,6 +132,7 @@ export class BrowserView extends Disposable { private readonly _createChildView: (owner: IBrowserViewOwner, url: string, electronOptions: Electron.WebContentsViewConstructorOptions | undefined, editorOptions: IBrowserViewEditorOpenOptions) => BrowserView, openContextMenu: (view: BrowserView, params: Electron.ContextMenuParams) => void, options: Electron.WebContentsViewConstructorOptions | undefined, + createWebContentsView: (options: Electron.WebContentsViewConstructorOptions) => WebContentsView = options => new WebContentsView(options), @IWindowsMainService private readonly windowsMainService: IWindowsMainService, @IAuxiliaryWindowsMainService private readonly auxiliaryWindowsMainService: IAuxiliaryWindowsMainService, @ILogService private readonly logService: ILogService, @@ -157,7 +158,7 @@ export class BrowserView extends Disposable { focusOnNavigation: false }; - this._view = new WebContentsView({ + this._view = createWebContentsView({ webPreferences, // Passing an `undefined` webContents triggers an error in Electron. ...(options?.webContents ? { webContents: options.webContents } : {}) @@ -323,9 +324,18 @@ export class BrowserView extends Disposable { webContents.on('page-favicon-updated', (_event, favicons) => { void faviconLoader.load(favicons).catch(error => this.logService.warn('[BrowserView] Failed to update favicon.', error)); }); - webContents.on('did-start-navigation', (_event, _url, isInPlace, isMainFrame) => { + const resetFaviconForNavigation = (currentUrl: string, targetUrl: string) => { + // URL.parse (vs `new URL`) tolerates about:/blob:/empty strings without throwing. + if (URL.parse(targetUrl)?.host !== URL.parse(currentUrl)?.host) { + faviconLoader.invalidate(); + this._lastFavicon = undefined; + } + }; + let pendingNavigationUrl: string | undefined; + webContents.on('did-start-navigation', (_event, url, isInPlace, isMainFrame) => { if (isMainFrame && !isInPlace) { faviconLoader.invalidate(); + pendingNavigationUrl = url; } }); webContents.on('will-navigate', (event) => { @@ -333,16 +343,16 @@ export class BrowserView extends Disposable { event.preventDefault(); return; } - // URL.parse (vs `new URL`) tolerates about:/blob:/empty strings without throwing. - const host = URL.parse(event.url)?.host; - const currHost = URL.parse(this.webContents.getURL())?.host; - if (host !== currHost) { - this._lastFavicon = undefined; - } + resetFaviconForNavigation(webContents.getURL(), event.url); }); webContents.on('will-redirect', event => { if (this._redirectPinnedNavigation(event.url)) { event.preventDefault(); + return; + } + if (event.isMainFrame && !event.isSameDocument) { + resetFaviconForNavigation(pendingNavigationUrl ?? webContents.getURL(), event.url); + pendingNavigationUrl = event.url; } }); diff --git a/src/vs/platform/browserView/electron-main/browserViewMainService.ts b/src/vs/platform/browserView/electron-main/browserViewMainService.ts index 2bb1c5a35bfa88..f78e7dbe38aeaf 100644 --- a/src/vs/platform/browserView/electron-main/browserViewMainService.ts +++ b/src/vs/platform/browserView/electron-main/browserViewMainService.ts @@ -488,7 +488,8 @@ export class BrowserViewMainService extends Disposable implements IBrowserViewMa }, editorOptions, electronOptions); }, (v, params) => this.showContextMenu(v, params), - options + options, + undefined ); this.browserViews.set(id, view); if (windowConfiguration?.theme) { diff --git a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts new file mode 100644 index 00000000000000..252f99b004d827 --- /dev/null +++ b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts @@ -0,0 +1,185 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { EventEmitter } from 'events'; +import { DeferredPromise } from '../../../../base/common/async.js'; +import { Event } from '../../../../base/common/event.js'; +import { URI } from '../../../../base/common/uri.js'; +import { upcastDeepPartial, upcastPartial } from '../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { nextMacrotask, realTimeApi } from '../../../../base/test/common/virtualScheduling/index.js'; +import { IAuxiliaryWindowsMainService } from '../../../auxiliaryWindow/electron-main/auxiliaryWindows.js'; +import { NullLogService } from '../../../log/common/log.js'; +import { NullTelemetryService } from '../../../telemetry/common/telemetryUtils.js'; +import { ICodeWindow } from '../../../window/electron-main/window.js'; +import { IWindowsMainService } from '../../../windows/electron-main/windows.js'; +import { IBrowserHistoryItemHandle } from '../../common/browserHistory.js'; +import { BrowserViewStorageScope } from '../../common/browserView.js'; +import { BrowserSession } from '../../electron-main/browserSession.js'; +import { BrowserView } from '../../electron-main/browserView.js'; + +suite('BrowserView favicon navigation', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + const oldIcon = 'data:image/png;base64,b2xk'; + + function createView(associatedResource?: URI) { + const events = new EventEmitter(); + const requests = new Map>(); + const history: { url: string; favicon: string | null | undefined }[] = []; + let url = associatedResource?.toString() ?? 'https://first.example/page'; + let destroyed = false; + let childCreates = 0; + const electronSession = upcastPartial({ + fetch: input => { + const request = new DeferredPromise(); + requests.set(input.toString(), request); + return request.p; + }, + }); + const webContents: Electron.WebContents = upcastPartial({ + on: (event: string | symbol, listener: Parameters[1]) => { events.on(event, listener); return webContents; }, + removeListener: (event: string | symbol, listener: Parameters[1]) => { events.removeListener(event, listener); return webContents; }, + session: electronSession, + ipc: upcastPartial({ on: () => webContents.ipc }), + getURL: () => url, + getTitle: () => 'Test page', + getUserAgent: () => 'Test', + getOrCreateDevToolsTargetId: () => 'target', + isDestroyed: () => destroyed, + isLoading: () => false, + setWindowOpenHandler: () => { }, + setZoomFactor: () => { }, + setVisualZoomLevelLimits: async () => { }, + close: () => { destroyed = true; events.emit('destroyed'); }, + navigationHistory: upcastPartial({ + canGoBack: () => false, canGoForward: () => false, getActiveIndex: () => history.length, + }), + debugger: upcastPartial({ + isAttached: () => true, + sendCommand: async () => ({}), + removeListener: () => webContents.debugger, + detach: () => { }, + }), + }); + const nativeView = upcastPartial({ + webContents, setBounds: () => { }, setVisible: () => { }, setBackgroundColor: () => { }, + }); + const session = upcastDeepPartial({ + electronSession, + storageScope: BrowserViewStorageScope.Ephemeral, + remote: { onDidStart: Event.None, onDidStop: Event.None, isRemote: false }, + permissions: { onDidRequestPermission: Event.None, onDidRequestDevice: Event.None, onDidChange: Event.None }, + trust: { installCertErrorHandler: () => { }, getCertificateError: () => undefined }, + history: { + add: (entryUrl: string, _title: string, favicon: string | undefined) => { + const entry: { url: string; favicon: string | null | undefined } = { url: entryUrl, favicon }; + history.push(entry); + return upcastPartial({ update: changes => { Object.assign(entry, changes); } }); + }, + }, + }); + const owner = upcastPartial({ + onDidClose: Event.None, onWillLoad: Event.None, + win: upcastDeepPartial({ contentView: { addChildView: () => { } } }), + }); + const view: BrowserView = store.add(new BrowserView( + 'view', { windowId: 1 }, { type: 'user' }, associatedResource, session, + () => { childCreates++; return view; }, () => { }, undefined, () => nativeView, + upcastPartial({ getWindowById: () => owner }), + upcastPartial({}), new NullLogService(), NullTelemetryService, + )); + const settle = () => new Promise(resolve => nextMacrotask(realTimeApi, resolve)); + const commit = (target: string) => { + url = target; + events.emit('did-navigate', {}, url); + }; + commit(url); + const navigate = (target: string) => { + const event = { url: target, preventDefault: () => assert.fail('Unexpected navigation rejection') }; + events.emit('will-navigate', event); + events.emit('did-start-navigation', {}, target, false, true); + }; + const redirect = (target: string, isMainFrame = true) => { + let prevented = false; + events.emit('will-redirect', { + url: target, isMainFrame, isSameDocument: false, preventDefault: () => { prevented = true; }, + }); + return prevented; + }; + const setIcon = async (icon: string) => { + events.emit('page-favicon-updated', {}, [icon]); + await settle(); + }; + return { view, requests, history, events, navigate, redirect, commit, setIcon, settle, get childCreates() { return childCreates; } }; + } + + test('clears the authoritative icon before a cross-host redirect commits', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://first.example/redirect'); + testCase.redirect('https://second.example/destination'); + const beforeCommit = testCase.view.getNavigationState().lastFavicon; + testCase.commit('https://second.example/destination'); + + assert.deepStrictEqual({ beforeCommit, snapshotIcon: testCase.view.getNavigationState().lastFavicon, history: testCase.history }, { + beforeCommit: undefined, + snapshotIcon: undefined, + history: [ + { url: 'https://first.example/page', favicon: oldIcon }, + { url: 'https://second.example/destination', favicon: undefined }, + ], + }); + }); + + test('discards a favicon request started after navigation but before the redirect', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://first.example/redirect'); + testCase.events.emit('page-favicon-updated', {}, ['https://first.example/intermediate.png']); + testCase.redirect('https://second.example/destination'); + testCase.commit('https://second.example/destination'); + await testCase.requests.get('https://first.example/intermediate.png')!.complete(new Response('stale-icon', { headers: { 'content-type': 'image/png' } })); + await testCase.settle(); + + assert.deepStrictEqual({ snapshotIcon: testCase.view.getNavigationState().lastFavicon, committedIcon: testCase.history[1].favicon }, { + snapshotIcon: undefined, committedIcon: undefined, + }); + }); + + test('clears intermediate icons when a redirect chain returns to the original host', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://first.example/redirect'); + testCase.redirect('https://second.example/intermediate'); + await testCase.setIcon('data:image/png;base64,aW50ZXJtZWRpYXRl'); + testCase.redirect('https://first.example/destination'); + + assert.strictEqual(testCase.view.getNavigationState().lastFavicon, undefined); + }); + + test('keeps the icon for same-host redirects and cross-host subframe redirects', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://first.example/redirect'); + testCase.redirect('https://first.example/destination'); + const sameHost = testCase.view.getNavigationState().lastFavicon; + testCase.redirect('https://second.example/frame', false); + + assert.deepStrictEqual({ sameHost, afterSubframe: testCase.view.getNavigationState().lastFavicon }, { + sameHost: oldIcon, afterSubframe: oldIcon, + }); + }); + + test('does not clear the icon when the redirect is diverted to a new editor', async () => { + const testCase = createView(URI.file('/workspace/page.html')); + await testCase.setIcon(oldIcon); + const prevented = testCase.redirect('https://second.example/destination'); + + assert.deepStrictEqual({ prevented, childCreates: testCase.childCreates, favicon: testCase.view.getNavigationState().lastFavicon }, { + prevented: true, childCreates: 1, favicon: oldIcon, + }); + }); +}); From ae55c4d79331d7889970d40e63997a80f50a0f37 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sat, 12 Sep 2026 16:39:43 -0700 Subject: [PATCH 4/7] browser: avoid main-only Electron named exports Access main-process Electron APIs through its default export so BrowserView can be dynamically imported by the renderer unit runner with its injected native view. Reproduce the CI ESM linking failure and validate all 47 targeted tests with native ESM loading instead of CommonJS bundling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/vs/platform/browserView/electron-main/browserView.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index d8858f9525dbab..c8800461aa99aa 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { screen, WebContentsView, webContents } from 'electron'; +import electron, { type WebContentsView } from 'electron'; import { Disposable } from '../../../base/common/lifecycle.js'; import { Emitter, Event } from '../../../base/common/event.js'; import { VSBuffer } from '../../../base/common/buffer.js'; @@ -132,7 +132,7 @@ export class BrowserView extends Disposable { private readonly _createChildView: (owner: IBrowserViewOwner, url: string, electronOptions: Electron.WebContentsViewConstructorOptions | undefined, editorOptions: IBrowserViewEditorOpenOptions) => BrowserView, openContextMenu: (view: BrowserView, params: Electron.ContextMenuParams) => void, options: Electron.WebContentsViewConstructorOptions | undefined, - createWebContentsView: (options: Electron.WebContentsViewConstructorOptions) => WebContentsView = options => new WebContentsView(options), + createWebContentsView: (options: Electron.WebContentsViewConstructorOptions) => WebContentsView = options => new electron.WebContentsView(options), @IWindowsMainService private readonly windowsMainService: IWindowsMainService, @IAuxiliaryWindowsMainService private readonly auxiliaryWindowsMainService: IAuxiliaryWindowsMainService, @ILogService private readonly logService: ILogService, @@ -925,7 +925,7 @@ export class BrowserView extends Disposable { // while the page is paused at a breakpoint. Fall back to the primary display if no host // window can be resolved (e.g. during teardown). const hostWindow = this._hostWindow; - const display = hostWindow ? screen.getDisplayMatching(hostWindow.getBounds()) : screen.getPrimaryDisplay(); + const display = hostWindow ? electron.screen.getDisplayMatching(hostWindow.getBounds()) : electron.screen.getPrimaryDisplay(); const devicePixelRatio = display.scaleFactor; const maxClipDimension = BrowserView.MAX_FULL_PAGE_SCREENSHOT_DIMENSION / Math.max(devicePixelRatio, 1); const scale = Math.min(1, maxClipDimension / Math.max(clipWidth, clipHeight)); @@ -1121,7 +1121,7 @@ export class BrowserView extends Disposable { return undefined; } - const contents = webContents.fromId(windowId); + const contents = electron.webContents.fromId(windowId); if (!contents) { return undefined; } From fc0c6378f0d8b567075d185ca848cd54eba49872 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sat, 12 Sep 2026 19:55:34 -0700 Subject: [PATCH 5/7] browser: synchronize favicon state at navigation boundaries Preserve pending favicon work for diverted previews and clear authoritative state for accepted programmatic navigation. Publish silently changed icons after commit without rewriting the old history entry. Reconcile title and favicon snapshots without synthetic navigation, while clearing restored label fallbacks. Cover Electron event order and real native-to-model delivery with deterministic ESM regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browserView/electron-main/browserView.ts | 19 ++- .../electron-main/browserViewFavicon.test.ts | 154 ++++++------------ .../electron-main/browserViewTestUtils.ts | 153 +++++++++++++++++ .../browserView/common/browserEditorInput.ts | 14 +- .../contrib/browserView/common/browserView.ts | 21 ++- .../test/common/browserViewModelState.test.ts | 65 +++++++- .../browserViewFaviconSync.test.ts | 76 +++++++++ 7 files changed, 380 insertions(+), 122 deletions(-) create mode 100644 src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts create mode 100644 src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index c8800461aa99aa..95756c2a923a74 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -286,6 +286,11 @@ export class BrowserView extends Disposable { }); // Favicon events + let lastEmittedFavicon: string | undefined; + const fireFaviconEvent = () => { + lastEmittedFavicon = this._lastFavicon; + this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); + }; const faviconLoader = this._register(new BrowserFaviconLoader( url => { if (!this._faviconRequestCache.has(url)) { @@ -316,7 +321,7 @@ export class BrowserView extends Disposable { return; } this._lastFavicon = favicon; - this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); + fireFaviconEvent(); this._currentHistoryHandle?.update({ favicon: favicon ?? null }); }, this.logService, @@ -333,7 +338,8 @@ export class BrowserView extends Disposable { }; let pendingNavigationUrl: string | undefined; webContents.on('did-start-navigation', (_event, url, isInPlace, isMainFrame) => { - if (isMainFrame && !isInPlace) { + if (isMainFrame && !isInPlace && !this._shouldRedirectPinnedNavigation(url)) { + resetFaviconForNavigation(webContents.getURL(), url); faviconLoader.invalidate(); pendingNavigationUrl = url; } @@ -372,6 +378,9 @@ export class BrowserView extends Disposable { certificateError: this.session.trust.getCertificateError(url) }); this._recordNavigation(url); + if (lastEmittedFavicon !== this._lastFavicon) { + fireFaviconEvent(); + } }; const fireLoadingEvent = (loading: boolean) => { @@ -779,8 +788,12 @@ export class BrowserView extends Disposable { await this._view.webContents.loadURL(url); } + private _shouldRedirectPinnedNavigation(url: string): boolean { + return !!this.associatedResource && !isBrowserViewAssociatedResourceNavigation(this.associatedResource, url); + } + private _redirectPinnedNavigation(url: string): boolean { - if (!this.associatedResource || isBrowserViewAssociatedResourceNavigation(this.associatedResource, url)) { + if (!this._shouldRedirectPinnedNavigation(url)) { return false; } diff --git a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts index 252f99b004d827..1c7dcf1b07118b 100644 --- a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts +++ b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts @@ -4,117 +4,15 @@ *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; -import { EventEmitter } from 'events'; -import { DeferredPromise } from '../../../../base/common/async.js'; -import { Event } from '../../../../base/common/event.js'; import { URI } from '../../../../base/common/uri.js'; -import { upcastDeepPartial, upcastPartial } from '../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { nextMacrotask, realTimeApi } from '../../../../base/test/common/virtualScheduling/index.js'; -import { IAuxiliaryWindowsMainService } from '../../../auxiliaryWindow/electron-main/auxiliaryWindows.js'; -import { NullLogService } from '../../../log/common/log.js'; -import { NullTelemetryService } from '../../../telemetry/common/telemetryUtils.js'; -import { ICodeWindow } from '../../../window/electron-main/window.js'; -import { IWindowsMainService } from '../../../windows/electron-main/windows.js'; -import { IBrowserHistoryItemHandle } from '../../common/browserHistory.js'; -import { BrowserViewStorageScope } from '../../common/browserView.js'; -import { BrowserSession } from '../../electron-main/browserSession.js'; -import { BrowserView } from '../../electron-main/browserView.js'; +import { createTestBrowserView } from './browserViewTestUtils.js'; suite('BrowserView favicon navigation', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); const oldIcon = 'data:image/png;base64,b2xk'; - function createView(associatedResource?: URI) { - const events = new EventEmitter(); - const requests = new Map>(); - const history: { url: string; favicon: string | null | undefined }[] = []; - let url = associatedResource?.toString() ?? 'https://first.example/page'; - let destroyed = false; - let childCreates = 0; - const electronSession = upcastPartial({ - fetch: input => { - const request = new DeferredPromise(); - requests.set(input.toString(), request); - return request.p; - }, - }); - const webContents: Electron.WebContents = upcastPartial({ - on: (event: string | symbol, listener: Parameters[1]) => { events.on(event, listener); return webContents; }, - removeListener: (event: string | symbol, listener: Parameters[1]) => { events.removeListener(event, listener); return webContents; }, - session: electronSession, - ipc: upcastPartial({ on: () => webContents.ipc }), - getURL: () => url, - getTitle: () => 'Test page', - getUserAgent: () => 'Test', - getOrCreateDevToolsTargetId: () => 'target', - isDestroyed: () => destroyed, - isLoading: () => false, - setWindowOpenHandler: () => { }, - setZoomFactor: () => { }, - setVisualZoomLevelLimits: async () => { }, - close: () => { destroyed = true; events.emit('destroyed'); }, - navigationHistory: upcastPartial({ - canGoBack: () => false, canGoForward: () => false, getActiveIndex: () => history.length, - }), - debugger: upcastPartial({ - isAttached: () => true, - sendCommand: async () => ({}), - removeListener: () => webContents.debugger, - detach: () => { }, - }), - }); - const nativeView = upcastPartial({ - webContents, setBounds: () => { }, setVisible: () => { }, setBackgroundColor: () => { }, - }); - const session = upcastDeepPartial({ - electronSession, - storageScope: BrowserViewStorageScope.Ephemeral, - remote: { onDidStart: Event.None, onDidStop: Event.None, isRemote: false }, - permissions: { onDidRequestPermission: Event.None, onDidRequestDevice: Event.None, onDidChange: Event.None }, - trust: { installCertErrorHandler: () => { }, getCertificateError: () => undefined }, - history: { - add: (entryUrl: string, _title: string, favicon: string | undefined) => { - const entry: { url: string; favicon: string | null | undefined } = { url: entryUrl, favicon }; - history.push(entry); - return upcastPartial({ update: changes => { Object.assign(entry, changes); } }); - }, - }, - }); - const owner = upcastPartial({ - onDidClose: Event.None, onWillLoad: Event.None, - win: upcastDeepPartial({ contentView: { addChildView: () => { } } }), - }); - const view: BrowserView = store.add(new BrowserView( - 'view', { windowId: 1 }, { type: 'user' }, associatedResource, session, - () => { childCreates++; return view; }, () => { }, undefined, () => nativeView, - upcastPartial({ getWindowById: () => owner }), - upcastPartial({}), new NullLogService(), NullTelemetryService, - )); - const settle = () => new Promise(resolve => nextMacrotask(realTimeApi, resolve)); - const commit = (target: string) => { - url = target; - events.emit('did-navigate', {}, url); - }; - commit(url); - const navigate = (target: string) => { - const event = { url: target, preventDefault: () => assert.fail('Unexpected navigation rejection') }; - events.emit('will-navigate', event); - events.emit('did-start-navigation', {}, target, false, true); - }; - const redirect = (target: string, isMainFrame = true) => { - let prevented = false; - events.emit('will-redirect', { - url: target, isMainFrame, isSameDocument: false, preventDefault: () => { prevented = true; }, - }); - return prevented; - }; - const setIcon = async (icon: string) => { - events.emit('page-favicon-updated', {}, [icon]); - await settle(); - }; - return { view, requests, history, events, navigate, redirect, commit, setIcon, settle, get childCreates() { return childCreates; } }; - } + const createView = (associatedResource?: URI) => createTestBrowserView(store, associatedResource); test('clears the authoritative icon before a cross-host redirect commits', async () => { const testCase = createView(); @@ -141,8 +39,7 @@ suite('BrowserView favicon navigation', () => { testCase.events.emit('page-favicon-updated', {}, ['https://first.example/intermediate.png']); testCase.redirect('https://second.example/destination'); testCase.commit('https://second.example/destination'); - await testCase.requests.get('https://first.example/intermediate.png')!.complete(new Response('stale-icon', { headers: { 'content-type': 'image/png' } })); - await testCase.settle(); + await testCase.completeFavicon('https://first.example/intermediate.png', 'stale-icon'); assert.deepStrictEqual({ snapshotIcon: testCase.view.getNavigationState().lastFavicon, committedIcon: testCase.history[1].favicon }, { snapshotIcon: undefined, committedIcon: undefined, @@ -182,4 +79,49 @@ suite('BrowserView favicon navigation', () => { prevented: true, childCreates: 1, favicon: oldIcon, }); }); + + test('preserves a pending favicon when navigation is diverted after did-start-navigation', async () => { + const testCase = createView(URI.file('/workspace/page.html')); + const iconUrl = 'https://first.example/pending.png'; + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + const prevented = testCase.navigate('https://second.example/destination'); + await testCase.completeFavicon(iconUrl, 'kept'); + + assert.deepStrictEqual({ + prevented, childCreates: testCase.childCreates, + favicon: testCase.view.getNavigationState().lastFavicon, historyIcon: testCase.history[0].favicon, + }, { + prevented: true, childCreates: 1, + favicon: 'data:image/png;base64,a2VwdA==', historyIcon: 'data:image/png;base64,a2VwdA==', + }); + }); + + test('still discards pending work after an accepted document navigation', async () => { + const testCase = createView(); + const iconUrl = 'https://first.example/pending.png'; + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + const prevented = testCase.navigate('https://first.example/next'); + await testCase.completeFavicon(iconUrl, 'stale'); + + assert.deepStrictEqual({ prevented, favicon: testCase.view.getNavigationState().lastFavicon }, { + prevented: false, favicon: undefined, + }); + }); + + for (const method of ['loadURL', 'back', 'forward'] as const) { + test(`clears cross-host favicon state for ${method} without will-navigate`, async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + await testCase.navigateProgrammatically(method, 'https://second.example/destination'); + const beforeCommit = testCase.view.getNavigationState().lastFavicon; + testCase.commit('https://second.example/destination'); + + assert.deepStrictEqual({ + calls: testCase.programmaticCalls, beforeCommit, + snapshotIcon: testCase.view.getNavigationState().lastFavicon, historyIcon: testCase.history[1].favicon, + }, { + calls: [method], beforeCommit: undefined, snapshotIcon: undefined, historyIcon: undefined, + }); + }); + } }); diff --git a/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts new file mode 100644 index 00000000000000..f66c2cada3103a --- /dev/null +++ b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts @@ -0,0 +1,153 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { EventEmitter } from 'events'; +import { DeferredPromise } from '../../../../base/common/async.js'; +import { Event } from '../../../../base/common/event.js'; +import { DisposableStore } from '../../../../base/common/lifecycle.js'; +import { URI } from '../../../../base/common/uri.js'; +import { upcastDeepPartial, upcastPartial } from '../../../../base/test/common/mock.js'; +import { nextMacrotask, realTimeApi } from '../../../../base/test/common/virtualScheduling/index.js'; +import { IAuxiliaryWindowsMainService } from '../../../auxiliaryWindow/electron-main/auxiliaryWindows.js'; +import { NullLogService } from '../../../log/common/log.js'; +import { NullTelemetryService } from '../../../telemetry/common/telemetryUtils.js'; +import { ICodeWindow } from '../../../window/electron-main/window.js'; +import { IWindowsMainService } from '../../../windows/electron-main/windows.js'; +import { IBrowserHistoryItemHandle } from '../../common/browserHistory.js'; +import { BrowserViewStorageScope } from '../../common/browserView.js'; +import { BrowserSession } from '../../electron-main/browserSession.js'; +import { BrowserView } from '../../electron-main/browserView.js'; + +export function createTestBrowserView(store: Pick, associatedResource?: URI) { + const events = new EventEmitter(); + const requests = new Map>(); + const history: { url: string; favicon: string | null | undefined }[] = []; + const programmaticCalls: string[] = []; + let url = associatedResource?.toString() ?? 'https://first.example/page'; + let historyTarget = url; + let destroyed = false; + let childCreates = 0; + const startNavigation = (target: string, isMainFrame = true, isSameDocument = false) => { + events.emit('did-start-navigation', { url: target, isMainFrame, isSameDocument }, target, isSameDocument, isMainFrame); + }; + const electronSession = upcastPartial({ + fetch: input => { + const request = new DeferredPromise(); + requests.set(input.toString(), request); + return request.p; + }, + }); + const webContents: Electron.WebContents = upcastPartial({ + on: (event: string | symbol, listener: Parameters[1]) => { events.on(event, listener); return webContents; }, + removeListener: (event: string | symbol, listener: Parameters[1]) => { events.removeListener(event, listener); return webContents; }, + session: electronSession, + ipc: upcastPartial({ on: () => webContents.ipc }), + getURL: () => url, + getTitle: () => 'Test page', + getUserAgent: () => 'Test', + getOrCreateDevToolsTargetId: () => 'target', + isDestroyed: () => destroyed, + isLoading: () => false, + isFocused: () => false, + isDevToolsOpened: () => false, + setWindowOpenHandler: () => { }, + setZoomFactor: () => { }, + setVisualZoomLevelLimits: async () => { }, + loadURL: async target => { programmaticCalls.push('loadURL'); startNavigation(target); }, + close: () => { destroyed = true; events.emit('destroyed'); }, + navigationHistory: upcastPartial({ + canGoBack: () => true, canGoForward: () => true, getActiveIndex: () => history.length, + goBack: () => { programmaticCalls.push('back'); startNavigation(historyTarget); }, + goForward: () => { programmaticCalls.push('forward'); startNavigation(historyTarget); }, + }), + debugger: upcastPartial({ + isAttached: () => true, + sendCommand: async () => ({}), + removeListener: () => webContents.debugger, + detach: () => { }, + }), + }); + const nativeView = upcastPartial({ + webContents, setBounds: () => { }, setVisible: () => { }, getVisible: () => false, setBackgroundColor: () => { }, + }); + const session = upcastDeepPartial({ + electronSession, + storageScope: BrowserViewStorageScope.Ephemeral, + remote: { onDidStart: Event.None, onDidStop: Event.None, isRemote: false, whenReady: Promise.resolve() }, + permissions: { + onDidRequestPermission: Event.None, onDidRequestDevice: Event.None, onDidChange: Event.None, + storageKeys: {}, serialize: () => ({ origins: {} }), + }, + trust: { installCertErrorHandler: () => { }, getCertificateError: () => undefined }, + history: { + storageKeys: {}, + add: (entryUrl: string, _title: string, favicon: string | undefined) => { + const entry: { url: string; favicon: string | null | undefined } = { url: entryUrl, favicon }; + history.push(entry); + return upcastPartial({ update: changes => { Object.assign(entry, changes); } }); + }, + }, + }); + const owner = upcastPartial({ + onDidClose: Event.None, onWillLoad: Event.None, + win: upcastDeepPartial({ contentView: { addChildView: () => { } } }), + }); + const view: BrowserView = store.add(new BrowserView( + 'view', { windowId: 1 }, { type: 'user' }, associatedResource, session, + () => { childCreates++; return view; }, () => { }, undefined, () => nativeView, + upcastPartial({ getWindowById: () => owner }), + upcastPartial({}), new NullLogService(), NullTelemetryService, + )); + const settle = () => new Promise(resolve => nextMacrotask(realTimeApi, resolve)); + const commit = (target: string) => { + url = target; + events.emit('did-navigate', {}, url); + }; + commit(url); + const navigate = (target: string) => { + let prevented = false; + startNavigation(target); + events.emit('will-navigate', { url: target, preventDefault: () => { prevented = true; } }); + return prevented; + }; + const navigateProgrammatically = async (method: 'loadURL' | 'back' | 'forward', target: string) => { + historyTarget = target; + if (method === 'loadURL') { + await view.loadURL(target); + } else if (method === 'back') { + view.goBack(); + } else { + view.goForward(); + } + }; + const redirect = (target: string, isMainFrame = true) => { + let prevented = false; + events.emit('will-redirect', { + url: target, isMainFrame, isSameDocument: false, preventDefault: () => { prevented = true; }, + }); + return prevented; + }; + const setIcon = async (icon: string) => { + events.emit('page-favicon-updated', {}, [icon]); + await settle(); + }; + const completeFavicon = async (iconUrl: string, contents: string, status = 200) => { + const request = requests.get(iconUrl); + assert.ok(request, `No pending favicon request for ${iconUrl}`); + const bytes = new TextEncoder().encode(contents); + const body = new ArrayBuffer(bytes.byteLength); + new Uint8Array(body).set(bytes); + await request.complete(upcastPartial({ + ok: status >= 200 && status < 300, status, statusText: status === 404 ? 'Not Found' : 'OK', + headers: new Headers({ 'content-type': 'image/png' }), arrayBuffer: async () => body, + })); + await settle(); + }; + return { + view, history, events, navigate, startNavigation, navigateProgrammatically, programmaticCalls, + redirect, commit, setIcon, completeFavicon, settle, get childCreates() { return childCreates; }, + }; +} diff --git a/src/vs/workbench/contrib/browserView/common/browserEditorInput.ts b/src/vs/workbench/contrib/browserView/common/browserEditorInput.ts index f276d27ee9535a..aed7dfe527d3a5 100644 --- a/src/vs/workbench/contrib/browserView/common/browserEditorInput.ts +++ b/src/vs/workbench/contrib/browserView/common/browserEditorInput.ts @@ -124,8 +124,18 @@ export class BrowserEditorInput extends EditorInput { })); // Listen for label-relevant changes to fire onDidChangeLabel - this._modelStore.add(this._model.onDidChangeTitle(() => this._onDidChangeLabel.fire())); - this._modelStore.add(this._model.onDidChangeFavicon(() => this._onDidChangeLabel.fire())); + this._modelStore.add(this._model.onDidChangeTitle(() => { + if (this._initialData.title !== undefined) { + this._initialData = { ...this._initialData, title: undefined }; + } + this._onDidChangeLabel.fire(); + })); + this._modelStore.add(this._model.onDidChangeFavicon(() => { + if (this._initialData.favicon !== undefined) { + this._initialData = { ...this._initialData, favicon: undefined }; + } + this._onDidChangeLabel.fire(); + })); this._modelStore.add(this._model.onDidChangeLoadingState(() => this._onDidChangeLabel.fire())); this._modelStore.add(this._model.onDidNavigate(() => { this._initialData = { ...this._initialData, title: undefined, favicon: undefined }; diff --git a/src/vs/workbench/contrib/browserView/common/browserView.ts b/src/vs/workbench/contrib/browserView/common/browserView.ts index b90929b782efe6..f2585f71c0e45a 100644 --- a/src/vs/workbench/contrib/browserView/common/browserView.ts +++ b/src/vs/workbench/contrib/browserView/common/browserView.ts @@ -698,10 +698,13 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { const didChange = event.url !== this._url || event.canGoBack !== this._canGoBack || event.canGoForward !== this._canGoForward - || !structuralEquals(event.certificateError, this._certificateError) - || (shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.title, isSnapshot) && event.title !== this._title); + || !structuralEquals(event.certificateError, this._certificateError); this._navigationStateVersions.navigation = event.navigationStateVersion; + if (isSnapshot && !didChange) { + return; + } + if (shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.favicon, isSnapshot) && URL.parse(event.url)?.host !== URL.parse(this._url)?.host) { this._favicon = undefined; this._navigationStateVersions.favicon = event.navigationStateVersion; @@ -717,10 +720,6 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { this._certificateError = event.certificateError; this._updateSharingState(); - if (isSnapshot && !didChange) { - return; - } - // Chromium resets zoom on cross-origin navigation, even when the host is unchanged. void this.setBrowserZoomIndex( this.zoomService.getEffectiveZoomIndex(this._zoomHost, this._isInMemory), @@ -740,9 +739,12 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.title, isSnapshot)) { return; } + const didChange = event.title !== this._title; this._navigationStateVersions.title = event.navigationStateVersion; this._title = event.title; - this._onDidChangeTitle.fire(event); + if (!isSnapshot || didChange) { + this._onDidChangeTitle.fire(event); + } } private _updateLoadingState(event: IBrowserViewLoadingEvent, isSnapshot = false): void { @@ -762,9 +764,12 @@ export class BrowserViewModel extends Disposable implements IBrowserViewModel { if (!shouldApplyNavigationState(event.navigationStateVersion, this._navigationStateVersions.favicon, isSnapshot)) { return; } + const didChange = event.favicon !== this._favicon; this._navigationStateVersions.favicon = event.navigationStateVersion; this._favicon = event.favicon; - this._onDidChangeFavicon.fire(event); + if (!isSnapshot || didChange) { + this._onDidChangeFavicon.fire(event); + } } get url(): string { return this._url; } diff --git a/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts index 57fa8e7f6e9c09..bc855342b81d1d 100644 --- a/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts +++ b/src/vs/workbench/contrib/browserView/test/common/browserViewModelState.test.ts @@ -16,7 +16,7 @@ import { IAgentNetworkFilterService } from '../../../../../platform/networkFilte import { IStorageService } from '../../../../../platform/storage/common/storage.js'; import { NullTelemetryService } from '../../../../../platform/telemetry/common/telemetryUtils.js'; import { IThemeService } from '../../../../../platform/theme/common/themeService.js'; -import { BrowserEditorInput } from '../../common/browserEditorInput.js'; +import { BrowserEditorInput, IBrowserEditorInputData } from '../../common/browserEditorInput.js'; import { BrowserViewModel, IBrowserViewWorkbenchService } from '../../common/browserView.js'; import { IBrowserZoomService } from '../../common/browserZoomService.js'; @@ -130,7 +130,7 @@ suite('BrowserViewModel initial state handoff', () => { loading.fire({ navigationStateVersion: state.navigationStateVersion, loading: false }); }; - const adopt = (creationState = initialState) => { + const adopt = (creationState = initialState, presentation: Pick = {}) => { trace.push('create model child'); const model = store.add(new BrowserViewModel( 'child', { windowId: 1 }, { type: 'user' }, undefined, creationState, service, workbenchService, @@ -141,7 +141,7 @@ suite('BrowserViewModel initial state handoff', () => { )); trace.push('create editor child'); const input = store.add(new BrowserEditorInput( - { id: 'child', url: childUrl }, async () => model, + { id: 'child', url: childUrl, ...presentation }, async () => model, upcastPartial({}), upcastPartial({}), NullTelemetryService, workbenchService, )); input.model = model; @@ -208,6 +208,65 @@ suite('BrowserViewModel initial state handoff', () => { assert.deepStrictEqual({ title: model.title, navigations }, { title: childTitle, navigations: [] }); }); + test('only emits a title change for a title-only snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model, input } = popup.adopt(popup.state); + const events: string[] = []; + store.add(model.onDidNavigate(() => events.push('navigation'))); + store.add(model.onDidChangeTitle(() => events.push('title'))); + store.add(model.onDidChangeFavicon(() => events.push('favicon'))); + store.add(model.onDidChangeLoadingState(() => events.push('loading'))); + await popup.snapshot.complete({ ...popup.state, navigationStateVersion: popup.state.navigationStateVersion + 1, title: 'Updated title' }); + + assert.deepStrictEqual({ title: model.title, label: input.getName(), events }, { + title: 'Updated title', label: 'Updated title', events: ['title'], + }); + }); + + test('only emits a favicon change for a favicon-only snapshot', async () => { + const popup = createPopup(); + popup.commit(); + const { model } = popup.adopt(popup.state); + const events: string[] = []; + store.add(model.onDidNavigate(() => events.push('navigation'))); + store.add(model.onDidChangeTitle(() => events.push('title'))); + store.add(model.onDidChangeFavicon(() => events.push('favicon'))); + store.add(model.onDidChangeLoadingState(() => events.push('loading'))); + await popup.snapshot.complete({ ...popup.state, navigationStateVersion: popup.state.navigationStateVersion + 1, lastFavicon: 'data:image/png;base64,aWNvbg==' }); + + assert.deepStrictEqual({ favicon: model.favicon, events }, { + favicon: 'data:image/png;base64,aWNvbg==', events: ['favicon'], + }); + }); + + test('a title-only snapshot can clear restored presentation without navigation', async () => { + const popup = createPopup(); + popup.commit(); + const { model, input } = popup.adopt(popup.state, { title: childTitle }); + const navigations: string[] = []; + store.add(model.onDidNavigate(event => navigations.push(event.url))); + await popup.snapshot.complete({ ...popup.state, title: '' }); + + assert.deepStrictEqual({ modelTitle: model.title, label: input.getName(), navigations }, { + modelTitle: '', label: 'localhost', navigations: [], + }); + }); + + test('a favicon-only snapshot can clear restored presentation without navigation', async () => { + const popup = createPopup(); + popup.commit(); + const favicon = 'data:image/png;base64,aWNvbg=='; + const { model, input } = popup.adopt({ ...popup.state, lastFavicon: favicon }, { favicon }); + const navigations: string[] = []; + store.add(model.onDidNavigate(event => navigations.push(event.url))); + await popup.snapshot.complete(popup.state); + + assert.deepStrictEqual({ modelIcon: model.favicon, editorIcon: input.favicon, navigations }, { + modelIcon: undefined, editorIcon: undefined, navigations: [], + }); + }); + test('does not emit loading changes for an unchanged snapshot', async () => { const popup = createPopup(); popup.commit(); diff --git a/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts new file mode 100644 index 00000000000000..230dff048915f5 --- /dev/null +++ b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts @@ -0,0 +1,76 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { Event } from '../../../../../base/common/event.js'; +import { DisposableStore } from '../../../../../base/common/lifecycle.js'; +import { upcastPartial } from '../../../../../base/test/common/mock.js'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { browserZoomDefaultIndex, IBrowserViewService } from '../../../../../platform/browserView/common/browserView.js'; +import { createTestBrowserView } from '../../../../../platform/browserView/test/electron-main/browserViewTestUtils.js'; +import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; +import { NullLogService } from '../../../../../platform/log/common/log.js'; +import { IAgentNetworkFilterService } from '../../../../../platform/networkFilter/common/networkFilterService.js'; +import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { NullTelemetryService } from '../../../../../platform/telemetry/common/telemetryUtils.js'; +import { BrowserViewModel, IBrowserViewWorkbenchService } from '../../common/browserView.js'; +import { IBrowserZoomService } from '../../common/browserZoomService.js'; + +suite('BrowserView native favicon synchronization', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + test('clears the synchronized favicon when a redirect chain returns to the original host', async () => { + const models = store.add(new DisposableStore()); + const native = createTestBrowserView(store); + const oldIcon = 'data:image/png;base64,b2xk'; + await native.setIcon(oldIcon); + const service = upcastPartial({ + getNavigationState: async () => native.view.getNavigationState(), + destroyBrowserView: async () => native.view.dispose(), + setBrowserZoomIndex: async (_id, index) => native.view.setBrowserZoomIndex(index), + onDynamicDidNavigate: () => native.view.onDidNavigate, + onDynamicDidChangeTitle: () => native.view.onDidChangeTitle, + onDynamicDidChangeFavicon: () => native.view.onDidChangeFavicon, + onDynamicDidChangeLoadingState: () => native.view.onDidChangeLoadingState, + onDynamicDidChangePermissions: () => native.view.onDidChangePermissions, + onDynamicDidChangeDevToolsState: () => native.view.onDidChangeDevToolsState, + onDynamicDidChangeOwner: () => native.view.onDidChangeOwner, + onDynamicDidChangeFocus: () => native.view.onDidChangeFocus, + onDynamicDidChangeVisibility: () => native.view.onDidChangeVisibility, + onDynamicDidChangeDeviceEmulation: () => native.view.emulator.onDidChange, + onDynamicDidChangeElementSelectionState: () => native.view.inspector.onDidChangeElementSelectionState, + onDynamicDidChangeAreaSelectionActive: () => native.view.inspector.onDidChangeAreaSelectionActive, + onDynamicDidChangeAudiences: () => native.view.onDidChangeAudiences, + onDynamicDidChangeRemoteStatus: () => native.view.onDidChangeRemoteStatus, + }); + const model = models.add(new BrowserViewModel( + native.view.id, native.view.host, native.view.owner, undefined, native.view.getState(), service, + upcastPartial({ isSharingAvailable: false, onDidChangeSharingAvailable: Event.None }), + NullTelemetryService, upcastPartial({}), upcastPartial({}), + upcastPartial({ getEffectiveZoomIndex: () => browserZoomDefaultIndex, onDidChangeZoom: Event.None }), + upcastPartial({ onDidChange: Event.None }), new NullLogService(), + )); + await native.settle(); + const before = model.favicon; + + native.navigate('https://first.example/redirect'); + native.redirect('https://second.example/intermediate'); + native.redirect('https://first.example/page'); + native.commit('https://first.example/page'); + const atCommit = model.favicon; + const iconUrl = 'https://first.example/missing.png'; + native.events.emit('page-favicon-updated', {}, [iconUrl]); + await native.completeFavicon(iconUrl, '', 404); + + assert.deepStrictEqual({ + before, atCommit, afterMissingIcon: model.favicon, + nativeIcon: native.view.getNavigationState().lastFavicon, + oldHistoryIcon: native.history[0].favicon, newHistoryIcon: native.history[1].favicon, + }, { + before: oldIcon, atCommit: undefined, afterMissingIcon: undefined, nativeIcon: undefined, + oldHistoryIcon: oldIcon, newHistoryIcon: undefined, + }); + }); +}); From fb6ca213fb63d38f6117542c245f07de175a7cd5 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 13 Sep 2026 00:00:10 -0700 Subject: [PATCH 6/7] browser: restore aborted navigation icons and finish native cleanup Preserve the committed favicon across provisional navigation and restore it when an uncommitted load stops, without reviving stale favicon requests. Complete registered cleanup when Electron has already cleared a closed view's WebContents. Add 13 regressions for abort ordering, model synchronization, and native/editor/window disposal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browserView/electron-main/browserView.ts | 32 ++++- .../electron-main/browserViewFavicon.test.ts | 114 ++++++++++++++++++ .../browserViewLifecycle.test.ts | 44 +++++++ .../electron-main/browserViewTestUtils.ts | 15 ++- .../browserViewFaviconSync.test.ts | 45 ++++++- 5 files changed, 234 insertions(+), 16 deletions(-) create mode 100644 src/vs/platform/browserView/test/electron-main/browserViewLifecycle.test.ts diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index 95756c2a923a74..c2a7849c56c5cc 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -337,8 +337,10 @@ export class BrowserView extends Disposable { } }; let pendingNavigationUrl: string | undefined; + let pendingNavigationFavicon: { favicon: string | undefined } | undefined; webContents.on('did-start-navigation', (_event, url, isInPlace, isMainFrame) => { if (isMainFrame && !isInPlace && !this._shouldRedirectPinnedNavigation(url)) { + pendingNavigationFavicon ??= { favicon: this._lastFavicon }; resetFaviconForNavigation(webContents.getURL(), url); faviconLoader.invalidate(); pendingNavigationUrl = url; @@ -396,7 +398,20 @@ export class BrowserView extends Disposable { fireLoadingEvent(true); } }); - webContents.on('did-stop-loading', () => fireLoadingEvent(false)); + webContents.on('did-stop-loading', () => { + // An uncommitted load leaves the previous document active, even after redirects or superseding loads. + if (pendingNavigationFavicon) { + faviconLoader.invalidate(); + const { favicon } = pendingNavigationFavicon; + pendingNavigationFavicon = undefined; + pendingNavigationUrl = undefined; + if (this._lastFavicon !== favicon) { + this._lastFavicon = favicon; + fireFaviconEvent(); + } + } + fireLoadingEvent(false); + }); webContents.on('did-fail-load', (e, errorCode, errorDescription, validatedURL, isMainFrame) => { if (isMainFrame) { // Ignore ERR_ABORTED (-3) which is the expected error when user stops a page load. @@ -405,6 +420,8 @@ export class BrowserView extends Disposable { return; } + pendingNavigationFavicon = undefined; + pendingNavigationUrl = undefined; this._lastError = { url: validatedURL, errorCode, @@ -451,7 +468,11 @@ export class BrowserView extends Disposable { }); // Navigation events (when URL actually changes) - webContents.on('did-navigate', (_, url) => fireNavigationEvent(url)); + webContents.on('did-navigate', (_, url) => { + pendingNavigationFavicon = undefined; + pendingNavigationUrl = undefined; + fireNavigationEvent(url); + }); webContents.on('did-navigate-in-page', (_, url, isMainFrame) => { // Ignore subframe (iframe) navigations: they must not rewrite the // main frame's URL bar or its history entry. @@ -1109,9 +1130,10 @@ export class BrowserView extends Disposable { // Fire close event BEFORE disposing emitters. This signals the view has been destroyed. this._onDidClose.fire(); - // Clean up the view and all its event listeners - if (!this._view.webContents.isDestroyed()) { - this._view.webContents.close({ waitForBeforeUnload: false }); + // Electron clears the view's webContents before emitting destroyed. + const webContents = this._view.webContents; + if (webContents && !webContents.isDestroyed()) { + webContents.close({ waitForBeforeUnload: false }); } super.dispose(); diff --git a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts index 1c7dcf1b07118b..fbec7fc067e4c2 100644 --- a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts +++ b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts @@ -124,4 +124,118 @@ suite('BrowserView favicon navigation', () => { }); }); } + + for (const redirect of [false, true]) { + test(`restores the committed favicon after an aborted cross-host ${redirect ? 'redirect' : 'navigation'}`, async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const favicons: (string | undefined)[] = []; + store.add(testCase.view.onDidChangeFavicon(event => favicons.push(event.favicon))); + const target = 'https://second.example/destination'; + testCase.navigate(redirect ? 'https://first.example/redirect' : target); + if (redirect) { + testCase.redirect(target); + } + const pending = testCase.view.getNavigationState(); + testCase.events.emit('did-fail-provisional-load', {}, -3, 'ERR_ABORTED', target, true); + testCase.events.emit('did-stop-loading'); + const stopped = testCase.view.getNavigationState(); + testCase.events.emit('did-navigate-in-page', {}, 'https://first.example/page#after-cancel', true); + + assert.deepStrictEqual({ + pendingIcon: pending.lastFavicon, stoppedIcon: stopped.lastFavicon, + newerVersion: stopped.navigationStateVersion > pending.navigationStateVersion, + afterSameDocumentNavigation: testCase.view.getNavigationState().lastFavicon, + originalHistoryIcon: testCase.history[0].favicon, favicons, + }, { + pendingIcon: undefined, stoppedIcon: oldIcon, newerVersion: true, + afterSameDocumentNavigation: oldIcon, originalHistoryIcon: oldIcon, favicons: [oldIcon], + }); + }); + } + + test('preserves the original favicon across multiple uncommitted navigations and an abort', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://second.example/first'); + testCase.navigate('https://third.example/second'); + testCase.events.emit('did-fail-load', {}, -3, 'ERR_ABORTED', 'https://second.example/first', true); + const whileLoading = testCase.view.getNavigationState().lastFavicon; + testCase.events.emit('did-navigate-in-page', {}, 'https://first.example/page#pending', true); + testCase.events.emit('did-stop-loading'); + + assert.deepStrictEqual({ whileLoading, stopped: testCase.view.getNavigationState().lastFavicon }, { + whileLoading: undefined, stopped: oldIcon, + }); + }); + + test('discards pending provisional favicon work after restoring the committed icon', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://second.example/destination'); + const iconUrl = 'https://second.example/pending.png'; + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + testCase.events.emit('did-stop-loading'); + await testCase.completeFavicon(iconUrl, 'stale'); + + assert.deepStrictEqual({ icon: testCase.view.getNavigationState().lastFavicon, historyIcon: testCase.history[0].favicon }, { + icon: oldIcon, historyIcon: oldIcon, + }); + }); + + test('does not restore the previous favicon after a successful commit', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://second.example/destination'); + testCase.commit('https://second.example/destination'); + testCase.events.emit('did-stop-loading'); + const iconless = testCase.view.getNavigationState().lastFavicon; + const newIcon = 'data:image/png;base64,bmV3'; + await testCase.setIcon(newIcon); + testCase.events.emit('did-stop-loading'); + testCase.navigate('https://third.example/destination'); + testCase.events.emit('did-stop-loading'); + + assert.deepStrictEqual({ iconless, afterLaterAbort: testCase.view.getNavigationState().lastFavicon }, { + iconless: undefined, afterLaterAbort: newIcon, + }); + }); + + test('does not restore the previous favicon after a non-aborted main-frame failure', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const target = 'https://second.example/destination'; + testCase.navigate(target); + testCase.events.emit('did-fail-load', {}, -105, 'ERR_NAME_NOT_RESOLVED', target, true); + testCase.events.emit('did-stop-loading'); + const state = testCase.view.getNavigationState(); + + assert.deepStrictEqual({ favicon: state.lastFavicon, error: state.lastError?.errorCode, historyIcon: testCase.history[0].favicon }, { + favicon: undefined, error: -105, historyIcon: oldIcon, + }); + }); + + test('subframe failures do not discard the main-frame favicon needed on abort', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://second.example/destination'); + testCase.events.emit('did-fail-load', {}, -105, 'ERR_NAME_NOT_RESOLVED', 'https://frame.example/', false); + testCase.events.emit('did-stop-loading'); + + assert.strictEqual(testCase.view.getNavigationState().lastFavicon, oldIcon); + }); + + test('an unchanged favicon does not produce another notification after a same-host abort', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const favicons: (string | undefined)[] = []; + store.add(testCase.view.onDidChangeFavicon(event => favicons.push(event.favicon))); + testCase.navigate('https://first.example/next'); + testCase.events.emit('did-stop-loading'); + testCase.events.emit('did-stop-loading'); + + assert.deepStrictEqual({ favicon: testCase.view.getNavigationState().lastFavicon, favicons }, { + favicon: oldIcon, favicons: [], + }); + }); }); diff --git a/src/vs/platform/browserView/test/electron-main/browserViewLifecycle.test.ts b/src/vs/platform/browserView/test/electron-main/browserViewLifecycle.test.ts new file mode 100644 index 00000000000000..1fb2a9ffa5b144 --- /dev/null +++ b/src/vs/platform/browserView/test/electron-main/browserViewLifecycle.test.ts @@ -0,0 +1,44 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { createTestBrowserView } from './browserViewTestUtils.js'; + +suite('BrowserView native lifecycle', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + + for (const source of ['native', 'editor', 'window'] as const) { + test(`disposes listeners and pending favicon work after ${source} closure`, async () => { + const testCase = createTestBrowserView(store); + let closed = 0; + store.add(testCase.view.onDidClose(() => closed++)); + const iconUrl = 'https://first.example/pending.png'; + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + const subscribed = testCase.windowClosed.hasListeners() && testCase.permissionsChanged.hasListeners(); + + if (source === 'native') { + testCase.webContents.close(); + } else if (source === 'editor') { + testCase.view.dispose(); + } else { + testCase.windowClosed.fire(); + } + testCase.view.dispose(); + await testCase.completeFavicon(iconUrl, 'late-icon'); + + assert.deepStrictEqual({ + subscribed, closed, closeCalls: testCase.closeCalls, + contents: testCase.view.getWebContentsView().webContents, + windowListeners: testCase.windowClosed.hasListeners(), + permissionListeners: testCase.permissionsChanged.hasListeners(), + historyIcon: testCase.history[0].favicon, + }, { + subscribed: true, closed: 1, closeCalls: 1, contents: undefined, + windowListeners: false, permissionListeners: false, historyIcon: undefined, + }); + }); + } +}); diff --git a/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts index f66c2cada3103a..92088c5867d106 100644 --- a/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts +++ b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts @@ -6,7 +6,7 @@ import assert from 'assert'; import { EventEmitter } from 'events'; import { DeferredPromise } from '../../../../base/common/async.js'; -import { Event } from '../../../../base/common/event.js'; +import { Emitter, Event } from '../../../../base/common/event.js'; import { DisposableStore } from '../../../../base/common/lifecycle.js'; import { URI } from '../../../../base/common/uri.js'; import { upcastDeepPartial, upcastPartial } from '../../../../base/test/common/mock.js'; @@ -26,9 +26,12 @@ export function createTestBrowserView(store: Pick, assoc const requests = new Map>(); const history: { url: string; favicon: string | null | undefined }[] = []; const programmaticCalls: string[] = []; + const windowClosed = store.add(new Emitter()); + const permissionsChanged = store.add(new Emitter()); let url = associatedResource?.toString() ?? 'https://first.example/page'; let historyTarget = url; let destroyed = false; + let closeCalls = 0; let childCreates = 0; const startNavigation = (target: string, isMainFrame = true, isSameDocument = false) => { events.emit('did-start-navigation', { url: target, isMainFrame, isSameDocument }, target, isSameDocument, isMainFrame); @@ -57,7 +60,7 @@ export function createTestBrowserView(store: Pick, assoc setZoomFactor: () => { }, setVisualZoomLevelLimits: async () => { }, loadURL: async target => { programmaticCalls.push('loadURL'); startNavigation(target); }, - close: () => { destroyed = true; events.emit('destroyed'); }, + close: () => { closeCalls++; destroyed = true; events.emit('destroyed'); }, navigationHistory: upcastPartial({ canGoBack: () => true, canGoForward: () => true, getActiveIndex: () => history.length, goBack: () => { programmaticCalls.push('back'); startNavigation(historyTarget); }, @@ -71,14 +74,15 @@ export function createTestBrowserView(store: Pick, assoc }), }); const nativeView = upcastPartial({ - webContents, setBounds: () => { }, setVisible: () => { }, getVisible: () => false, setBackgroundColor: () => { }, + get webContents() { return destroyed ? undefined : webContents; }, + setBounds: () => { }, setVisible: () => { }, getVisible: () => false, setBackgroundColor: () => { }, }); const session = upcastDeepPartial({ electronSession, storageScope: BrowserViewStorageScope.Ephemeral, remote: { onDidStart: Event.None, onDidStop: Event.None, isRemote: false, whenReady: Promise.resolve() }, permissions: { - onDidRequestPermission: Event.None, onDidRequestDevice: Event.None, onDidChange: Event.None, + onDidRequestPermission: Event.None, onDidRequestDevice: Event.None, onDidChange: permissionsChanged.event, storageKeys: {}, serialize: () => ({ origins: {} }), }, trust: { installCertErrorHandler: () => { }, getCertificateError: () => undefined }, @@ -92,7 +96,7 @@ export function createTestBrowserView(store: Pick, assoc }, }); const owner = upcastPartial({ - onDidClose: Event.None, onWillLoad: Event.None, + onDidClose: windowClosed.event, onWillLoad: Event.None, win: upcastDeepPartial({ contentView: { addChildView: () => { } } }), }); const view: BrowserView = store.add(new BrowserView( @@ -149,5 +153,6 @@ export function createTestBrowserView(store: Pick, assoc return { view, history, events, navigate, startNavigation, navigateProgrammatically, programmaticCalls, redirect, commit, setIcon, completeFavicon, settle, get childCreates() { return childCreates; }, + webContents, windowClosed, permissionsChanged, get closeCalls() { return closeCalls; }, }; } diff --git a/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts index 230dff048915f5..63beb5b1762f75 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts @@ -21,11 +21,7 @@ import { IBrowserZoomService } from '../../common/browserZoomService.js'; suite('BrowserView native favicon synchronization', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); - test('clears the synchronized favicon when a redirect chain returns to the original host', async () => { - const models = store.add(new DisposableStore()); - const native = createTestBrowserView(store); - const oldIcon = 'data:image/png;base64,b2xk'; - await native.setIcon(oldIcon); + function createModel(native: ReturnType, models: DisposableStore): BrowserViewModel { const service = upcastPartial({ getNavigationState: async () => native.view.getNavigationState(), destroyBrowserView: async () => native.view.dispose(), @@ -45,13 +41,21 @@ suite('BrowserView native favicon synchronization', () => { onDynamicDidChangeAudiences: () => native.view.onDidChangeAudiences, onDynamicDidChangeRemoteStatus: () => native.view.onDidChangeRemoteStatus, }); - const model = models.add(new BrowserViewModel( + return models.add(new BrowserViewModel( native.view.id, native.view.host, native.view.owner, undefined, native.view.getState(), service, upcastPartial({ isSharingAvailable: false, onDidChangeSharingAvailable: Event.None }), NullTelemetryService, upcastPartial({}), upcastPartial({}), upcastPartial({ getEffectiveZoomIndex: () => browserZoomDefaultIndex, onDidChangeZoom: Event.None }), upcastPartial({ onDidChange: Event.None }), new NullLogService(), )); + } + + test('clears the synchronized favicon when a redirect chain returns to the original host', async () => { + const models = store.add(new DisposableStore()); + const native = createTestBrowserView(store); + const oldIcon = 'data:image/png;base64,b2xk'; + await native.setIcon(oldIcon); + const model = createModel(native, models); await native.settle(); const before = model.favicon; @@ -73,4 +77,33 @@ suite('BrowserView native favicon synchronization', () => { oldHistoryIcon: oldIcon, newHistoryIcon: undefined, }); }); + + for (const attachDuringNavigation of [false, true]) { + test(`restores the favicon after aborting navigation with a model attached ${attachDuringNavigation ? 'during' : 'before'} the load`, async () => { + const models = store.add(new DisposableStore()); + const native = createTestBrowserView(store); + const oldIcon = 'data:image/png;base64,b2xk'; + await native.setIcon(oldIcon); + if (attachDuringNavigation) { + native.navigate('https://second.example/destination'); + } + const model = createModel(native, models); + await native.settle(); + if (!attachDuringNavigation) { + native.navigate('https://second.example/destination'); + } + const beforeAbort = model.favicon; + native.events.emit('did-stop-loading'); + native.events.emit('did-navigate-in-page', {}, 'https://first.example/page#after-cancel', true); + + assert.deepStrictEqual({ + beforeAbort, favicon: model.favicon, nativeIcon: native.view.getNavigationState().lastFavicon, + url: model.url, oldHistoryIcon: native.history[0].favicon, + }, { + beforeAbort: attachDuringNavigation ? undefined : oldIcon, + favicon: oldIcon, nativeIcon: oldIcon, + url: 'https://first.example/page#after-cancel', oldHistoryIcon: oldIcon, + }); + }); + } }); From 7466a6fa8e1d143d064eb0fb8337e1974b700628 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 13 Sep 2026 01:21:27 -0700 Subject: [PATCH 7/7] browser: isolate provisional favicon state from history Defer favicon history updates until navigation commits, including replacement entries and same-document updates during a pending load. Invalidate pending favicon requests and clear provisional icons on non-aborted main-frame failures. Add 14 regressions for commit, abort, history replacement, subframes, and failure/completion ordering. Addresses the additional favicon review feedback on #335987. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../browserView/electron-main/browserView.ts | 19 ++- .../electron-main/browserViewFavicon.test.ts | 127 ++++++++++++++++++ .../electron-main/browserViewTestUtils.ts | 14 +- .../browserViewFaviconSync.test.ts | 31 +++++ 4 files changed, 182 insertions(+), 9 deletions(-) diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index c2a7849c56c5cc..675c2e3a4b8002 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -287,6 +287,7 @@ export class BrowserView extends Disposable { // Favicon events let lastEmittedFavicon: string | undefined; + let pendingNavigationFavicon: { favicon: string | undefined } | undefined; const fireFaviconEvent = () => { lastEmittedFavicon = this._lastFavicon; this._onDidChangeFavicon.fire({ navigationStateVersion: ++this._navigationStateVersion, favicon: this._lastFavicon }); @@ -322,7 +323,9 @@ export class BrowserView extends Disposable { } this._lastFavicon = favicon; fireFaviconEvent(); - this._currentHistoryHandle?.update({ favicon: favicon ?? null }); + if (!pendingNavigationFavicon) { + this._currentHistoryHandle?.update({ favicon: favicon ?? null }); + } }, this.logService, )); @@ -337,7 +340,6 @@ export class BrowserView extends Disposable { } }; let pendingNavigationUrl: string | undefined; - let pendingNavigationFavicon: { favicon: string | undefined } | undefined; webContents.on('did-start-navigation', (_event, url, isInPlace, isMainFrame) => { if (isMainFrame && !isInPlace && !this._shouldRedirectPinnedNavigation(url)) { pendingNavigationFavicon ??= { favicon: this._lastFavicon }; @@ -379,7 +381,7 @@ export class BrowserView extends Disposable { canGoForward: webContents.navigationHistory.canGoForward(), certificateError: this.session.trust.getCertificateError(url) }); - this._recordNavigation(url); + this._recordNavigation(url, pendingNavigationFavicon ? pendingNavigationFavicon.favicon : this._lastFavicon); if (lastEmittedFavicon !== this._lastFavicon) { fireFaviconEvent(); } @@ -420,6 +422,11 @@ export class BrowserView extends Disposable { return; } + faviconLoader.invalidate(); + if (this._lastFavicon !== undefined) { + this._lastFavicon = undefined; + fireFaviconEvent(); + } pendingNavigationFavicon = undefined; pendingNavigationUrl = undefined; this._lastError = { @@ -611,7 +618,7 @@ export class BrowserView extends Disposable { /** * Record a committed navigation in the session's history. */ - private _recordNavigation(url: string): void { + private _recordNavigation(url: string, favicon: string | undefined): void { const webContents = this._view.webContents; const activeIndex = webContents.navigationHistory.getActiveIndex(); @@ -626,7 +633,7 @@ export class BrowserView extends Disposable { // a duplicate. const handle = this._currentHistoryHandle; if (handle && activeIndex === this._lastCommittedEntryIndex) { - handle.update({ url, title: webContents.getTitle() }); + handle.update({ url, title: webContents.getTitle(), favicon: favicon ?? null }); return; } this._lastCommittedEntryIndex = activeIndex; @@ -636,7 +643,7 @@ export class BrowserView extends Disposable { this._currentHistoryHandle = this.session.history.add( url, webContents.getTitle(), - this._lastFavicon, + favicon, userInitiated, ); } diff --git a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts index fbec7fc067e4c2..d43317b2d54173 100644 --- a/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts +++ b/src/vs/platform/browserView/test/electron-main/browserViewFavicon.test.ts @@ -238,4 +238,131 @@ suite('BrowserView favicon navigation', () => { favicon: oldIcon, favicons: [], }); }); + + for (const success of [true, false]) { + for (const commit of [true, false]) { + test(`defers a provisional favicon ${success ? 'update' : 'clear'} until navigation ${commit ? 'commits' : 'aborts'}`, async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const target = 'https://first.example/next'; + const iconUrl = 'https://first.example/provisional.png'; + const provisionalIcon = success ? 'data:image/png;base64,bmV3' : undefined; + testCase.navigate(target); + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + await testCase.completeFavicon(iconUrl, 'new', success ? 200 : 404); + const duringNavigation = { + favicon: testCase.view.getNavigationState().lastFavicon, + history: testCase.history.map(entry => ({ ...entry })), + }; + if (commit) { + testCase.commit(target); + } + testCase.events.emit('did-stop-loading'); + + assert.deepStrictEqual({ + duringNavigation, + favicon: testCase.view.getNavigationState().lastFavicon, + history: testCase.history, + }, { + duringNavigation: { + favicon: provisionalIcon, + history: [{ url: 'https://first.example/page', favicon: oldIcon }], + }, + favicon: commit ? provisionalIcon : oldIcon, + history: [ + { url: 'https://first.example/page', favicon: oldIcon }, + ...(commit ? [{ url: target, favicon: provisionalIcon }] : []), + ], + }); + }); + } + + test(`records a provisional favicon ${success ? 'update' : 'clear'} when the document replaces its history entry`, async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const target = 'https://first.example/replacement'; + const iconUrl = 'https://first.example/provisional.png'; + testCase.navigate(target); + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + await testCase.completeFavicon(iconUrl, 'new', success ? 200 : 404); + const beforeCommit = testCase.history[0].favicon; + testCase.commit(target, { replace: true }); + + assert.deepStrictEqual({ + beforeCommit, + favicon: testCase.view.getNavigationState().lastFavicon, + history: testCase.history.map(({ url, favicon }) => ({ url, favicon })), + }, { + beforeCommit: oldIcon, + favicon: success ? 'data:image/png;base64,bmV3' : undefined, + history: [{ url: target, favicon: success ? 'data:image/png;base64,bmV3' : null }], + }); + }); + } + + test('same-document history updates during a provisional load keep the committed favicon', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + testCase.navigate('https://second.example/destination'); + await testCase.setIcon('data:image/png;base64,bmV3'); + testCase.commit('https://first.example/page#pending', { sameDocument: true, replace: true }); + const duringNavigation = testCase.history[0].favicon; + testCase.events.emit('did-stop-loading'); + + assert.deepStrictEqual({ + duringNavigation, favicon: testCase.view.getNavigationState().lastFavicon, + history: testCase.history.map(({ url, favicon }) => ({ url, favicon })), + }, { + duringNavigation: oldIcon, favicon: oldIcon, + history: [{ url: 'https://first.example/page#pending', favicon: oldIcon }], + }); + }); + + for (const sameHost of [true, false]) { + for (const completeBeforeFailure of [true, false]) { + test(`rejects a ${sameHost ? 'same-host' : 'cross-host'} favicon completed ${completeBeforeFailure ? 'before' : 'after'} a failed navigation`, async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const target = `https://${sameHost ? 'first' : 'second'}.example/destination`; + const iconUrl = 'https://first.example/provisional.png'; + const favicons: (string | undefined)[] = []; + store.add(testCase.view.onDidChangeFavicon(event => favicons.push(event.favicon))); + testCase.navigate(target); + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + if (completeBeforeFailure) { + await testCase.completeFavicon(iconUrl, 'new'); + } + testCase.events.emit('did-fail-load', {}, -105, 'ERR_NAME_NOT_RESOLVED', target, true); + testCase.events.emit('did-stop-loading'); + if (!completeBeforeFailure) { + await testCase.completeFavicon(iconUrl, 'new'); + } + const state = testCase.view.getNavigationState(); + + assert.deepStrictEqual({ + favicon: state.lastFavicon, error: state.lastError?.errorCode, + historyIcon: testCase.history[0].favicon, favicons, + }, { + favicon: undefined, error: -105, historyIcon: oldIcon, + favicons: completeBeforeFailure ? ['data:image/png;base64,bmV3', undefined] : sameHost ? [undefined] : [], + }); + }); + } + } + + test('subframe failures leave current favicon work active', async () => { + const testCase = createView(); + await testCase.setIcon(oldIcon); + const iconUrl = 'https://first.example/current.png'; + testCase.events.emit('page-favicon-updated', {}, [iconUrl]); + testCase.events.emit('did-fail-load', {}, -105, 'ERR_NAME_NOT_RESOLVED', 'https://frame.example/', false); + await testCase.completeFavicon(iconUrl, 'new'); + + assert.deepStrictEqual({ + favicon: testCase.view.getNavigationState().lastFavicon, + historyIcon: testCase.history[0].favicon, + }, { + favicon: 'data:image/png;base64,bmV3', historyIcon: 'data:image/png;base64,bmV3', + }); + }); }); diff --git a/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts index 92088c5867d106..c7d38d524f6361 100644 --- a/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts +++ b/src/vs/platform/browserView/test/electron-main/browserViewTestUtils.ts @@ -30,6 +30,7 @@ export function createTestBrowserView(store: Pick, assoc const permissionsChanged = store.add(new Emitter()); let url = associatedResource?.toString() ?? 'https://first.example/page'; let historyTarget = url; + let activeHistoryIndex = -1; let destroyed = false; let closeCalls = 0; let childCreates = 0; @@ -62,7 +63,7 @@ export function createTestBrowserView(store: Pick, assoc loadURL: async target => { programmaticCalls.push('loadURL'); startNavigation(target); }, close: () => { closeCalls++; destroyed = true; events.emit('destroyed'); }, navigationHistory: upcastPartial({ - canGoBack: () => true, canGoForward: () => true, getActiveIndex: () => history.length, + canGoBack: () => true, canGoForward: () => true, getActiveIndex: () => activeHistoryIndex, goBack: () => { programmaticCalls.push('back'); startNavigation(historyTarget); }, goForward: () => { programmaticCalls.push('forward'); startNavigation(historyTarget); }, }), @@ -106,9 +107,16 @@ export function createTestBrowserView(store: Pick, assoc upcastPartial({}), new NullLogService(), NullTelemetryService, )); const settle = () => new Promise(resolve => nextMacrotask(realTimeApi, resolve)); - const commit = (target: string) => { + const commit = (target: string, options?: { replace?: boolean; sameDocument?: boolean }) => { url = target; - events.emit('did-navigate', {}, url); + if (!options?.replace) { + activeHistoryIndex++; + } + if (options?.sameDocument) { + events.emit('did-navigate-in-page', {}, url, true); + } else { + events.emit('did-navigate', {}, url); + } }; commit(url); const navigate = (target: string) => { diff --git a/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts index 63beb5b1762f75..a5903f9926902e 100644 --- a/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts +++ b/src/vs/workbench/contrib/browserView/test/electron-main/browserViewFaviconSync.test.ts @@ -106,4 +106,35 @@ suite('BrowserView native favicon synchronization', () => { }); }); } + + for (const completeBeforeFailure of [true, false]) { + test(`clears the model favicon after a failed load with icon completion ${completeBeforeFailure ? 'before' : 'after'} the failure`, async () => { + const models = store.add(new DisposableStore()); + const native = createTestBrowserView(store); + const oldIcon = 'data:image/png;base64,b2xk'; + await native.setIcon(oldIcon); + const model = createModel(native, models); + await native.settle(); + const target = 'https://first.example/failure'; + const iconUrl = 'https://first.example/provisional.png'; + native.navigate(target); + native.events.emit('page-favicon-updated', {}, [iconUrl]); + if (completeBeforeFailure) { + await native.completeFavicon(iconUrl, 'new'); + } + native.events.emit('did-fail-load', {}, -105, 'ERR_NAME_NOT_RESOLVED', target, true); + native.events.emit('did-stop-loading'); + if (!completeBeforeFailure) { + await native.completeFavicon(iconUrl, 'new'); + } + + assert.deepStrictEqual({ + favicon: model.favicon, nativeIcon: native.view.getNavigationState().lastFavicon, + url: model.url, error: model.error?.errorCode, historyIcon: native.history[0].favicon, + }, { + favicon: undefined, nativeIcon: undefined, + url: target, error: -105, historyIcon: oldIcon, + }); + }); + } });