Skip to content

fix(core): customElements wait read its timeout from the wrong argument, hanging the CLI forever (PER-10405) #2383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aryanku-dev merged 1 commit into from
Aug 11, 2026
Merged

fix(core): customElements wait read its timeout from the wrong argument, hanging the CLI forever (PER-10405) #2383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions packages/core/src/page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,40 @@ export const DEFAULT_WAIT_FOR_CUSTOM_ELEMENTS_TIMEOUT = 500;
// IMPORTANT: this body is intentionally ES5 — it is evaluated in the
// page's realm and must work in any browser the page targets. Don't
// "modernize" with arrow functions, let/const, or optional chaining.
//
// serializeFunction() injects the Percy helpers object as `arguments[0]`, so
// the caller's first argument is `arguments[1]`.
export const WAIT_FOR_CUSTOM_ELEMENTS_BODY = `
var deadline = Date.now() + (arguments[0] || 500);
var timeoutMs = typeof arguments[1] === 'number' && isFinite(arguments[1]) && arguments[1] > 0
? arguments[1]
: 500;
var deadline = Date.now() + timeoutMs;
return new Promise(function(resolve) {
var settled = false;
function finish() {
if (settled) return;
settled = true;
resolve();
}
setTimeout(finish, timeoutMs);

@aryanku-dev aryanku-dev Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Ceiling setTimeout is never cleared on early resolve

When finish() is reached via the normal tick path (the common case — :not(:defined) empties on the first tick), this ceiling timer stays pending in the page realm until timeoutMs elapses. Harmless, since finish() is idempotent via settled, but it is an avoidable lingering timer.

Suggestion: capture the id and clear it in finish():

var t = setTimeout(finish, timeoutMs);
function finish() {
  if (settled) return;
  settled = true;
  clearTimeout(t);
  resolve();
}

Reviewer: stack-code-reviewer

function tick() {
if (settled) return;
var undef = document.querySelectorAll(":not(:defined)");
if (!undef.length) return resolve();
if (Date.now() >= deadline) return resolve();
if (!undef.length) return finish();
if (Date.now() >= deadline) return finish();
var names = {};
for (var i = 0; i < undef.length; i++) names[undef[i].localName] = true;
var promises = Object.keys(names).map(function(n) {
return window.customElements.whenDefined(n).catch(function(){});
try {
return window.customElements.whenDefined(n).catch(function(){});
} catch (e) {
return Promise.resolve();
}
});
Promise.race([
Promise.all(promises),
new Promise(function(r) { setTimeout(r, 100); })
]).then(tick);
]).then(tick, finish);
}
tick();
});
Expand Down
31 changes: 30 additions & 1 deletion packages/core/test/percy.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import Percy from '@percy/core';
import Pako from 'pako';
import DetectProxy from '@percy/client/detect-proxy';
import { validateSnapshotOptions } from '../src/snapshot.js';
import { Page, WAIT_FOR_CUSTOM_ELEMENTS_BODY } from '../src/page.js';
import {
Page,
WAIT_FOR_CUSTOM_ELEMENTS_BODY,
DEFAULT_WAIT_FOR_CUSTOM_ELEMENTS_TIMEOUT
} from '../src/page.js';

describe('Percy', () => {
let percy, server;
Expand Down Expand Up @@ -216,6 +220,31 @@ describe('Percy', () => {
]));
});

it('does not hang on a page holding a never-defined custom element', async () => {
// PER-10405: an unregistered tag matches `:not(:defined)` forever and its
// `customElements.whenDefined()` never resolves, so the wait must fall back
// on its own ceiling.
server.reply('/', () => [200, 'text/html', (
'<p>hi</p><next-route-announcer></next-route-announcer>'
)]);
await percy.browser.launch();
let page = await percy.browser.page();
await page.goto('http://localhost:8000');

let undefinedCount = await page.eval(
() => document.querySelectorAll(':not(:defined)').length);
expect(undefinedCount).toBe(1);

let start = Date.now();
await expectAsync(page.eval(
WAIT_FOR_CUSTOM_ELEMENTS_BODY,
DEFAULT_WAIT_FOR_CUSTOM_ELEMENTS_TIMEOUT
)).toBeResolved();
let elapsed = Date.now() - start;

expect(elapsed).toBeLessThan(5000);

@aryanku-dev aryanku-dev Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Elapsed assertion is upper-bound only

< 5000 against a 500 ms timeout guards against a hang but not much else. Since the fixture element is genuinely never defined, the only way this resolves well before the deadline is a future regression that exits the wait early for the wrong reason — which this assertion would not catch.

Suggestion: pin the deadline branch with a loose lower bound alongside it.

Suggested change
expect(elapsed).toBeLessThan(5000);
expect(elapsed).toBeGreaterThanOrEqual(400);
expect(elapsed).toBeLessThan(5000);

Reviewer: stack-code-reviewer

});

it('runs readiness check before serializing when readiness option is set', async () => {
server.reply('/', () => [200, 'text/html', '<p>Hello Percy!</p>']);

Expand Down
Loading