Stop state pseudo-classes from re-entering the engine - #178
Conversation
|
@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. |
0c2f3b6 to
c88f2fa
Compare
Current fixThis 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 costThe 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 coverageTests 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. |
c88f2fa to
45c07d5
Compare
|
@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. |
|
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. |
0b09e98 to
e6ad525
Compare
eb63768 to
7a8a32e
Compare
7a8a32e to
847523e
Compare
':autofill' and ':-webkit-autofill' are handled by the pseudo_nop group, which breaks out of the compile loop without emitting a test. A resolver with no test accepts whatever it is given, so ':autofill' matches every element in the document and 'input:autofill' matches every input. Emitting a test that never passes keeps the selector valid, which is what the group is for, and answers it the way a host with no autofill state should: matching nothing. That is also what the reference engine answers. The case that reads the host, further down the same switch, is unreachable today because this group claims the selector first. Routing there would be the browser-accurate answer, and it belongs with the re-entrancy fix in dperini#178, since the call it makes is the one that recurses under a host whose matcher routes back into this engine. References: - Spec: https://html.spec.whatwg.org/#selector-autofill — ':autofill' matches a control the user agent has autofilled - Chromium: https://github.com/chromium/chromium/blob/155.0.8041.1/third_party/blink/renderer/core/css/selector_checker.cc#L2778 — the state is read from the control, not inferred - MDN: https://developer.mozilla.org/en-US/docs/Web/CSS/:autofill
847523e to
5ff4409
Compare
|
The merge audit found two compatibility gaps, so I am leaving this open.
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. |
d526ff2 to
7b24fe0
Compare
7b24fe0 to
35c9cb6
Compare
2 participants
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.