Skip to content

Fix Node#contains infinite loop for indirect descendants - #641

Open
yzxcj797 wants to merge 1 commit into
Shopify:mainfrom
yzxcj797:fix/node-contains-infinite-loop
Open

Fix Node#contains infinite loop for indirect descendants#641
yzxcj797 wants to merge 1 commit into
Shopify:mainfrom
yzxcj797:fix/node-contains-infinite-loop

Conversation

@yzxcj797

Copy link
Copy Markdown

What?

Node#contains() never terminates for any indirect descendant: the walk re-reads the original node's parentNode on every iteration instead of advancing.

contains(node: Node | null) {
  let currentNode: Node | null = node;
  while (true) {
    if (currentNode == null) return false;
    if (currentNode === this) return true;
    currentNode = node!.parentNode;   // ← never advances past the argument's own parent
  }
}

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 direct fetch heartbeats all stopping at the call site.

Fix

Advance the walk with currentNode.parentNode, per the Node.prototype.contains contract.

Testing

New packages/polyfill/source/tests/node.test.ts (vitest, same harness as the existing serialization.test.ts): direct child, indirect descendant (the hanging case), self, detached node, sibling subtrees, and null — 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant