Skip to content

Fix selector cache element identity reuse (#1341) - #1342

Merged
chubes4 merged 1 commit into
trunkfrom
fix-1341-selector-cache-element-identity
Aug 29, 2026
Merged

Fix selector cache element identity reuse (#1341)#1342
chubes4 merged 1 commit into
trunkfrom
fix-1341-selector-cache-element-identity

Conversation

@chubes4

@chubes4 chubes4 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #1341. Unblocks the SvgMaterializationTrait extraction under #242.

CssSelectorMatchCache::elementKey() used bare spl_object_id($element) for class tokens, attributes, attribute names, selector results, and candidate-rule lists. PHP reuses that integer as soon as a temporary DOMElement wrapper is released, while the scalar cache entry survives. A later wrapper for a different source node could therefore inherit every cached selector input and result belonging to the first node.

Reproduction

The regression creates two distinct connected elements, primes all five cache surfaces with the first, releases its PHP wrapper, then fetches the second. The runtime immediately gives the second wrapper the first wrapper's spl_object_id().

Against the parent commit:

FAIL: class-token cache does not alias a distinct element with a recycled wrapper ID
FAIL: attribute cache does not alias a distinct element with a recycled wrapper ID
FAIL: attribute-name cache does not alias a distinct element with a recycled wrapper ID
FAIL: selector-result cache does not alias a distinct element with a recycled wrapper ID
FAIL: candidate-rule cache does not alias a distinct element with a recycled wrapper ID
CssSelectorMatcher unit tests: 5 failed, 81 passed

Here: CssSelectorMatcher unit tests: 86 passed.

Identity model

Connected nodes use DOMNode::getNodePath(). A document path is stable across multiple PHP wrappers around the same native node and unique within this cache's immutable document revision. This preserves cache hits when repeated DOM lookups return different wrappers.

Detached nodes use a monotonically increasing token held in a WeakMap. Detached nodes can share paths such as /p, so path identity is unsafe there. The weak key avoids retaining wrappers; the monotonic value is never reused, so scalar entries cannot alias a later wrapper.

clear() resets both identity stores alongside the cached values. Existing mutation call sites already clear the immutable revision cache when source paths or attributes change.

I first tried a weak token for every wrapper. That prevented aliasing but destroyed legitimate cache hits because fetching the same native node can produce another PHP wrapper. The connected-path/detached-token split preserves both correctness and the existing cache behavior.

Blast radius

This is not an extraction-only edge case. Correct identity changes output for 331 of 383 corpus documents on current trunk, adding 92,643 bytes of block markup in aggregate while leaving total diagnostic and fallback counts unchanged. The old cache was broadly suppressing or misclassifying output using another element's selector state.

The motivating fixture is now context-independent:

52-css-3d-world/about.html
isolated:    21,929 bytes
full corpus: 21,929 bytes

Before this fix, moving SVG methods to a collaborator changed temporary wrapper lifetime and made that document produce 22,011 bytes alone but 21,929 bytes during the corpus run. The refactor did not change selector logic; it changed which stale entry won.

Verification

  • composer test: exit 0, including 289 parity fixtures and the package-install proof.
  • Selector benchmark retains bounded LRU behavior: 7,168 hits, 9,216 misses, 5,120 evictions, 4,096 peak entries over 16,384 accesses.
  • CSS-attached corpus: 383 documents, 0 throwing, 383 distinct hashes.
  • The paused SVG extraction will be rebased onto this fix and must return 0 differing against this corrected baseline before it resumes.

AI assistance disclosure: implemented and drafted by OpenAI GPT-5.6 Sol running in OpenCode, operated by @chubes4. The AI found the defect while extracting SvgMaterializationTrait, reproduced wrapper-ID reuse, wrote and proved the five-surface regression against the parent commit, refined the initial all-WeakMap approach to preserve cross-wrapper cache hits, and ran the suite, benchmark, and corpus measurements quoted above. Reviewed by a human before opening.

@chubes4
chubes4 merged commit dca5da8 into trunk Aug 29, 2026
10 of 11 checks passed
@chubes4
chubes4 deleted the fix-1341-selector-cache-element-identity branch August 29, 2026 04:16
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.

CssSelectorMatchCache aliases distinct DOM elements when spl_object_id is reused

1 participant