Skip to content

Stop state pseudo-classes from re-entering the engine - #178

Open
jdalton wants to merge 1 commit into
dperini:masterfrom
jdalton:fix/jsdom-reentry
Open

Stop state pseudo-classes from re-entering the engine#178
jdalton wants to merge 1 commit into
dperini:masterfrom
jdalton:fix/jsdom-reentry

Conversation

@jdalton

@jdalton jdalton commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Stop state selectors from re-entering nwsapi through a host's Element.matches(). Fixes #172, #171 and #177.

Builds on merged #197. The matcher cache uses its shared allocator after legacy mode is selected. Modern and legacy hosts with WeakMap retain delegation results across documents; legacy hosts without it use a bounded single-document cache. Prefixed matcher aliases are checked only in legacy mode and cached per document. Modern mode uses .matches and retains the factory guards for CommonJS callers.

Validation: all 27 Node tests pass, including jsdom 26.1.0, both legacy paths, and alias lookup counts. All 5 Chromium scenarios pass across factory shapes, iframe documents, and installation before or after caching. Reproduction commands are in test/display-state-testing.md.

@charlesverge

Copy link
Copy Markdown

@jdalton The key part of the solution I proposed was the elimination of the need to use a call to the matches function to detect if it is a native function or not. This heads off future bugs like this.

@jdalton

jdalton commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Updated after reviewing the overhead concern and testing both PRs: capturing a known platform matcher once is simpler, but #176's CommonJS path also disables browser state unless existing callers supply its new third argument. This PR preserves that behavior and now reduces the repeated lookup/detection work.

My earlier wording needed two corrections. “At most once per document” was too broad for the previous one-entry cache; switching documents discarded the result. Also, a nested call returns false to break recursion. The outer call has not produced an answer yet.

The revised optimization

Document matcher records now live in a WeakMap, with a last-document fast path. Consecutive queries avoid both the map lookup and the former ownerMatcher() helper call; alternating documents reuses each document's delegation result. The active record is marked directly on re-entry, removing the separate re-entry flag and retaining the result even if the host subsequently throws.

The regression test alternates two documents over 50 rounds and six state pseudo-classes: one delegating host call per document in total. The cache now uses createWeakMap() from merged #197 after legacy mode is selected. Modern mode uses the native constructor; legacy mode checks support once and reuses the result. Prefixed matcher aliases are resolved only in legacy mode and cached per document. The Node suite also covers alias lookup counts and the shared allocator. Legacy hosts without WeakMap retain bounded recursion and the single-document cache; the persistent per-document guarantee applies when WeakMap is available.

There is still a cache comparison/guard check during matching. Real browser matchers must still be called to obtain current state, just as #176 calls its captured matcher. This change does not inspect function source or assume a captured function is native.

Why capturing the factory global alone is insufficient

I checked #176 at ea7d2f3 in Chromium with a modal dialog and an open popover. Both native answers were true. Browser-script initialization worked, but both factory({ document, DOMException }) and factory(window) returned false / false. Explicitly supplying #176's new third argument restored the results. Master and this PR preserve the existing factory calls.

Capturing Element.prototype.matches also captures any pre-existing shim. In a separate jsdom browser-bootstrap reproduction where that prototype delegated into the newly created engine, #176 re-entered until a test limiter threw on call 21; #178 made one call. That is a conditional shimmed-host case, not a claim about an untouched browser prototype.

Reading from the node's document provides the matcher when the factory receives only a document and supports querying iframe documents. My earlier blanket claim that cross-realm browser matchers necessarily fail was too strong; the Chromium checks here also pass for those elements.

Regression coverage included in the PR

npm test: 27 passing tests, including the original jsdom 26.1.0 Element.matches route, alternating documents in both modes, browser bootstrap with an already-delegating prototype, exceptions, nested document changes, and the fallback without WeakMap.

npm run test:browser: all 5 Chromium scenarios pass (6 tests including the parent). Modal/popover state agrees with the browser before opening, while open, and after closing, across browser scripts, both CommonJS factory shapes, iframe documents, and installation before/after caching.

The tests and reproduction instructions are committed with the optimization.

Two unrelated bugs the review turned up, now their own PRs

Both are already on master and neither is caused by this patch.

#195 is uninstall() restoring querySelectorAll from _querySelector, the single-result method, rather than from _querySelectorAll. After one install and uninstall cycle a host is left with querySelectorAll answering one element instead of a list.

#196 is :autofill matching every element in the document. The pseudo_nop group breaks out of the compile loop without emitting a test, and a resolver with no test accepts whatever it is handed, so input:autofill matches every input.

The second one touches this patch indirectly. The case 'autofill' handler further down the switch is unreachable, because that group claims the selector first, and the call it would make is a raw e.matches() with no guard. Wiring it up without the change in this PR would bring back the recursion this PR is about, so #196 leaves it unreachable and says why.

@charlesverge

Copy link
Copy Markdown

@jdalton Your code makes calls repeatedly on every use to a function to detect if it is native or not native. This wastes cpu cycles and is prone to create future bugs.

@jdalton

jdalton commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

@charlesverge I followed up on the overhead concern with 7a8a32e. The optimization is now in #178, together with regression coverage for the original jsdom reproduction and the browser cases discussed below.

The repeated ownerMatcher(node) helper call is gone. Document matcher records are retained in a WeakMap, with a last-document fast path so consecutive queries also avoid the map lookup. The active record is marked directly on re-entry, replacing the separate re-entry flag and preserving the delegation result even when the host subsequently throws. The cache now uses createWeakMap() from merged #197 after legacy mode is selected. Modern mode uses the native constructor; legacy mode checks support once and reuses the result. Prefixed matcher aliases are resolved only in legacy mode and cached per document. The Node suite also covers alias lookup counts and the shared allocator.

This also corrects my earlier “at most once per document” claim. At 45c07d5, the one-entry cache discarded that information when documents alternated: 10 alternating queries made 10 host calls. The new regression alternates two documents over 50 rounds and six state pseudo-classes, with one delegating host call per document in total. Legacy hosts without WeakMap retain the guarded single-document fallback, so the persistent per-document guarantee applies when WeakMap is available.

There is still a cache comparison and re-entry guard during matching. A functioning browser matcher continues to be called to obtain current selector state; #176 also calls its captured matcher for those state queries. Capturing a function alone does not establish native provenance.

For the compatibility check, I tested #176 at ea7d2f3 against actual Chromium state: a dialog after showModal() and a popover after showPopover(). Both browser-native answers were true:

Factory/bootstrap master 2e9498f #176 ea7d2f3 #178 with optimization
Browser script true / true true / true true / true
CommonJS factory({ document, DOMException }) true / true false / false true / true
CommonJS factory(window) true / true false / false true / true

In #176, only the browser bootstrap supplies platformMatches. Existing CommonJS callers get the unconditional-false helper, even when given a real browser window. Explicitly supplying the new third argument restores both results, but requires existing callers to change their initialization. Its three synthetic-element tests do not cover this browser/factory combination.

In a separate jsdom browser-bootstrap test where Element.prototype.matches already delegated into the newly created engine, #176 repeatedly re-entered until my test limiter threw on call 21. #178, including the optimization, made one call. That is a conditional shimmed-host case, not a claim that an untouched browser prototype recurses.

The committed coverage now checks all of these cases:

  • npm test: 27 passing tests, including jsdom 26.1.0's actual Element.matches route, alternating documents in both modes, browser bootstrap with a delegating prototype, exceptions, nested document changes, and the fallback without WeakMap.
  • npm run test:browser: all 5 Chromium scenarios pass (6 tests including the parent). Modal/popover results agree with the browser before opening, while open, and after closing, across browser scripts, both CommonJS factory shapes, iframe documents, and installation before/after caching.

The optimization reduces repeated lookup and delegation detection while preserving changing browser state. I also corrected the earlier comment so the first explanation readers encounter reflects the current implementation.

@jdalton
jdalton force-pushed the fix/jsdom-reentry branch 2 times, most recently from 7d95120 to eb63768 Compare September 5, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2.2.26+: matches(':modal') recurses to stack overflow under jsdom - ~1.3s per call, always returns false

2 participants