diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index fd526c4b4..09f995ac0 100644 --- a/packages/core/src/browser.js +++ b/packages/core/src/browser.js @@ -10,6 +10,7 @@ import logger from '@percy/logger'; import install from './install.js'; import { MAX_CDP_PAYLOAD } from './network.js'; import Session from './session.js'; +import { pendingCommand } from './utils.js'; import Page from './page.js'; // Chrome features Percy disables for v143 new-headless asset discovery. @@ -204,6 +205,8 @@ export class Browser extends EventEmitter { // reject any pending callbacks for (let callback of this.#callbacks.values()) { + clearTimeout(callback.timer); + callback.reject(Object.assign(callback.error, { message: `Protocol error (${callback.method}): Browser closed.` })); @@ -278,7 +281,7 @@ export class Browser extends EventEmitter { return page; } - async send(method, params) { + async send(method, params, { timeout } = {}) { /* istanbul ignore next: * difficult to test failure here without mocking private properties */ if (!this.isConnected()) throw new Error('Browser not connected'); @@ -294,10 +297,7 @@ export class Browser extends EventEmitter { // send the message payload this.ws.send(JSON.stringify({ id, method, params })); - // will resolve or reject when a matching response is received - return new Promise((resolve, reject) => { - this.#callbacks.set(id, { error: new Error(), resolve, reject, method }); - }); + return pendingCommand(this.#callbacks, id, method, timeout); } } @@ -368,6 +368,7 @@ export class Browser extends EventEmitter { // resolve or reject a pending promise created with #send() let callback = this.#callbacks.get(data.id); this.#callbacks.delete(data.id); + clearTimeout(callback.timer); /* istanbul ignore next: races with page._handleMessage() */ if (data.error) { diff --git a/packages/core/src/page.js b/packages/core/src/page.js index 1a5db7aac..d4e41384f 100644 --- a/packages/core/src/page.js +++ b/packages/core/src/page.js @@ -7,7 +7,9 @@ import { hostname, waitFor, waitForTimeout as sleep, - serializeFunction + serializeFunction, + normalizeEvalException, + DEFAULT_CDP_CLOSE_TIMEOUT } from './utils.js'; // Internal ceiling on the customElements wait. Set tight (500ms) so a @@ -95,13 +97,17 @@ export class Page { // Close the page async close() { let browser = this.session.browser; - await this.session.close(); + + await this.session.close().catch(error => { + this.log.debug('Failed to close page session', this.meta); + this.log.debug(error); + }); if (this.browserContextId && browser) { /* istanbul ignore next: safety net for already-disposed contexts */ await browser.send('Target.disposeBrowserContext', { browserContextId: this.browserContextId - }).catch(() => {}); + }, { timeout: DEFAULT_CDP_CLOSE_TIMEOUT }).catch(() => {}); } this.log.debug('Page closed', this.meta); @@ -216,7 +222,7 @@ export class Page { }); if (exceptionDetails) { - throw exceptionDetails.exception.description; + throw normalizeEvalException(exceptionDetails); } else { return result.value; } diff --git a/packages/core/src/session.js b/packages/core/src/session.js index bd19e720b..2a319fe54 100644 --- a/packages/core/src/session.js +++ b/packages/core/src/session.js @@ -1,5 +1,6 @@ import EventEmitter from 'events'; import logger from '@percy/logger'; +import { pendingCommand, DEFAULT_CDP_CLOSE_TIMEOUT } from './utils.js'; export class Session extends EventEmitter { #callbacks = new Map(); @@ -33,10 +34,10 @@ export class Session extends EventEmitter { await this.browser.send('Target.closeTarget', { targetId: this.targetId - }).catch(this._handleClosedError); + }, { timeout: DEFAULT_CDP_CLOSE_TIMEOUT }).catch(this._handleClosedError); } - async send(method, params) { + async send(method, params, { timeout } = {}) { /* istanbul ignore next: race condition paranoia */ if (this.closedReason) { throw new Error(`Protocol error (${method}): ${this.closedReason}`); @@ -45,10 +46,7 @@ export class Session extends EventEmitter { // send a raw message to the browser so we can provide a sessionId let id = await this.browser.send({ sessionId: this.sessionId, method, params }); - // will resolve or reject when a matching response is received - return new Promise((resolve, reject) => { - this.#callbacks.set(id, { error: new Error(), resolve, reject, method }); - }); + return pendingCommand(this.#callbacks, id, method, timeout); } _handleMessage(data) { @@ -56,6 +54,7 @@ export class Session extends EventEmitter { // resolve or reject a pending promise created with #send() let callback = this.#callbacks.get(data.id); this.#callbacks.delete(data.id); + clearTimeout(callback.timer); /* istanbul ignore next: races with browser._handleMessage() */ if (data.error) { @@ -75,8 +74,9 @@ export class Session extends EventEmitter { _handleClose() { this.closedReason ||= 'Session closed.'; - // reject any pending callbacks for (let callback of this.#callbacks.values()) { + clearTimeout(callback.timer); + callback.reject(Object.assign(callback.error, { message: `Protocol error (${callback.method}): ${this.closedReason}` })); diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index 917e36988..728e36f62 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -19,6 +19,51 @@ export { createServer } from './server.js'; +export const DEFAULT_CDP_TIMEOUT = 300000; + +export const DEFAULT_CDP_CLOSE_TIMEOUT = 10000; + +export function cdpTimeout(override) { + if (override != null) return override; + let configured = parseInt(process.env.PERCY_CDP_TIMEOUT, 10); + return Number.isFinite(configured) ? configured : DEFAULT_CDP_TIMEOUT; +} + +export function pendingCommand(callbacks, id, method, timeout) { + return new Promise((resolve, reject) => { + let callback = { error: new Error(), resolve, reject, method }; + callbacks.set(id, callback); + + let ms = cdpTimeout(timeout); + if (!(ms > 0)) return; + + callback.timer = setTimeout(() => { + callbacks.delete(id); + + reject(Object.assign(callback.error, { + message: `Protocol error (${method}): Timed out after ${ms}ms` + })); + }, ms); + + callback.timer.unref?.(); + }); +} + +export function normalizeEvalException(exceptionDetails) { + let { exception, text } = exceptionDetails ?? {}; + let description = exception?.description; + + if (typeof description === 'string') return new Error(description); + + let value = exception && 'value' in exception ? exception.value : undefined; + + let detail = typeof value === 'string' ? value + : value !== undefined ? JSON.stringify(value) + : exception?.type ?? 'unknown'; + + return new Error(`${text || 'Page evaluation failed'}: ${detail}`); +} + // Returns the hostname portion of a URL. export function hostname(url) { return new URL(url).hostname; diff --git a/packages/core/test/unit/cdp-timeout.test.js b/packages/core/test/unit/cdp-timeout.test.js new file mode 100644 index 000000000..fb2c7f83d --- /dev/null +++ b/packages/core/test/unit/cdp-timeout.test.js @@ -0,0 +1,136 @@ +import { + cdpTimeout, + pendingCommand, + normalizeEvalException, + DEFAULT_CDP_TIMEOUT +} from '../../src/utils.js'; +import { setupTest } from '../helpers/index.js'; + +describe('Unit / CDP command deadlines', () => { + beforeEach(async () => { + await setupTest(); + delete process.env.PERCY_CDP_TIMEOUT; + }); + + afterEach(() => { + delete process.env.PERCY_CDP_TIMEOUT; + }); + + describe('cdpTimeout', () => { + it('defaults when nothing is configured', () => { + expect(cdpTimeout()).toBe(DEFAULT_CDP_TIMEOUT); + }); + + it('prefers an explicit override', () => { + process.env.PERCY_CDP_TIMEOUT = '1234'; + expect(cdpTimeout(50)).toBe(50); + }); + + it('reads PERCY_CDP_TIMEOUT', () => { + process.env.PERCY_CDP_TIMEOUT = '4321'; + expect(cdpTimeout()).toBe(4321); + }); + + it('ignores a non-numeric PERCY_CDP_TIMEOUT', () => { + process.env.PERCY_CDP_TIMEOUT = 'soon'; + expect(cdpTimeout()).toBe(DEFAULT_CDP_TIMEOUT); + }); + + it('allows opting out with zero', () => { + process.env.PERCY_CDP_TIMEOUT = '0'; + expect(cdpTimeout()).toBe(0); + }); + }); + + describe('pendingCommand', () => { + it('resolves when the response settles the callback', async () => { + let callbacks = new Map(); + let pending = pendingCommand(callbacks, 1, 'Target.closeTarget', 10000); + + let callback = callbacks.get(1); + clearTimeout(callback.timer); + callbacks.delete(1); + callback.resolve({ ok: true }); + + await expectAsync(pending).toBeResolvedTo({ ok: true }); + }); + + it('rejects a command the browser never answers', async () => { + let callbacks = new Map(); + let pending = pendingCommand(callbacks, 2, 'Target.closeTarget', 50); + + await expectAsync(pending).toBeRejectedWithError( + 'Protocol error (Target.closeTarget): Timed out after 50ms'); + expect(callbacks.has(2)).toBe(false); + }); + + it('does not reject a command that settled before the deadline', async () => { + let callbacks = new Map(); + let pending = pendingCommand(callbacks, 3, 'Runtime.callFunctionOn', 50); + + let callback = callbacks.get(3); + clearTimeout(callback.timer); + callbacks.delete(3); + callback.resolve('done'); + + await expectAsync(pending).toBeResolvedTo('done'); + await new Promise(r => setTimeout(r, 80)); + await expectAsync(pending).toBeResolvedTo('done'); + }); + + it('never registers a deadline when disabled', async () => { + let callbacks = new Map(); + pendingCommand(callbacks, 4, 'Page.navigate', 0); + expect(callbacks.get(4).timer).toBeUndefined(); + }); + }); + + describe('normalizeEvalException', () => { + it('keeps an error description', () => { + let error = normalizeEvalException({ + exception: { type: 'object', subtype: 'error', description: 'Error: boom\n at ' } + }); + + expect(error instanceof Error).toBe(true); + expect(error.message).toContain('Error: boom'); + }); + + it('reports a string rejection that carries no description', () => { + let error = normalizeEvalException({ + text: 'Uncaught (in promise)', + exception: { type: 'string', value: 'sections-one-column-layout-hero--home-page' } + }); + + expect(error instanceof Error).toBe(true); + expect(error.message).toBe( + 'Uncaught (in promise): sections-one-column-layout-hero--home-page'); + }); + + it('reports an undefined rejection instead of throwing undefined', () => { + let error = normalizeEvalException({ + text: 'Uncaught (in promise)', + exception: { type: 'undefined' } + }); + + expect(error instanceof Error).toBe(true); + expect(error.message).toBe('Uncaught (in promise): undefined'); + }); + + it('reports a null rejection', () => { + let error = normalizeEvalException({ + text: 'Uncaught (in promise)', + exception: { type: 'object', subtype: 'null', value: null } + }); + + expect(error instanceof Error).toBe(true); + expect(error.message).toBe('Uncaught (in promise): null'); + }); + + it('falls back when there are no details at all', () => { + let error = normalizeEvalException({}); + + expect(error instanceof Error).toBe(true); + expect(error.message).toBe('Page evaluation failed: unknown'); + }); + }); +});