Invalidate iterator over elements of a XPathResult when the document changes (#39411)

Also includes a fix to not throw a type error in
`XPathResult.invalidIteratorState`.

Testing: Includes a new web platform test
Part of https://github.com/servo/servo/issues/34527

---------

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

components/script/dom/xpathresult.rs

@@ -6,11 +6,13 @@ use std::cell::{Cell, RefCell};
 
 use dom_struct::dom_struct;
 use js::rust::HandleObject;
+use script_bindings::codegen::GenericBindings::WindowBinding::WindowMethods;
 
 use crate::dom::bindings::codegen::Bindings::XPathResultBinding::{
     XPathResultConstants, XPathResultMethods,
 };
 use crate::dom::bindings::error::{Error, Fallible};
+use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::{Reflector, reflect_dom_object_with_proto};
 use crate::dom::bindings::root::{Dom, DomRoot};
 use crate::dom::bindings::str::DOMString;
@@ -84,9 +86,11 @@ impl From<Value> for XPathResultValue {
 pub(crate) struct XPathResult {
     reflector_: Reflector,
     window: Dom<Window>,
+    /// The revision of the owner document when this result was created. When iterating over the
+    /// values in the result, this is used to invalidate the iterator when the document is modified.
+    version: Cell<u64>,
     result_type: Cell<XPathResultType>,
     value: RefCell<XPathResultValue>,
-    iterator_invalid: Cell<bool>,
     iterator_pos: Cell<usize>,
 }
 
@@ -112,13 +116,27 @@ impl XPathResult {
         XPathResult {
             reflector_: Reflector::new(),
             window: Dom::from_ref(window),
+            version: Cell::new(
+                window
+                    .Document()
+                    .upcast::<Node>()
+                    .inclusive_descendants_version(),
+            ),
             result_type: Cell::new(inferred_result_type),
-            iterator_invalid: Cell::new(false),
             iterator_pos: Cell::new(0),
             value: RefCell::new(value),
         }
     }
 
+    fn document_changed_since_creation(&self) -> bool {
+        let current_document_version = self
+            .window
+            .Document()
+            .upcast::<Node>()
+            .inclusive_descendants_version();
+        current_document_version != self.version.get()
+    }
+
     /// NB: Blindly trusts `result_type` and constructs an object regardless of the contents
     /// of `value`. The exception is `XPathResultType::Any`, for which we look at the value
     /// to determine the type.
@@ -140,7 +158,12 @@ impl XPathResult {
     pub(crate) fn reinitialize_with(&self, result_type: XPathResultType, value: XPathResultValue) {
         self.result_type.set(result_type);
         *self.value.borrow_mut() = value;
-        self.iterator_invalid.set(false);
+        self.version.set(
+            self.window
+                .Document()
+                .upcast::<Node>()
+                .inclusive_descendants_version(),
+        );
         self.iterator_pos.set(0);
     }
 }
@@ -183,48 +206,41 @@ impl XPathResultMethods<crate::DomTypeHolder> for XPathResult {
 
     /// <https://dom.spec.whatwg.org/#dom-xpathresult-iteratenext>
     fn IterateNext(&self) -> Fallible<Option<DomRoot<Node>>> {
-        // TODO(vlindhol): actually set `iterator_invalid` somewhere
+        if !matches!(
-        if self.iterator_invalid.get() {
+            self.result_type.get(),
-            return Err(Error::Range(
+            XPathResultType::OrderedNodeIterator | XPathResultType::UnorderedNodeIterator
-                "Invalidated iterator for XPathResult, the DOM has mutated.".to_string(),
+        ) {
-            ));
+            return Err(Error::Type("Result is not an iterator".into()));
         }
 
-        match (&*self.value.borrow(), self.result_type.get()) {
+        if self.document_changed_since_creation() {
-            (
+            return Err(Error::InvalidState);
-                XPathResultValue::Nodeset(nodes),
+        }
-                XPathResultType::OrderedNodeIterator | XPathResultType::UnorderedNodeIterator,
+
-            ) => {
+        let XPathResultValue::Nodeset(nodes) = &*self.value.borrow() else {
-                let pos = self.iterator_pos.get();
+            return Err(Error::Type(
-                if pos >= nodes.len() {
-                    Ok(None)
-                } else {
-                    let node = nodes[pos].clone();
-                    self.iterator_pos.set(pos + 1);
-                    Ok(Some(node))
-                }
-            },
-            _ => Err(Error::Type(
                 "Can't iterate on XPathResult that is not a node-set".to_string(),
-            )),
+            ));
+        };
+
+        let position = self.iterator_pos.get();
+        if position >= nodes.len() {
+            Ok(None)
+        } else {
+            let node = nodes[position].clone();
+            self.iterator_pos.set(position + 1);
+            Ok(Some(node))
         }
     }
 
     /// <https://dom.spec.whatwg.org/#dom-xpathresult-invaliditeratorstate>
-    fn GetInvalidIteratorState(&self) -> Fallible<bool> {
+    fn InvalidIteratorState(&self) -> bool {
-        let is_iterator_invalid = self.iterator_invalid.get();
+        let is_iterable = matches!(
-        if is_iterator_invalid ||
+            self.result_type.get(),
-            matches!(
+            XPathResultType::OrderedNodeIterator | XPathResultType::UnorderedNodeIterator
-                self.result_type.get(),
+        );
-                XPathResultType::OrderedNodeIterator | XPathResultType::UnorderedNodeIterator
+
-            )
+        is_iterable && self.document_changed_since_creation()
-        {
-            Ok(is_iterator_invalid)
-        } else {
-            Err(Error::Type(
-                "Can't iterate on XPathResult that is not a node-set".to_string(),
-            ))
-        }
     }
 
     /// <https://dom.spec.whatwg.org/#dom-xpathresult-snapshotlength>

components/script_bindings/webidls/XPathResult.webidl

@@ -21,7 +21,7 @@ interface XPathResult {
   [Throws] readonly attribute DOMString stringValue;
   [Throws] readonly attribute boolean booleanValue;
   [Throws] readonly attribute Node? singleNodeValue;
-  [Throws] readonly attribute boolean invalidIteratorState;
+  readonly attribute boolean invalidIteratorState;
   [Throws] readonly attribute unsigned long snapshotLength;
 
   [Throws] Node? iterateNext();

tests/wpt/meta/MANIFEST.json

@@ -643317,6 +643317,13 @@
       {}
      ]
     ],
+    "result-iterateNext.html": [
+     "8c17f7ebc1c4292a0d8eb32b3d3503ca1e693eb9",
+     [
+      null,
+      {}
+     ]
+    ],
     "text-html-attributes.html": [
      "2157dcd2d68c5701b47ba907f7b1c3b2176f9239",
      [

tests/wpt/tests/domxpath/result-iterateNext.html

@@ -0,0 +1,100 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Invalidation of iterators over XPath results</title>
+  <link rel="author" title="Simon Wülker" href="mailto:simon.wuelker@arcor.de">
+  <link rel="help" href="https://www.w3.org/TR/DOM-Level-3-XPath/xpath.html#XPathResult-iterateNext">
+  <script src="/resources/testharness.js"></script>
+  <script src="/resources/testharnessreport.js"></script>
+</head>
+<body>
+  <ul id="list">
+      <li id="first-child"></li>
+      <li id="second-child"></li>
+  </ul>
+  <script>
+    function make_xpath_query(result_type) {
+      return document.evaluate(
+        "//li",
+        document,
+        null,
+        result_type,
+        null
+      );
+    }
+
+    function invalidate_iterator(test) {
+      let new_element = document.createElement("li");
+      document.getElementById("list").appendChild(new_element);
+      test.add_cleanup(() => {
+        new_element.remove();
+      })
+    }
+
+    test((t) => {
+      let iterator = make_xpath_query(XPathResult.ORDERED_NODE_ITERATOR_TYPE);
+
+      assert_equals(iterator.iterateNext(), document.getElementById("first-child"));
+      assert_equals(iterator.iterateNext(), document.getElementById("second-child"));
+      assert_equals(iterator.iterateNext(), null);
+      assert_false(iterator.invalidIteratorState);
+    }, "Using an ordered iterator without modifying the dom should yield the expected elements in correct order without errors.");
+
+    test((t) => {
+      let iterator = make_xpath_query(XPathResult.UNORDERED_NODE_ITERATOR_TYPE);
+
+      assert_not_equals(iterator.iterateNext(), null);
+      assert_not_equals(iterator.iterateNext(), null);
+      assert_equals(iterator.iterateNext(), null);
+      assert_false(iterator.invalidIteratorState);
+    }, "Using an unordered iterator without modifying the dom should yield the correct number of elements without errors.");
+
+    test((t) => {
+      let non_iterator_query = make_xpath_query(XPathResult.BOOLEAN_TYPE);
+
+      assert_false(non_iterator_query.invalidIteratorState);
+      invalidate_iterator(t);
+      assert_false(non_iterator_query.invalidIteratorState);
+    }, "invalidIteratorState should be false for non-iterable results.");
+
+    test((t) => {
+      let non_iterator_query = make_xpath_query(XPathResult.BOOLEAN_TYPE);
+
+      assert_throws_js(TypeError, () => non_iterator_query.iterateNext());
+    }, "Calling iterateNext on a non-iterable XPathResult should throw a TypeError.");
+
+    test((t) => {
+      let non_iterator_query = make_xpath_query(XPathResult.BOOLEAN_TYPE);
+      invalidate_iterator(t);
+      assert_throws_js(TypeError, () => non_iterator_query.iterateNext());
+    }, "Calling iterateNext on a non-iterable XPathResult after modifying the DOM should throw a TypeError.");
+
+    test((t) => {
+        let iterator = make_xpath_query(XPathResult.ORDERED_NODE_ITERATOR_TYPE);
+
+        iterator.iterateNext();
+
+        invalidate_iterator(t);
+
+        assert_throws_dom(
+          "InvalidStateError",
+          () => iterator.iterateNext(),
+        );
+    }, "Calling iterateNext after having modified the DOM should throw an exception.");
+
+    test((t) => {
+        let iterator = make_xpath_query(XPathResult.ORDERED_NODE_ITERATOR_TYPE);
+
+        iterator.iterateNext();
+        iterator.iterateNext();
+
+        invalidate_iterator(t);
+
+        assert_throws_dom(
+          "InvalidStateError",
+          () => iterator.iterateNext(),
+        );
+    }, "Calling iterateNext after having modified the DOM should throw an exception even if the iterator is exhausted.");
+  </script>
+</body>
+</html>