Fix selector cache element identity reuse (#1341) - #1342
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1341. Unblocks the
SvgMaterializationTraitextraction under #242.CssSelectorMatchCache::elementKey()used barespl_object_id($element)for class tokens, attributes, attribute names, selector results, and candidate-rule lists. PHP reuses that integer as soon as a temporaryDOMElementwrapper 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:
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:
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.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.