Fix Node#contains infinite loop for indirect descendants - #641
Open
yzxcj797 wants to merge 1 commit into
Open
Conversation
The contains() walk re-read the ORIGINAL node's parentNode on every iteration (currentNode = node.parentNode instead of currentNode.parentNode), so it never advanced past the first step: direct children happened to return true (the first step hits the receiver), but any indirect descendant re-read the same middle node forever. In a synchronous environment such as a Web Worker running the remote DOM polyfill, that single call deadlocks the entire event loop with no exception — reported downstream as a silent permanent freeze in twenty's front component sandbox (twentyhq/twenty#24573). Advance the walk with currentNode.parentNode, per the Node.prototype.contains contract. New node.test.ts covers direct and indirect descendants, self, detached nodes, sibling subtrees and null. Differential: on the previous code the indirect-descendant case does not merely fail — it hangs the test runner indefinitely (verified by killing the run after several minutes and by an isolated child-process repro that never terminates); with this change the whole suite passes in under five seconds.
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.
What?
Node#contains()never terminates for any indirect descendant: the walk re-reads the original node'sparentNodeon every iteration instead of advancing.For a direct child the first step happens to hit the receiver and returns
true— which makes the bug look intermittent: only deeper nesting hangs. For an indirect descendant the loop re-reads the same middle node forever. In a synchronous environment — a Web Worker running the polyfill as its DOM — that single call deadlocks the whole event loop with no exception and no recovery short of terminating the worker.This is reported downstream in the wild: twenty's front-component Remote DOM worker freezes permanently whenever a component hit-tests with
Element.contains()on anything nested deeper than one level (twentyhq/twenty#24573); a maintainer-confirmed MEDIUM with the worker's timers and directfetchheartbeats all stopping at the call site.Fix
Advance the walk with
currentNode.parentNode, per theNode.prototype.containscontract.Testing
New
packages/polyfill/source/tests/node.test.ts(vitest, same harness as the existingserialization.test.ts): direct child, indirect descendant (the hanging case), self, detached node, sibling subtrees, andnull— 6/6 pass.Differential: on the previous code the indirect-descendant case does not merely fail — it hangs the runner indefinitely (the run had to be killed after several minutes; an isolated child-process repro of the exact loop also never terminates against a three-level chain). With the change the whole file passes in under five seconds.