Skip to content

fix: correctly traverse ancestor chain in Node.contains#931

Merged
karfau merged 1 commit intoxmldom:masterfrom
yoshi389111:fix_contains_infinite_loop
Aug 14, 2025
Merged

fix: correctly traverse ancestor chain in Node.contains#931
karfau merged 1 commit intoxmldom:masterfrom
yoshi389111:fix_contains_infinite_loop

Conversation

@yoshi389111
Copy link
Copy Markdown
Contributor

@yoshi389111 yoshi389111 commented Aug 13, 2025

Hello, and thank you for maintaining this project!
I found a small bug in Node.contains() and would like to propose a fix.

Description

Node.contains() currently does not traverse the ancestor chain correctly, and therefore does not work correctly when the reference node is a deeper descendant.

Problem:

The current implementation resets parent to other.parentNode in each loop iteration, so it only checks the immediate parent and in an infinite loop.

Fix:

Assign parent = parent.parentNode instead, so the loop correctly walks up the ancestor chain.

Reproduction Example:

const { DOMParser } = require("@xmldom/xmldom");

const xml = "<root><foo><bar><baz>test</baz></bar></foo></root>";
const doc = new DOMParser().parseFromString(xml, "application/xml");
const rootNode = doc.documentElement;
const fooNode = rootNode?.firstChild;
const barNode = fooNode?.firstChild;
const bazNode = barNode?.firstChild;

console.log(fooNode?.contains(barNode)); // <- true
console.log(fooNode?.contains(bazNode)); // <- Before fix: infinite loop, After fix: true

Thank you for reviewing this pull request! I hope this helps improve the library.

Copilot AI review requested due to automatic review settings August 13, 2025 14:00
Copilot AI reviewed Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a critical bug in the Node.contains() method where the function would enter an infinite loop when checking if a node contains a deeper descendant. The issue was caused by incorrectly resetting the parent pointer to the same node in each iteration instead of traversing up the ancestor chain.

  • Fixed the infinite loop bug by correcting the parent traversal logic
  • Added test coverage for the descendant containment scenario

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/dom.js Fixed the bug by changing parent = other.parentNode to parent = parent.parentNode to properly traverse the ancestor chain
test/dom/node.test.js Added test case and setup to verify that contains() works correctly for deeper descendants

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment thread lib/dom.js
@karfau karfau changed the title Fix Node.contains() not traversing ancestor chain correctly fix: correctly traverse ancestor chain in Node.contains() Aug 14, 2025
@karfau karfau changed the title fix: correctly traverse ancestor chain in Node.contains() fix: correctly traverse ancestor chain in Node.contains Aug 14, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 14, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.12%. Comparing base (8ced78c) to head (0a8e32b).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #931   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files           8        8           
  Lines        2196     2196           
  Branches      577      577           
=======================================
  Hits         2089     2089           
  Misses        107      107

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@karfau
Copy link
Copy Markdown
Member

karfau commented Aug 14, 2025

Awesome, thx for your finding and contribution!

@karfau karfau added this to the 0.9.9 milestone Aug 14, 2025
@karfau karfau merged commit 9dcc14f into xmldom:master Aug 14, 2025
38 checks passed
@karfau
Copy link
Copy Markdown
Member

karfau commented Aug 14, 2025 •
edited
Loading

If you are up for it you can also provide a pull request with a backport for the 0.8 version, but I can also take care of it.

@yoshi389111
Copy link
Copy Markdown
Contributor Author

yoshi389111 commented Aug 14, 2025

Thanks for the suggestion! I think I’ll pass on the backport this time, but I appreciate the offer.

@yoshi389111 yoshi389111 deleted the fix_contains_infinite_loop branch August 14, 2025 22:10
@karfau
Copy link
Copy Markdown
Member

karfau commented Aug 17, 2025

FYI: The method contains was only added in version 0.9.3, so there is nothing to backport the fix to in v0.8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@karfau karfau karfau approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.9

Development

Successfully merging this pull request may close these issues.

3 participants

@yoshi389111 @karfau