From a82e62c56558cae561b308e4b6078a9cfbcf84a8 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 13 Sep 2026 15:52:34 -0700 Subject: [PATCH 1/3] browser: isolate favicon state by document Keep the committed favicon separate from pending navigation candidates. Promote candidates at commit, discard them on abort or failure, and reject superseded asynchronous requests without rewriting prior history. Preserve same-document and replacement history, and cancel favicon work on native destruction. Cover the state transitions with focused unit tests and a browser API/CDP navigation scenario. Separates favicon correctness from #335987 without introducing its snapshot protocol or the native-close fix in #336075. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../singlefolder-tests/browser.cdp.test.ts | 71 ++++- .../browserView/common/browserFavicon.ts | 110 ++++++++ .../browserView/electron-main/browserView.ts | 115 +++++---- .../test/common/browserFavicon.test.ts | 242 ++++++++++++++++++ 4 files changed, 483 insertions(+), 55 deletions(-) create mode 100644 src/vs/platform/browserView/common/browserFavicon.ts create mode 100644 src/vs/platform/browserView/test/common/browserFavicon.test.ts diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts index cf97fb0da26a12..fef8b815ed4e2b 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts @@ -4,10 +4,11 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; +import * as http from 'http'; import * as path from 'path'; import * as vscode from 'vscode'; import { window, workspace } from 'vscode'; -import { assertNoRpc, closeAllEditors } from '../utils'; +import { assertNoRpc, closeAllEditors, poll } from '../utils'; /** * We only care about target-lifecycle and browser-level events. @@ -179,6 +180,74 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; // #endregion + (vscode.env.remoteName ? test.skip : test)('favicons follow native document navigation and redirect history', async function () { + this.timeout(30_000); + const red = 'data:image/svg+xml;base64,' + Buffer.from('').toString('base64'); + const blue = 'data:image/svg+xml;base64,' + Buffer.from('').toString('base64'); + const server = http.createServer((request, response) => { + const url = new URL(request.url ?? '/', 'http://localhost'); + if (url.pathname === '/redirect') { + response.writeHead(302, { Location: url.searchParams.get('to')! }); + response.end(); + } else if (url.pathname === '/favicon.ico') { + response.writeHead(404); + response.end(); + } else { + const icon = url.pathname === '/first' ? red : url.pathname === '/second' ? blue : undefined; + response.writeHead(200, { 'Content-Type': 'text/html', 'Cache-Control': 'no-store' }); + response.end(`${url.pathname}${icon ? `` : ''}${url.pathname}`); + } + }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + server.off('error', reject); + resolve(); + }); + }); + try { + const address = server.address(); + assert.ok(address && typeof address !== 'string'); + const firstUrl = `http://127.0.0.1:${address.port}/first`; + const secondUrl = `http://localhost:${address.port}/second`; + const tab = await window.openBrowserTab(firstUrl); + const session = await tab.startCDPSession(); + try { + const { cdpSend } = createHarness(session); + const browser: { sessionId: string } = await cdpSend('Target.attachToBrowserTarget'); + const targets: { targetInfos: { targetId: string; type: string; url: string }[] } = await cdpSend('Target.getTargets', {}, browser.sessionId); + const target = targets.targetInfos.find(target => target.type === 'page' && target.url === firstUrl); + assert.ok(target); + const page: { sessionId: string } = await cdpSend('Target.attachToTarget', { targetId: target.targetId, flatten: true }, browser.sessionId); + const waitForIcon = (url: string, icon: string) => poll( + async () => ({ url: tab.url, icon: tab.icon instanceof vscode.Uri ? tab.icon.toString(true) : tab.icon instanceof vscode.ThemeIcon ? tab.icon.id : undefined }), + state => state.url === url && state.icon === icon, + `Browser favicon for ${url} should be ${icon}`, + ); + await waitForIcon(firstUrl, red); + await cdpSend('Page.navigate', { url: secondUrl }, page.sessionId); + await waitForIcon(secondUrl, blue); + const history: { currentIndex: number; entries: { id: number }[] } = await cdpSend('Page.getNavigationHistory', {}, page.sessionId); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, page.sessionId); + await waitForIcon(firstUrl, red); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex].id }, page.sessionId); + await waitForIcon(secondUrl, blue); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, page.sessionId); + await waitForIcon(firstUrl, red); + + const finalUrl = `http://127.0.0.1:${address.port}/iconless`; + const intermediate = `http://localhost:${address.port}/redirect?to=${encodeURIComponent(finalUrl)}`; + await cdpSend('Page.navigate', { url: `http://127.0.0.1:${address.port}/redirect?to=${encodeURIComponent(intermediate)}` }, page.sessionId); + await waitForIcon(finalUrl, 'globe'); + } finally { + await session.close(); + } + } finally { + server.closeAllConnections(); + await new Promise((resolve, reject) => server.close(error => error ? reject(error) : resolve())); + } + }); + // Loads `file:////index.html`. Skipped in remote // workspaces: the workspace folder is a `vscode-remote://` URI so it // isn't added to the local `file://` trust allowlist, and the harness diff --git a/src/vs/platform/browserView/common/browserFavicon.ts b/src/vs/platform/browserView/common/browserFavicon.ts new file mode 100644 index 00000000000000..d189ca21577568 --- /dev/null +++ b/src/vs/platform/browserView/common/browserFavicon.ts @@ -0,0 +1,110 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { Emitter } from '../../../base/common/event.js'; +import { Disposable } from '../../../base/common/lifecycle.js'; +import { ILogService } from '../../log/common/log.js'; + +interface FaviconDocument { + url: string; + favicon: string | undefined; +} + +/** Keeps provisional navigation icons separate from the committed document. */ +export class BrowserFavicon extends Disposable { + private _document: FaviconDocument | undefined; + private _navigation: FaviconDocument | undefined; + private _requestId = 0; + + private readonly _onDidLoad = this._register(new Emitter()); + /** Fires when a fetch updates the committed document; navigation changes are published by the view. */ + readonly onDidLoad = this._onDidLoad.event; + + constructor( + url: string, + private readonly fetchFavicon: (url: string) => Promise, + @ILogService private readonly logService: ILogService, + ) { + super(); + this._document = { url, favicon: undefined }; + } + + get favicon(): string | undefined { + return this._document?.favicon; + } + + beginNavigation(url: string): void { + this._requestId++; + this._navigation = { + url, + favicon: URL.parse(url)?.host === URL.parse(this._document?.url ?? '')?.host ? this.favicon : undefined, + }; + } + + redirectNavigation(url: string): void { + if (!this._navigation) { + this.beginNavigation(url); + return; + } + if (URL.parse(url)?.host !== URL.parse(this._navigation.url)?.host) { + this._requestId++; + this._navigation.favicon = undefined; + } + this._navigation.url = url; + } + + /** Promotes the candidate before the view records history and publishes its navigation events. */ + commitNavigation(url: string, sameDocument = false): void { + if (sameDocument) { + if (this._document) { + this._document.url = url; + } + return; + } + this.redirectNavigation(url); + this._document = this._navigation; + this._navigation = undefined; + } + + abortNavigation(): void { + if (this._navigation) { + this._requestId++; + this._navigation = undefined; + } + } + + failNavigation(): void { + this._requestId++; + this._navigation = undefined; + this._document = undefined; + } + + async load(urls: readonly string[]): Promise { + const document = this._navigation ?? this._document; + if (this._store.isDisposed || !document) { + return; + } + const requestId = ++this._requestId; + let favicon: string | undefined; + for (const url of urls) { + try { + favicon = await this.fetchFavicon(url); + } catch (error) { + this.logService.trace('[BrowserFavicon] Failed to fetch favicon, trying the next candidate.', error); + } + if (this._store.isDisposed || requestId !== this._requestId) { + return; + } + if (favicon !== undefined) { + break; + } + } + const changed = document.favicon !== favicon; + document.favicon = favicon; + if (document === this._document && changed) { + this._onDidLoad.fire(favicon); + } + } +} diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index 6831bae3194932..e6642ac21dd66e 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -23,6 +23,7 @@ import { SCAN_CODE_STR_TO_EVENT_KEY_CODE } from '../../../base/common/keyCodes.j import { ITelemetryService } from '../../telemetry/common/telemetry.js'; import { logBrowserOpen } from '../common/browserViewTelemetry.js'; import { URI } from '../../../base/common/uri.js'; +import { BrowserFavicon } from '../common/browserFavicon.js'; enum NewPageLocation { Foreground = 'foreground', @@ -37,9 +38,9 @@ enum NewPageLocation { export class BrowserView extends Disposable { private readonly _view: WebContentsView; private readonly _faviconRequestCache = new Map>(); + private readonly _favicon: BrowserFavicon; private _lastScreenshot: VSBuffer | undefined = undefined; - private _lastFavicon: string | undefined = undefined; private _lastError: IBrowserViewLoadError | undefined = undefined; private _lastUserGestureTimestamp: number = -Infinity; private _browserZoomIndex: number = browserZoomDefaultIndex; @@ -160,6 +161,7 @@ export class BrowserView extends Disposable { // Passing an `undefined` webContents triggers an error in Electron. ...(options?.webContents ? { webContents: options.webContents } : {}) }); + this._favicon = this._register(new BrowserFavicon(this._view.webContents.getURL(), url => this._fetchFavicon(url), this.logService)); // Use a default size of 1024x768. // Important: The bounds here must be on-screen, otherwise some OSes (like macOS) may not actually start rendering. @@ -232,6 +234,7 @@ export class BrowserView extends Disposable { }); this._view.webContents.on('destroyed', () => { + this._favicon.dispose(); this.dispose(); }); @@ -283,46 +286,16 @@ 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) { - if (!this._faviconRequestCache.has(url)) { - this._faviconRequestCache.set(url, (async () => { - if (url.startsWith('data:image/')) { - return url; - } - const response = await webContents.session.fetch(url, { - cache: 'force-cache' - }); - if (!response.ok) { - throw new Error(`Failed to fetch favicon: ${response.status} ${response.statusText}`); - } - const type = await response.headers.get('content-type'); - if (!type?.startsWith('image/')) { - throw new Error(`Favicon is not an image: ${type}`); - } - const buffer = await response.arrayBuffer(); - - return `data:${type};base64,${Buffer.from(buffer).toString('base64')}`; - })()); - } - - try { - this._lastFavicon = await this._faviconRequestCache.get(url)!; - this._onDidChangeFavicon.fire({ favicon: this._lastFavicon }); - this._currentHistoryHandle?.update({ favicon: this._lastFavicon }); - // On success, stop searching - 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._onDidChangeFavicon.fire({ favicon: this._lastFavicon }); - this._currentHistoryHandle?.update({ favicon: null }); + this._register(this._favicon.onDidLoad(favicon => { + this._currentHistoryHandle?.update({ favicon: favicon ?? null }); + this._onDidChangeFavicon.fire({ favicon }); + })); + webContents.on('page-favicon-updated', (_event, favicons) => { + void this._favicon.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 && !this._shouldRedirectPinnedNavigation(url)) { + this._favicon.beginNavigation(url); } }); webContents.on('will-navigate', (event) => { @@ -330,16 +303,14 @@ 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; - } }); webContents.on('will-redirect', event => { if (this._redirectPinnedNavigation(event.url)) { event.preventDefault(); + return; + } + if (event.isMainFrame && !event.isSameDocument) { + this._favicon.redirectNavigation(event.url); } }); @@ -349,7 +320,8 @@ export class BrowserView extends Disposable { this._currentHistoryHandle?.update({ title }); }); - const fireNavigationEvent = (url: string) => { + const fireNavigationEvent = (url: string, sameDocument = false) => { + this._favicon.commitNavigation(url, sameDocument); this._onDidNavigate.fire({ url, title: webContents.getTitle(), @@ -358,6 +330,9 @@ export class BrowserView extends Disposable { certificateError: this.session.trust.getCertificateError(url) }); this._recordNavigation(url); + if (!sameDocument) { + this._onDidChangeFavicon.fire({ favicon: this._favicon.favicon }); + } }; const fireLoadingEvent = (loading: boolean) => { @@ -373,7 +348,10 @@ export class BrowserView extends Disposable { fireLoadingEvent(true); } }); - webContents.on('did-stop-loading', () => fireLoadingEvent(false)); + webContents.on('did-stop-loading', () => { + this._favicon.abortNavigation(); + 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. @@ -382,6 +360,7 @@ export class BrowserView extends Disposable { return; } + this._favicon.failNavigation(); this._lastError = { url: validatedURL, errorCode, @@ -398,6 +377,7 @@ export class BrowserView extends Disposable { canGoForward: webContents.navigationHistory.canGoForward(), certificateError: this.session.trust.getCertificateError(validatedURL) }); + this._onDidChangeFavicon.fire({ favicon: this._favicon.favicon }); } }); webContents.on('did-finish-load', () => fireLoadingEvent(false)); @@ -432,7 +412,7 @@ export class BrowserView extends Disposable { // Ignore subframe (iframe) navigations: they must not rewrite the // main frame's URL bar or its history entry. if (isMainFrame) { - fireNavigationEvent(url); + fireNavigationEvent(url, true); } }); @@ -547,6 +527,29 @@ export class BrowserView extends Disposable { }); } + private _fetchFavicon(url: string): Promise { + let request = this._faviconRequestCache.get(url); + if (!request) { + request = (async () => { + if (url.startsWith('data:image/')) { + return url; + } + const response = await this._view.webContents.session.fetch(url, { cache: 'force-cache' }); + if (!response.ok) { + throw new Error(`Failed to fetch favicon: ${response.status} ${response.statusText}`); + } + const type = response.headers.get('content-type'); + if (!type?.startsWith('image/')) { + throw new Error(`Favicon is not an image: ${type}`); + } + const buffer = await response.arrayBuffer(); + return `data:${type};base64,${Buffer.from(buffer).toString('base64')}`; + })(); + this._faviconRequestCache.set(url, request); + } + return request; + } + private consumePopupPermission(location: NewPageLocation): boolean { switch (location) { case NewPageLocation.Foreground: @@ -581,7 +584,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: this._favicon.favicon ?? null }); return; } this._lastCommittedEntryIndex = activeIndex; @@ -591,7 +594,7 @@ export class BrowserView extends Disposable { this._currentHistoryHandle = this.session.history.add( url, webContents.getTitle(), - this._lastFavicon, + this._favicon.favicon, userInitiated, ); } @@ -626,7 +629,7 @@ export class BrowserView extends Disposable { visible: this._view.getVisible(), isDevToolsOpen: webContents.isDevToolsOpened(), lastScreenshot: this._lastScreenshot, - lastFavicon: this._lastFavicon, + lastFavicon: this._favicon.favicon, lastError: this._lastError, certificateError: this.session.trust.getCertificateError(url), storageScope: this.session.storageScope, @@ -757,7 +760,7 @@ export class BrowserView extends Disposable { } private _redirectPinnedNavigation(url: string): boolean { - if (!this.associatedResource || isBrowserViewAssociatedResourceNavigation(this.associatedResource, url)) { + if (!this._shouldRedirectPinnedNavigation(url)) { return false; } @@ -769,6 +772,10 @@ export class BrowserView extends Disposable { return true; } + private _shouldRedirectPinnedNavigation(url: string): boolean { + return !!this.associatedResource && !isBrowserViewAssociatedResourceNavigation(this.associatedResource, url); + } + /** * Get the current URL */ diff --git a/src/vs/platform/browserView/test/common/browserFavicon.test.ts b/src/vs/platform/browserView/test/common/browserFavicon.test.ts new file mode 100644 index 00000000000000..9e8772c5ce6343 --- /dev/null +++ b/src/vs/platform/browserView/test/common/browserFavicon.test.ts @@ -0,0 +1,242 @@ +/*--------------------------------------------------------------------------------------------- + * 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 { BrowserFavicon } from '../../common/browserFavicon.js'; +import { BrowserHistoryStore, IBrowserHistoryItemHandle } from '../../common/browserHistory.js'; + +suite('BrowserFavicon', () => { + const store = ensureNoDisposablesAreLeakedInTestSuite(); + const firstUrl = 'https://first.example/page'; + const secondUrl = 'https://second.example/page'; + const oldIcon = 'data:image/png;base64,b2xk'; + const newIcon = 'data:image/png;base64,bmV3'; + + function createFavicon() { + const requests = new Map>(); + const loaded: (string | undefined)[] = []; + const favicon = store.add(new BrowserFavicon(firstUrl, url => { + if (url.startsWith('data:')) { + return Promise.resolve(url); + } + const request = new DeferredPromise(); + requests.set(url, request); + return request.p; + }, new NullLogService())); + store.add(favicon.onDidLoad(icon => loaded.push(icon))); + return { favicon, requests, loaded }; + } + + test('only the newest request may publish, clear, or try fallback URLs', async () => { + const { favicon, requests, loaded } = createFavicon(); + const oldSuccess = favicon.load(['old-success']); + const oldFailure = favicon.load(['old-failure', 'stale-fallback']); + await favicon.load([newIcon]); + await requests.get('old-success')!.complete(oldIcon); + await requests.get('old-failure')!.error(new Error('Unavailable')); + await Promise.all([oldSuccess, oldFailure]); + + assert.deepStrictEqual({ icon: favicon.favicon, loaded, requests: [...requests.keys()] }, { + icon: newIcon, loaded: [newIcon], requests: ['old-success', 'old-failure'], + }); + }); + + test('tries current fallbacks and clears when none succeeds', async () => { + const { favicon, requests, loaded } = createFavicon(); + const fallback = favicon.load(['missing', newIcon]); + await requests.get('missing')!.error(new Error('Unavailable')); + await fallback; + const missing = favicon.load(['also-missing']); + await requests.get('also-missing')!.error(new Error('Unavailable')); + await missing; + + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: undefined, loaded: [newIcon, undefined] }); + }); + + test('an empty update supersedes an outstanding request', async () => { + const { favicon, requests, loaded } = createFavicon(); + await favicon.load([oldIcon]); + const pending = favicon.load(['pending']); + await favicon.load([]); + await requests.get('pending')!.complete(newIcon); + await pending; + + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: undefined, loaded: [oldIcon, undefined] }); + }); + + test('does not publish a request from the previous document after navigation starts', async () => { + const { favicon, requests, loaded } = createFavicon(); + await favicon.load([oldIcon]); + const pending = favicon.load(['old-document']); + favicon.beginNavigation(secondUrl); + await requests.get('old-document')!.complete(newIcon); + await pending; + const duringNavigation = favicon.favicon; + favicon.commitNavigation(secondUrl); + + assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { + duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], + }); + }); + + for (const finish of ['commit', 'abort', 'failure'] as const) { + test(`keeps provisional icons out of committed state and history until ${finish}`, async () => { + const { favicon } = createFavicon(); + const history = store.add(new BrowserHistoryStore()); + await favicon.load([oldIcon]); + let handle: IBrowserHistoryItemHandle = history.add(firstUrl, 'First', favicon.favicon); + store.add(favicon.onDidLoad(icon => handle.update({ favicon: icon ?? null }))); + favicon.beginNavigation(secondUrl); + await favicon.load([newIcon]); + const duringNavigation = favicon.favicon; + if (finish === 'commit') { + favicon.commitNavigation(secondUrl); + handle = history.add(secondUrl, 'Second', favicon.favicon); + } else if (finish === 'abort') { + favicon.abortNavigation(); + } else { + favicon.failNavigation(); + } + const icons = history.entries.items.map(entry => entry.icon ? history.favicons.get(entry.icon) : undefined); + + assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, icons }, { + duringNavigation: oldIcon, + committed: finish === 'commit' ? newIcon : finish === 'abort' ? oldIcon : undefined, + icons: finish === 'commit' ? [oldIcon, newIcon] : [oldIcon], + }); + }); + } + + test('a candidate request may finish after its document commits', async () => { + const { favicon, requests, loaded } = createFavicon(); + favicon.beginNavigation(secondUrl); + const pending = favicon.load(['new-document']); + favicon.commitNavigation(secondUrl); + favicon.abortNavigation(); + await requests.get('new-document')!.complete(newIcon); + await pending; + + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: newIcon, loaded: [newIcon] }); + }); + + test('replacement history records the committed icon, including an explicit clear', async () => { + const observations = []; + for (const icon of [newIcon, undefined]) { + const { favicon } = createFavicon(); + const history = store.add(new BrowserHistoryStore()); + await favicon.load([oldIcon]); + const handle = history.add(firstUrl, 'First', favicon.favicon); + store.add(favicon.onDidLoad(value => handle.update({ favicon: value ?? null }))); + favicon.beginNavigation(secondUrl); + await favicon.load(icon ? [icon] : []); + const beforeCommit = history.favicons.get(history.entries.items[0].icon!); + favicon.commitNavigation(secondUrl); + handle.update({ url: secondUrl, favicon: favicon.favicon ?? null }); + observations.push({ + beforeCommit, + entries: history.entries.items.map(entry => ({ url: entry.url, icon: entry.icon ? history.favicons.get(entry.icon) : undefined })), + }); + } + assert.deepStrictEqual(observations, [ + { beforeCommit: oldIcon, entries: [{ url: secondUrl, icon: newIcon }] }, + { beforeCommit: oldIcon, entries: [{ url: secondUrl, icon: undefined }] }, + ]); + }); + + test('cross-host redirects invalidate candidates even when returning to the original host', async () => { + const { favicon, requests, loaded } = createFavicon(); + await favicon.load([oldIcon]); + favicon.beginNavigation(firstUrl); + const pending = favicon.load(['intermediate']); + favicon.redirectNavigation(secondUrl); + await favicon.load([newIcon]); + favicon.redirectNavigation(firstUrl); + await requests.get('intermediate')!.complete(newIcon); + await pending; + const duringNavigation = favicon.favicon; + favicon.commitNavigation(firstUrl); + + assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { + duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], + }); + }); + + test('same-host redirects keep the candidate and do not cancel its request', async () => { + const { favicon, requests } = createFavicon(); + await favicon.load([oldIcon]); + favicon.beginNavigation(firstUrl); + const pending = favicon.load(['same-host']); + favicon.redirectNavigation('https://first.example/redirected'); + await requests.get('same-host')!.complete(newIcon); + await pending; + favicon.commitNavigation('https://first.example/redirected'); + + assert.strictEqual(favicon.favicon, newIcon); + }); + + test('superseding provisional navigations do not replace the original committed icon on abort', async () => { + const { favicon, requests, loaded } = createFavicon(); + await favicon.load([oldIcon]); + favicon.beginNavigation(secondUrl); + await favicon.load([newIcon]); + favicon.beginNavigation('https://third.example/'); + const pending = favicon.load(['superseded']); + favicon.commitNavigation(firstUrl + '#same-document', true); + favicon.abortNavigation(); + await requests.get('superseded')!.complete(newIcon); + await pending; + + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: oldIcon, loaded: [oldIcon] }); + }); + + test('same-document navigation preserves current fetches and explicit empty candidates clear at commit', async () => { + const { favicon, requests, loaded } = createFavicon(); + const pending = favicon.load(['current']); + favicon.commitNavigation(firstUrl + '#fragment', true); + await requests.get('current')!.complete(oldIcon); + await pending; + favicon.beginNavigation(firstUrl); + await favicon.load([]); + const duringNavigation = favicon.favicon; + favicon.commitNavigation(firstUrl); + + assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { + duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], + }); + }); + + test('failed loads reject pending and late favicon work until a new navigation', async () => { + const { favicon, requests, loaded } = createFavicon(); + await favicon.load([oldIcon]); + favicon.beginNavigation(secondUrl); + const pending = favicon.load(['failing-document']); + favicon.failNavigation(); + await requests.get('failing-document')!.complete(newIcon); + await pending; + await favicon.load(['after-failure']); + const failedIcon = favicon.favicon; + favicon.beginNavigation(firstUrl); + await favicon.load([newIcon]); + favicon.commitNavigation(firstUrl); + + assert.deepStrictEqual({ failedIcon, recovered: favicon.favicon, loaded, requests: [...requests.keys()] }, { + failedIcon: undefined, recovered: newIcon, loaded: [oldIcon], requests: ['failing-document'], + }); + }); + + test('disposal prevents pending completions and new requests', async () => { + const { favicon, requests, loaded } = createFavicon(); + const pending = favicon.load(['pending']); + favicon.dispose(); + await requests.get('pending')!.complete(newIcon); + await pending; + await favicon.load(['after-disposal']); + + assert.deepStrictEqual({ loaded, requests: [...requests.keys()] }, { loaded: [], requests: ['pending'] }); + }); +}); From 36194a87d7c4f7d8b02ea90e5da26e66f2d5a90f Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 13 Sep 2026 16:16:17 -0700 Subject: [PATCH 2/3] browser: initialize favicon test before navigation Open an empty browser and attach CDP before navigating the local fixture. This keeps the favicon navigation test from depending on the separate initial URL/subscription handoff race that failed Linux CI. Preserve all URL and icon assertions, history checks, and existing timeouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/singlefolder-tests/browser.cdp.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts index fef8b815ed4e2b..466a6395db38f5 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts @@ -210,13 +210,13 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; assert.ok(address && typeof address !== 'string'); const firstUrl = `http://127.0.0.1:${address.port}/first`; const secondUrl = `http://localhost:${address.port}/second`; - const tab = await window.openBrowserTab(firstUrl); + const tab = await window.openBrowserTab(''); const session = await tab.startCDPSession(); try { const { cdpSend } = createHarness(session); const browser: { sessionId: string } = await cdpSend('Target.attachToBrowserTarget'); const targets: { targetInfos: { targetId: string; type: string; url: string }[] } = await cdpSend('Target.getTargets', {}, browser.sessionId); - const target = targets.targetInfos.find(target => target.type === 'page' && target.url === firstUrl); + const target = targets.targetInfos.find(target => target.type === 'page'); assert.ok(target); const page: { sessionId: string } = await cdpSend('Target.attachToTarget', { targetId: target.targetId, flatten: true }, browser.sessionId); const waitForIcon = (url: string, icon: string) => poll( @@ -224,6 +224,8 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; state => state.url === url && state.icon === icon, `Browser favicon for ${url} should be ${icon}`, ); + // Establish the model and its subscriptions before navigating the local fixture. + await cdpSend('Page.navigate', { url: firstUrl }, page.sessionId); await waitForIcon(firstUrl, red); await cdpSend('Page.navigate', { url: secondUrl }, page.sessionId); await waitForIcon(secondUrl, blue); From 6f0baf84d16f0cea9b6cefa75c0d1725e88e3566 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 13 Sep 2026 19:00:46 -0700 Subject: [PATCH 3/3] browser: keep favicon work with its committed document Invalidate favicon work only on document replacement, failure or disposal. Reacquire current-document candidates when Electron suppresses an unchanged URL set, while retaining native updates as the fast path. Cover outgoing-page updates and requests, shared favicon URLs, navigation history, and native candidate filtering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../singlefolder-tests/browser.cdp.test.ts | 225 ++++++++++++++---- .../browserView/common/browserFavicon.ts | 81 ++++--- .../electron-browser/preload-browserView.ts | 15 +- .../browserView/electron-main/browserView.ts | 47 ++-- .../test/common/browserFavicon.test.ts | 200 ++++++++-------- 5 files changed, 367 insertions(+), 201 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts index 466a6395db38f5..96a15578074f20 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/browser.cdp.test.ts @@ -92,6 +92,54 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; return { log, cdpSend, waitForEvent }; } + async function withBrowserPage(run: (tab: vscode.BrowserTab, sessionId: string, harness: ReturnType) => Promise): Promise { + // Attach the model and CDP before navigating so initial subscription timing is not part of the fixture. + const tab = await window.openBrowserTab(''); + const session = await tab.startCDPSession(); + try { + const harness = createHarness(session); + const browser: { sessionId: string } = await harness.cdpSend('Target.attachToBrowserTarget'); + const targets: { targetInfos: { targetId: string; type: string }[] } = await harness.cdpSend('Target.getTargets', {}, browser.sessionId); + const target = targets.targetInfos.find(target => target.type === 'page'); + assert.ok(target); + const page: { sessionId: string } = await harness.cdpSend('Target.attachToTarget', { targetId: target.targetId, flatten: true }, browser.sessionId); + await run(tab, page.sessionId, harness); + } finally { + await session.close(); + } + } + + async function withHttpServer(listener: http.RequestListener, run: (port: number) => Promise): Promise { + const server = http.createServer(listener); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + server.off('error', reject); + resolve(); + }); + }); + try { + const address = server.address(); + assert.ok(address && typeof address !== 'string'); + await run(address.port); + } finally { + server.closeAllConnections(); + await new Promise((resolve, reject) => server.close(error => error ? reject(error) : resolve())); + } + } + + function createFavicon(color: string): string { + return 'data:image/svg+xml;base64,' + Buffer.from(``).toString('base64'); + } + + async function waitForBrowserIcon(tab: vscode.BrowserTab, url: string, icon: string): Promise { + await poll( + async () => ({ url: tab.url, icon: tab.icon instanceof vscode.Uri ? tab.icon.toString(true) : tab.icon instanceof vscode.ThemeIcon ? tab.icon.id : undefined }), + state => state.url === url && state.icon === icon, + `Browser favicon for ${url} should be ${icon}`, + ); + } + /** * Normalize a log for snapshot comparison. Replaces volatile IDs with * stable placeholders and strips file-system-specific paths. @@ -182,9 +230,11 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; (vscode.env.remoteName ? test.skip : test)('favicons follow native document navigation and redirect history', async function () { this.timeout(30_000); - const red = 'data:image/svg+xml;base64,' + Buffer.from('').toString('base64'); - const blue = 'data:image/svg+xml;base64,' + Buffer.from('').toString('base64'); - const server = http.createServer((request, response) => { + const red = createFavicon('red'); + const blue = createFavicon('blue'); + const ignoredIcon = createFavicon('green'); + let sharedIconUrl = ''; + await withHttpServer((request, response) => { const url = new URL(request.url ?? '/', 'http://localhost'); if (url.pathname === '/redirect') { response.writeHead(302, { Location: url.searchParams.get('to')! }); @@ -192,62 +242,141 @@ const CAPTURED_DOMAINS = ['Browser', 'Target']; } else if (url.pathname === '/favicon.ico') { response.writeHead(404); response.end(); + } else if (url.pathname === '/shared.svg') { + response.writeHead(200, { 'Content-Type': 'image/svg+xml' }); + response.end(Buffer.from(blue.slice(blue.indexOf(',') + 1), 'base64')); } else { - const icon = url.pathname === '/first' ? red : url.pathname === '/second' ? blue : undefined; + const icon = url.pathname === '/first' || url.pathname === '/same-icon' ? red : url.pathname === '/second' ? blue : url.pathname === '/shared-icon' ? sharedIconUrl : undefined; + const ignoredLinks = url.pathname === '/shared-icon' + ? `` + : ''; response.writeHead(200, { 'Content-Type': 'text/html', 'Cache-Control': 'no-store' }); - response.end(`${url.pathname}${icon ? `` : ''}${url.pathname}`); + response.end(`${url.pathname}${icon ? `` : ''}${ignoredLinks}${url.pathname}`); } - }); - await new Promise((resolve, reject) => { - server.once('error', reject); - server.listen(0, '127.0.0.1', () => { - server.off('error', reject); - resolve(); - }); - }); - try { - const address = server.address(); - assert.ok(address && typeof address !== 'string'); - const firstUrl = `http://127.0.0.1:${address.port}/first`; - const secondUrl = `http://localhost:${address.port}/second`; - const tab = await window.openBrowserTab(''); - const session = await tab.startCDPSession(); - try { - const { cdpSend } = createHarness(session); - const browser: { sessionId: string } = await cdpSend('Target.attachToBrowserTarget'); - const targets: { targetInfos: { targetId: string; type: string; url: string }[] } = await cdpSend('Target.getTargets', {}, browser.sessionId); - const target = targets.targetInfos.find(target => target.type === 'page'); - assert.ok(target); - const page: { sessionId: string } = await cdpSend('Target.attachToTarget', { targetId: target.targetId, flatten: true }, browser.sessionId); - const waitForIcon = (url: string, icon: string) => poll( - async () => ({ url: tab.url, icon: tab.icon instanceof vscode.Uri ? tab.icon.toString(true) : tab.icon instanceof vscode.ThemeIcon ? tab.icon.id : undefined }), - state => state.url === url && state.icon === icon, - `Browser favicon for ${url} should be ${icon}`, - ); - // Establish the model and its subscriptions before navigating the local fixture. - await cdpSend('Page.navigate', { url: firstUrl }, page.sessionId); + }, async port => { + sharedIconUrl = `http://127.0.0.1:${port}/shared.svg`; + const firstUrl = `http://127.0.0.1:${port}/first`; + const secondUrl = `http://localhost:${port}/second`; + await withBrowserPage(async (tab, sessionId, { cdpSend, waitForEvent }) => { + const waitForIcon = (url: string, icon: string) => waitForBrowserIcon(tab, url, icon); + await cdpSend('Page.navigate', { url: firstUrl }, sessionId); await waitForIcon(firstUrl, red); - await cdpSend('Page.navigate', { url: secondUrl }, page.sessionId); + await cdpSend('Page.navigate', { url: secondUrl }, sessionId); await waitForIcon(secondUrl, blue); - const history: { currentIndex: number; entries: { id: number }[] } = await cdpSend('Page.getNavigationHistory', {}, page.sessionId); - await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, page.sessionId); + const history: { currentIndex: number; entries: { id: number }[] } = await cdpSend('Page.getNavigationHistory', {}, sessionId); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, sessionId); await waitForIcon(firstUrl, red); - await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex].id }, page.sessionId); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex].id }, sessionId); await waitForIcon(secondUrl, blue); - await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, page.sessionId); + await cdpSend('Page.navigateToHistoryEntry', { entryId: history.entries[history.currentIndex - 1].id }, sessionId); await waitForIcon(firstUrl, red); - const finalUrl = `http://127.0.0.1:${address.port}/iconless`; - const intermediate = `http://localhost:${address.port}/redirect?to=${encodeURIComponent(finalUrl)}`; - await cdpSend('Page.navigate', { url: `http://127.0.0.1:${address.port}/redirect?to=${encodeURIComponent(intermediate)}` }, page.sessionId); + await cdpSend('Page.enable', {}, sessionId); + const sameIconUrl = `http://localhost:${port}/same-icon`; + const loaded = waitForEvent(message => message.method === 'Page.loadEventFired' && message.sessionId === sessionId); + await cdpSend('Page.navigate', { url: sameIconUrl }, sessionId); + await loaded; + await waitForIcon(sameIconUrl, red); + + for (const host of ['127.0.0.1', 'localhost']) { + const url = `http://${host}:${port}/shared-icon`; + const loaded = waitForEvent(message => message.method === 'Page.loadEventFired' && message.sessionId === sessionId); + await cdpSend('Page.navigate', { url }, sessionId); + await loaded; + await waitForIcon(url, blue); + } + const sharedHistory: typeof history = await cdpSend('Page.getNavigationHistory', {}, sessionId); + await cdpSend('Page.navigateToHistoryEntry', { entryId: sharedHistory.entries[sharedHistory.currentIndex - 1].id }, sessionId); + await waitForIcon(`http://127.0.0.1:${port}/shared-icon`, blue); + await cdpSend('Page.navigateToHistoryEntry', { entryId: sharedHistory.entries[sharedHistory.currentIndex].id }, sessionId); + await waitForIcon(`http://localhost:${port}/shared-icon`, blue); + + await cdpSend('Page.navigate', { url: firstUrl }, sessionId); + await waitForIcon(firstUrl, red); + const finalUrl = `http://127.0.0.1:${port}/iconless`; + const intermediate = `http://localhost:${port}/redirect?to=${encodeURIComponent(finalUrl)}`; + await cdpSend('Page.navigate', { url: `http://127.0.0.1:${port}/redirect?to=${encodeURIComponent(intermediate)}` }, sessionId); await waitForIcon(finalUrl, 'globe'); - } finally { - await session.close(); + }); + }); + }); + + (vscode.env.remoteName ? test.skip : test)('favicons retain outgoing document updates across cancelled navigation', async function () { + this.timeout(30_000); + const red = createFavicon('red'); + const blue = createFavicon('blue'); + let pendingRequested = false; + await withHttpServer((request, response) => { + if (request.url === '/pending') { + pendingRequested = true; + return; } - } finally { - server.closeAllConnections(); - await new Promise((resolve, reject) => server.close(error => error ? reject(error) : resolve())); - } + response.writeHead(200, { 'Content-Type': 'text/html' }); + response.end(`Retained pageRetained page`); + }, async port => { + await withBrowserPage(async (tab, sessionId, { cdpSend, waitForEvent }) => { + const firstUrl = `http://127.0.0.1:${port}/first`; + await cdpSend('Page.navigate', { url: firstUrl }, sessionId); + await waitForBrowserIcon(tab, firstUrl, red); + const contextReady = waitForEvent(message => message.method === 'Runtime.executionContextCreated' && message.sessionId === sessionId && message.params.context.auxData?.isDefault); + await cdpSend('Runtime.enable', {}, sessionId); + const context: { params: { context: { uniqueId: string } } } = await contextReady; + const target = `http://localhost:${port}/pending`; + try { + const source: { result: { value: string }; exceptionDetails?: object } = await cdpSend('Runtime.evaluate', { + expression: `(() => { const source = location.href; location.href = ${JSON.stringify(target)}; setTimeout(() => document.querySelector('link').href = ${JSON.stringify(blue)}, 0); return source; })()`, + uniqueContextId: context.params.context.uniqueId, + returnByValue: true, + }, sessionId); + assert.deepStrictEqual({ url: source.result.value, exception: source.exceptionDetails }, { url: firstUrl, exception: undefined }); + await poll(async () => pendingRequested, requested => requested, 'Destination headers should still be pending'); + await waitForBrowserIcon(tab, firstUrl, blue); + } finally { + await cdpSend('Page.stopLoading', {}, sessionId); + } + await waitForBrowserIcon(tab, firstUrl, blue); + }); + }); + }); + + (vscode.env.remoteName ? test.skip : test)('favicon recovery preserves cached defaults without new CSP-blocked requests', async function () { + this.timeout(30_000); + const red = createFavicon('red'); + const defaultRequests: http.IncomingHttpHeaders[] = []; + await withHttpServer((request, response) => { + if (request.url === '/favicon.ico') { + defaultRequests.push(request.headers); + response.writeHead(200, { 'Content-Type': 'image/svg+xml' }); + response.end(Buffer.from(red.slice(red.indexOf(',') + 1), 'base64')); + return; + } + response.writeHead(200, { + 'Content-Type': 'text/html', + 'Cache-Control': 'no-store', + ...(request.url === '/blocked' ? { 'Content-Security-Policy': 'img-src \'none\'' } : {}), + }); + response.end('Default faviconDefault favicon'); + }, async port => { + await withBrowserPage(async (tab, sessionId, { cdpSend, waitForEvent }) => { + const firstUrl = `http://127.0.0.1:${port}/first`; + await cdpSend('Page.navigate', { url: firstUrl }, sessionId); + await waitForBrowserIcon(tab, firstUrl, red); + await cdpSend('Page.enable', {}, sessionId); + // Same-origin navigation keeps the cached icon; a new blocked origin must not be fetched. + for (const [host, path, icon] of [ + ['127.0.0.1', '/second', red], + ['127.0.0.1', '/blocked', red], + ['localhost', '/blocked', 'globe'], + ]) { + const url = `http://${host}:${port}${path}`; + const loaded = waitForEvent(message => message.method === 'Page.loadEventFired' && message.sessionId === sessionId); + await cdpSend('Page.navigate', { url }, sessionId); + await loaded; + await waitForBrowserIcon(tab, url, icon); + } + assert.strictEqual(defaultRequests.filter(headers => headers.host === `localhost:${port}`).length, 0, 'CSP must prevent a default favicon request on the new host'); + }); + }); }); // Loads `file:////index.html`. Skipped in remote diff --git a/src/vs/platform/browserView/common/browserFavicon.ts b/src/vs/platform/browserView/common/browserFavicon.ts index d189ca21577568..c7d2f43551f974 100644 --- a/src/vs/platform/browserView/common/browserFavicon.ts +++ b/src/vs/platform/browserView/common/browserFavicon.ts @@ -10,13 +10,13 @@ import { ILogService } from '../../log/common/log.js'; interface FaviconDocument { url: string; favicon: string | undefined; + requestId: number; + readId: number; } -/** Keeps provisional navigation icons separate from the committed document. */ +/** Keeps favicon requests with their committed document until it is replaced. */ export class BrowserFavicon extends Disposable { private _document: FaviconDocument | undefined; - private _navigation: FaviconDocument | undefined; - private _requestId = 0; private readonly _onDidLoad = this._register(new Emitter()); /** Fires when a fetch updates the committed document; navigation changes are published by the view. */ @@ -24,38 +24,19 @@ export class BrowserFavicon extends Disposable { constructor( url: string, + private readonly readFaviconUrls: () => Promise, private readonly fetchFavicon: (url: string) => Promise, @ILogService private readonly logService: ILogService, ) { super(); - this._document = { url, favicon: undefined }; + this._document = { url, favicon: undefined, requestId: 0, readId: 0 }; } get favicon(): string | undefined { return this._document?.favicon; } - beginNavigation(url: string): void { - this._requestId++; - this._navigation = { - url, - favicon: URL.parse(url)?.host === URL.parse(this._document?.url ?? '')?.host ? this.favicon : undefined, - }; - } - - redirectNavigation(url: string): void { - if (!this._navigation) { - this.beginNavigation(url); - return; - } - if (URL.parse(url)?.host !== URL.parse(this._navigation.url)?.host) { - this._requestId++; - this._navigation.favicon = undefined; - } - this._navigation.url = url; - } - - /** Promotes the candidate before the view records history and publishes its navigation events. */ + /** Changes ownership before the view records history and publishes navigation events. */ commitNavigation(url: string, sameDocument = false): void { if (sameDocument) { if (this._document) { @@ -63,30 +44,44 @@ export class BrowserFavicon extends Disposable { } return; } - this.redirectNavigation(url); - this._document = this._navigation; - this._navigation = undefined; - } - - abortNavigation(): void { - if (this._navigation) { - this._requestId++; - this._navigation = undefined; - } + this._document = { + url, + favicon: URL.parse(url)?.host === URL.parse(this._document?.url ?? '')?.host ? this.favicon : undefined, + requestId: 0, + readId: 0, + }; } failNavigation(): void { - this._requestId++; - this._navigation = undefined; this._document = undefined; } + /** Reacquires candidates when Electron suppresses an unchanged URL set across documents. */ + async refresh(): Promise { + const document = this._document; + if (this._store.isDisposed || !document) { + return; + } + const requestId = document.requestId; + const readId = ++document.readId; + let urls: readonly string[] | undefined; + try { + urls = await this.readFaviconUrls(); + } catch (error) { + this.logService.trace('[BrowserFavicon] Failed to read document favicons.', error); + return; + } + if (urls !== undefined && readId === document.readId && this.isCurrentRequest(document, requestId)) { + await this.load(urls); + } + } + async load(urls: readonly string[]): Promise { - const document = this._navigation ?? this._document; + const document = this._document; if (this._store.isDisposed || !document) { return; } - const requestId = ++this._requestId; + const requestId = ++document.requestId; let favicon: string | undefined; for (const url of urls) { try { @@ -94,7 +89,7 @@ export class BrowserFavicon extends Disposable { } catch (error) { this.logService.trace('[BrowserFavicon] Failed to fetch favicon, trying the next candidate.', error); } - if (this._store.isDisposed || requestId !== this._requestId) { + if (!this.isCurrentRequest(document, requestId)) { return; } if (favicon !== undefined) { @@ -103,8 +98,12 @@ export class BrowserFavicon extends Disposable { } const changed = document.favicon !== favicon; document.favicon = favicon; - if (document === this._document && changed) { + if (changed) { this._onDidLoad.fire(favicon); } } + + private isCurrentRequest(document: FaviconDocument, requestId: number): boolean { + return !this._store.isDisposed && document === this._document && requestId === document.requestId; + } } diff --git a/src/vs/platform/browserView/electron-browser/preload-browserView.ts b/src/vs/platform/browserView/electron-browser/preload-browserView.ts index 1b6c35278abaab..aa15f9012d5ce1 100644 --- a/src/vs/platform/browserView/electron-browser/preload-browserView.ts +++ b/src/vs/platform/browserView/electron-browser/preload-browserView.ts @@ -258,7 +258,20 @@ function init() { const mainWorldHelpers = { getElement, /** Opaque token exposed for CDP-side frame matching. */ - getFrameToken(): string { return frameToken; } + getFrameToken(): string { return frameToken; }, + getFaviconUrls(): string[] | undefined { + if (document.readyState === 'loading') { + return undefined; + } + const links = document.head + ? [...document.head.children] + : document.documentElement instanceof SVGSVGElement ? [...document.querySelectorAll('link')] : []; + const urls = links + .filter((link): link is HTMLLinkElement => link instanceof HTMLLinkElement && link.matches('[rel~="icon" i]')) + .filter(link => link.href && URL.canParse(link.href) && (!link.media || window.matchMedia(link.media).matches)) + .map(link => link.href); + return [...new Set(urls)].sort(); + } }; try { diff --git a/src/vs/platform/browserView/electron-main/browserView.ts b/src/vs/platform/browserView/electron-main/browserView.ts index e6642ac21dd66e..a8f9abf2353765 100644 --- a/src/vs/platform/browserView/electron-main/browserView.ts +++ b/src/vs/platform/browserView/electron-main/browserView.ts @@ -24,6 +24,7 @@ import { ITelemetryService } from '../../telemetry/common/telemetry.js'; import { logBrowserOpen } from '../common/browserViewTelemetry.js'; import { URI } from '../../../base/common/uri.js'; import { BrowserFavicon } from '../common/browserFavicon.js'; +import { Schemas } from '../../../base/common/network.js'; enum NewPageLocation { Foreground = 'foreground', @@ -39,6 +40,7 @@ export class BrowserView extends Disposable { private readonly _view: WebContentsView; private readonly _faviconRequestCache = new Map>(); private readonly _favicon: BrowserFavicon; + private _nativeFaviconUrls: readonly string[] = []; private _lastScreenshot: VSBuffer | undefined = undefined; private _lastError: IBrowserViewLoadError | undefined = undefined; @@ -161,7 +163,7 @@ export class BrowserView extends Disposable { // Passing an `undefined` webContents triggers an error in Electron. ...(options?.webContents ? { webContents: options.webContents } : {}) }); - this._favicon = this._register(new BrowserFavicon(this._view.webContents.getURL(), url => this._fetchFavicon(url), this.logService)); + this._favicon = this._register(new BrowserFavicon(this._view.webContents.getURL(), () => this._readFaviconUrls(), url => this._fetchFavicon(url), this.logService)); // Use a default size of 1024x768. // Important: The bounds here must be on-screen, otherwise some OSes (like macOS) may not actually start rendering. @@ -291,13 +293,13 @@ export class BrowserView extends Disposable { this._onDidChangeFavicon.fire({ favicon }); })); webContents.on('page-favicon-updated', (_event, favicons) => { + this._nativeFaviconUrls = favicons; void this._favicon.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 && !this._shouldRedirectPinnedNavigation(url)) { - this._favicon.beginNavigation(url); - } - }); + const refreshFavicon = () => { + void this._favicon.refresh().catch(error => this.logService.warn('[BrowserView] Failed to refresh favicon.', error)); + }; + this._register(Event.fromNodeEventEmitter(webContents, 'dom-ready')(refreshFavicon)); webContents.on('will-navigate', (event) => { if (this._redirectPinnedNavigation(event.url)) { event.preventDefault(); @@ -309,9 +311,6 @@ export class BrowserView extends Disposable { event.preventDefault(); return; } - if (event.isMainFrame && !event.isSameDocument) { - this._favicon.redirectNavigation(event.url); - } }); // Title events @@ -332,6 +331,7 @@ export class BrowserView extends Disposable { this._recordNavigation(url); if (!sameDocument) { this._onDidChangeFavicon.fire({ favicon: this._favicon.favicon }); + refreshFavicon(); } }; @@ -349,7 +349,6 @@ export class BrowserView extends Disposable { } }); webContents.on('did-stop-loading', () => { - this._favicon.abortNavigation(); fireLoadingEvent(false); }); webContents.on('did-fail-load', (e, errorCode, errorDescription, validatedURL, isMainFrame) => { @@ -527,6 +526,28 @@ export class BrowserView extends Disposable { }); } + private async _readFaviconUrls(): Promise { + const webContents = this._view.webContents; + const frame = webContents.mainFrame; + // Unlike webContents.executeJavaScript, this also runs while the outgoing page has a pending navigation. + const urls = await frame.executeJavaScript('globalThis.__vscode_helpers?.getFaviconUrls()'); + if (webContents.isDestroyed() || frame.isDestroyed() || frame.detached || frame !== webContents.mainFrame || urls === undefined || urls === null) { + return undefined; + } + if (!Array.isArray(urls) || !urls.every(url => typeof url === 'string')) { + throw new Error('Invalid document favicon URLs'); + } + if (!urls.length) { + const documentUrl = URI.parse(frame.url); + if (documentUrl.scheme === Schemas.http || documentUrl.scheme === Schemas.https) { + const defaultUrl = new URL('/favicon.ico', frame.url).href; + // Reuse only a default candidate already requested through Chromium's CSP-aware notification. + return this._nativeFaviconUrls.filter(url => url === defaultUrl && this._faviconRequestCache.has(url)); + } + } + return urls; + } + private _fetchFavicon(url: string): Promise { let request = this._faviconRequestCache.get(url); if (!request) { @@ -760,7 +781,7 @@ export class BrowserView extends Disposable { } private _redirectPinnedNavigation(url: string): boolean { - if (!this._shouldRedirectPinnedNavigation(url)) { + if (!this.associatedResource || isBrowserViewAssociatedResourceNavigation(this.associatedResource, url)) { return false; } @@ -772,10 +793,6 @@ export class BrowserView extends Disposable { return true; } - private _shouldRedirectPinnedNavigation(url: string): boolean { - return !!this.associatedResource && !isBrowserViewAssociatedResourceNavigation(this.associatedResource, url); - } - /** * Get the current URL */ diff --git a/src/vs/platform/browserView/test/common/browserFavicon.test.ts b/src/vs/platform/browserView/test/common/browserFavicon.test.ts index 9e8772c5ce6343..5f9bf4c22bb589 100644 --- a/src/vs/platform/browserView/test/common/browserFavicon.test.ts +++ b/src/vs/platform/browserView/test/common/browserFavicon.test.ts @@ -17,10 +17,10 @@ suite('BrowserFavicon', () => { const oldIcon = 'data:image/png;base64,b2xk'; const newIcon = 'data:image/png;base64,bmV3'; - function createFavicon() { + function createFavicon(readFaviconUrls: () => Promise = async () => undefined) { const requests = new Map>(); const loaded: (string | undefined)[] = []; - const favicon = store.add(new BrowserFavicon(firstUrl, url => { + const favicon = store.add(new BrowserFavicon(firstUrl, readFaviconUrls, url => { if (url.startsWith('data:')) { return Promise.resolve(url); } @@ -69,59 +69,39 @@ suite('BrowserFavicon', () => { assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: undefined, loaded: [oldIcon, undefined] }); }); - test('does not publish a request from the previous document after navigation starts', async () => { - const { favicon, requests, loaded } = createFavicon(); - await favicon.load([oldIcon]); - const pending = favicon.load(['old-document']); - favicon.beginNavigation(secondUrl); - await requests.get('old-document')!.complete(newIcon); - await pending; - const duringNavigation = favicon.favicon; - favicon.commitNavigation(secondUrl); - - assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { - duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], - }); - }); - - for (const finish of ['commit', 'abort', 'failure'] as const) { - test(`keeps provisional icons out of committed state and history until ${finish}`, async () => { - const { favicon } = createFavicon(); - const history = store.add(new BrowserHistoryStore()); - await favicon.load([oldIcon]); - let handle: IBrowserHistoryItemHandle = history.add(firstUrl, 'First', favicon.favicon); - store.add(favicon.onDidLoad(icon => handle.update({ favicon: icon ?? null }))); - favicon.beginNavigation(secondUrl); - await favicon.load([newIcon]); - const duringNavigation = favicon.favicon; - if (finish === 'commit') { + for (const replaced of [false, true]) { + test(`an in-flight request ${replaced ? 'cannot outlive a full commit' : 'keeps its committed document as owner'}`, async () => { + const { favicon, requests, loaded } = createFavicon(); + const pending = favicon.load(['outgoing']); + if (replaced) { favicon.commitNavigation(secondUrl); - handle = history.add(secondUrl, 'Second', favicon.favicon); - } else if (finish === 'abort') { - favicon.abortNavigation(); - } else { - favicon.failNavigation(); } - const icons = history.entries.items.map(entry => entry.icon ? history.favicons.get(entry.icon) : undefined); + await requests.get('outgoing')!.complete(newIcon); + await pending; - assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, icons }, { - duringNavigation: oldIcon, - committed: finish === 'commit' ? newIcon : finish === 'abort' ? oldIcon : undefined, - icons: finish === 'commit' ? [oldIcon, newIcon] : [oldIcon], + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { + icon: replaced ? undefined : newIcon, loaded: replaced ? [] : [newIcon], }); }); } - test('a candidate request may finish after its document commits', async () => { - const { favicon, requests, loaded } = createFavicon(); - favicon.beginNavigation(secondUrl); - const pending = favicon.load(['new-document']); + test('updates the outgoing history entry until a new document commits', async () => { + const { favicon } = createFavicon(); + const history = store.add(new BrowserHistoryStore()); + await favicon.load([oldIcon]); + let handle: IBrowserHistoryItemHandle = history.add(firstUrl, 'First', favicon.favicon); + store.add(favicon.onDidLoad(icon => handle.update({ favicon: icon ?? null }))); + await favicon.load([newIcon]); + const beforeCommit = favicon.favicon; favicon.commitNavigation(secondUrl); - favicon.abortNavigation(); - await requests.get('new-document')!.complete(newIcon); - await pending; + handle = history.add(secondUrl, 'Second', favicon.favicon); + await favicon.load([oldIcon]); - assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: newIcon, loaded: [newIcon] }); + assert.deepStrictEqual({ + beforeCommit, + current: favicon.favicon, + icons: history.entries.items.map(entry => entry.icon ? history.favicons.get(entry.icon) : undefined), + }, { beforeCommit: newIcon, current: oldIcon, icons: [newIcon, oldIcon] }); }); test('replacement history records the committed icon, including an explicit clear', async () => { @@ -132,11 +112,10 @@ suite('BrowserFavicon', () => { await favicon.load([oldIcon]); const handle = history.add(firstUrl, 'First', favicon.favicon); store.add(favicon.onDidLoad(value => handle.update({ favicon: value ?? null }))); - favicon.beginNavigation(secondUrl); - await favicon.load(icon ? [icon] : []); const beforeCommit = history.favicons.get(history.entries.items[0].icon!); favicon.commitNavigation(secondUrl); handle.update({ url: secondUrl, favicon: favicon.favicon ?? null }); + await favicon.load(icon ? [icon] : []); observations.push({ beforeCommit, entries: history.entries.items.map(entry => ({ url: entry.url, icon: entry.icon ? history.favicons.get(entry.icon) : undefined })), @@ -148,84 +127,113 @@ suite('BrowserFavicon', () => { ]); }); - test('cross-host redirects invalidate candidates even when returning to the original host', async () => { + test('same-URL replacement cannot resurrect an outgoing request after clearing the icon', async () => { const { favicon, requests, loaded } = createFavicon(); await favicon.load([oldIcon]); - favicon.beginNavigation(firstUrl); - const pending = favicon.load(['intermediate']); - favicon.redirectNavigation(secondUrl); - await favicon.load([newIcon]); - favicon.redirectNavigation(firstUrl); - await requests.get('intermediate')!.complete(newIcon); - await pending; - const duringNavigation = favicon.favicon; + const pending = favicon.load(['superseded']); favicon.commitNavigation(firstUrl); + await favicon.load([]); + await requests.get('superseded')!.complete(newIcon); + await pending; - assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { - duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], - }); + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: undefined, loaded: [oldIcon, undefined] }); }); - test('same-host redirects keep the candidate and do not cancel its request', async () => { - const { favicon, requests } = createFavicon(); - await favicon.load([oldIcon]); - favicon.beginNavigation(firstUrl); - const pending = favicon.load(['same-host']); - favicon.redirectNavigation('https://first.example/redirected'); - await requests.get('same-host')!.complete(newIcon); + test('same-document navigation preserves current fetches', async () => { + const { favicon, requests, loaded } = createFavicon(); + const pending = favicon.load(['current']); + favicon.commitNavigation(firstUrl + '#fragment', true); + await requests.get('current')!.complete(oldIcon); await pending; - favicon.commitNavigation('https://first.example/redirected'); - assert.strictEqual(favicon.favicon, newIcon); + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: oldIcon, loaded: [oldIcon] }); }); - test('superseding provisional navigations do not replace the original committed icon on abort', async () => { - const { favicon, requests, loaded } = createFavicon(); + test('reacquires identical candidates after a cross-host commit', async () => { + const { favicon, loaded } = createFavicon(async () => [oldIcon]); + await favicon.refresh(); + favicon.commitNavigation(secondUrl); + const atCommit = favicon.favicon; + await favicon.refresh(); + + assert.deepStrictEqual({ atCommit, icon: favicon.favicon, loaded }, { atCommit: undefined, icon: oldIcon, loaded: [oldIcon, oldIcon] }); + }); + + test('only the latest document read may publish', async () => { + const first = new DeferredPromise(); + const second = new DeferredPromise(); + const reads = [first, second]; + const { favicon, loaded } = createFavicon(() => reads.shift()!.p); + const older = favicon.refresh(); + const newer = favicon.refresh(); + await first.complete([oldIcon]); + await older; + const beforeLatest = favicon.favicon; + await second.complete([newIcon]); + await newer; + + assert.deepStrictEqual({ beforeLatest, icon: favicon.favicon, loaded }, { beforeLatest: undefined, icon: newIcon, loaded: [newIcon] }); + }); + + test('a newer native request takes precedence over an outstanding document read', async () => { + const read = new DeferredPromise(); + const { favicon, requests, loaded } = createFavicon(() => read.p); + const reading = favicon.refresh(); + const loading = favicon.load(['native']); + await read.complete([oldIcon]); + await reading; + await requests.get('native')!.complete(newIcon); + await loading; + + assert.deepStrictEqual({ icon: favicon.favicon, loaded, requests: [...requests.keys()] }, { + icon: newIcon, loaded: [newIcon], requests: ['native'], + }); + }); + + test('discards a document read across a same-URL commit', async () => { + const read = new DeferredPromise(); + const { favicon, loaded } = createFavicon(() => read.p); await favicon.load([oldIcon]); - favicon.beginNavigation(secondUrl); - await favicon.load([newIcon]); - favicon.beginNavigation('https://third.example/'); - const pending = favicon.load(['superseded']); - favicon.commitNavigation(firstUrl + '#same-document', true); - favicon.abortNavigation(); - await requests.get('superseded')!.complete(newIcon); - await pending; + favicon.commitNavigation(firstUrl); + const reading = favicon.refresh(); + favicon.commitNavigation(firstUrl); + await read.complete([newIcon]); + await reading; assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: oldIcon, loaded: [oldIcon] }); }); - test('same-document navigation preserves current fetches and explicit empty candidates clear at commit', async () => { - const { favicon, requests, loaded } = createFavicon(); - const pending = favicon.load(['current']); - favicon.commitNavigation(firstUrl + '#fragment', true); - await requests.get('current')!.complete(oldIcon); - await pending; - favicon.beginNavigation(firstUrl); - await favicon.load([]); - const duringNavigation = favicon.favicon; - favicon.commitNavigation(firstUrl); + for (const fails of [false, true]) { + test(`a document read that ${fails ? 'fails' : 'is not ready'} does not invalidate an active fetch`, async () => { + const { favicon, requests, loaded } = createFavicon(async () => { + if (fails) { + throw new Error('Frame detached'); + } + return undefined; + }); + const pending = favicon.load(['pending']); + await favicon.refresh(); + await requests.get('pending')!.complete(newIcon); + await pending; - assert.deepStrictEqual({ duringNavigation, committed: favicon.favicon, loaded }, { - duringNavigation: oldIcon, committed: undefined, loaded: [oldIcon], + assert.deepStrictEqual({ icon: favicon.favicon, loaded }, { icon: newIcon, loaded: [newIcon] }); }); - }); + } test('failed loads reject pending and late favicon work until a new navigation', async () => { const { favicon, requests, loaded } = createFavicon(); await favicon.load([oldIcon]); - favicon.beginNavigation(secondUrl); const pending = favicon.load(['failing-document']); favicon.failNavigation(); await requests.get('failing-document')!.complete(newIcon); await pending; await favicon.load(['after-failure']); const failedIcon = favicon.favicon; - favicon.beginNavigation(firstUrl); - await favicon.load([newIcon]); favicon.commitNavigation(firstUrl); + await favicon.load([newIcon]); assert.deepStrictEqual({ failedIcon, recovered: favicon.favicon, loaded, requests: [...requests.keys()] }, { - failedIcon: undefined, recovered: newIcon, loaded: [oldIcon], requests: ['failing-document'], + failedIcon: undefined, recovered: newIcon, loaded: [oldIcon, newIcon], requests: ['failing-document'], }); });