Skip to content

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

Merged
jdalton merged 1 commit into
dperini:masterfrom
jdalton:fix/jsdom-reentry
Sep 6, 2026
Merged

jdalton merged 1 commit into
dperini:masterfrom
jdalton:fix/jsdom-reentry

Conversation

@jdalton

@jdalton jdalton commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

Fix

Cache host delegation without freezing the selected matcher. Element and prototype replacements invalidate the delegation result. Changing LEGACY clears mode-dependent document records, including cached aliases and absent matchers.

Unchanged matchers retain the per-document fast path. Legacy hosts without WeakMap retain bounded caching. Modern mode does not probe prefixed aliases. The existing CommonJS factory shapes and live browser state are preserved.

Tests

Focused tests cover the original jsdom adapter route, matcher replacement, exceptions, mode changes, document switching, legacy aliases, and missing WeakMap. Chromium checks cover browser and CommonJS factories, iframes, modal/popover changes, and installation before and after caching.

The WPT runner includes local cache regressions using the upstream test harness. The complete local Docker CI workflow passes with unchanged coverage thresholds; aggregate coverage is about 74%.

Fixes #171, fixes #172, fixes #177. Related uninstall and autofill fixes remain separate in #195 and #196.

@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

Current fix

This PR now preserves matcher replacement and LEGACY changes. An unchanged delegating matcher is cached per document, with a fast path for consecutive queries. Changing the selected function clears its delegation result; changing LEGACY clears mode-dependent document records.

Compatibility and cost

The engine reads the current standard matcher so element overrides and prototype replacements stay live. It does not infer native provenance from function source. Cached delegation avoids repeated calls back into nwsapi. Genuine browser matchers still run for each state query.

Legacy aliases are resolved only in legacy mode. WeakMap-backed records survive document switching; hosts without WeakMap retain a bounded fallback. Existing CommonJS factory shapes remain supported.

Regression coverage

Tests cover the jsdom adapter route, replacement after exceptions or delegation, mode changes, alternating documents, legacy aliases, missing WeakMap, browser bootstrap, iframes, modal/popover changes, and installation before or after caching. Local browser regressions also run through the upstream WPT harness. The full local CI workflow passes with unchanged coverage thresholds.

Simple tests now use direct default imports from the generated CommonJS module. CommonJS interception tests retain require intentionally.

The related uninstall and autofill defects are tracked separately in #195 and #196.

@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

The cache optimization now preserves live matcher replacement and LEGACY changes. It reuses delegation results only while the selected matcher remains unchanged. Standard matcher reads remain necessary to observe host changes; prefixed aliases are cached only in legacy mode.

The two later audit regressions are covered by unit tests and local browser regressions using the upstream WPT harness. The full local CI workflow passes without lowering coverage thresholds. See the current explanation for compatibility details and the related fixes in #195 and #196.

@jdalton

jdalton commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

The merge audit found two compatibility gaps, so I am leaving this open.

  • An element-level matcher can change after an unsupported-selector exception. The regression on current master returns false and then true after replacing node.matches; this branch returns false twice because it reads the prototype instead.
  • With standard matches absent and a working prefixed matcher, querying in modern mode and then enabling LEGACY still returns false. The cached document record is not refreshed when the mode changes.

Both were reproduced against this PR head, 5ff4409, and compared with current master. The new native-reentry tests on master also cover the first behavior. The existing reentry guard has already removed the stack-exhaustion problem; this cache optimization needs to preserve host behavior and invalidate mode-dependent records before it can merge.

@jdalton
jdalton force-pushed the fix/jsdom-reentry branch 2 times, most recently from d526ff2 to 7b24fe0 Compare September 6, 2026 04:43
@jdalton
jdalton merged commit 715f652 into dperini:master Sep 6, 2026
@jdalton

jdalton commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Both audit regressions are fixed and covered. The repaired PR passed the complete local Docker workflow and GitHub CI on top of merged #195, and is now merged. Issues #171, #172, and #177 are resolved in source; this does not indicate a new npm release. The related autofill fix continues in #196.

@jdalton jdalton mentioned this pull request Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants