fix(dom): make serialize, textContent, and import iterative (#367)

serialize_node/serialize_children, collect_text_inner (textContent), and
import_node_from recursed once per DOM nesting level with no depth cap, so a
deeply nested tree overflowed the thread stack and aborted the process. A
stack overflow is a hard abort that op_dom's catch_unwind cannot recover, so
one hostile page could take down the worker. descendants() was already made
iterative and capped for exactly this reason; these paths were missed.

Rewrite all three as iterative walks over an explicit heap stack and add the
same defense-in-depth cap so a cyclic graph stops instead of spinning.
Serialized output and traversal order are unchanged. Adds tests that build a
100k-deep tree and exercise each path.

Closes #364.

crates/obscura-dom/src/serialize.rs

@@ -1,103 +1,151 @@
 use crate::tree::{DomTree, NodeData, NodeId};
 
+// A unit of pending serialization work. Held on an explicit heap stack instead
+// of the call stack so a deeply nested tree cannot overflow the thread stack and
+// abort the process (a stack overflow is a hard abort that `op_dom`'s
+// catch_unwind cannot recover). `descendants()` is iterative + capped for the
+// same reason; the serializer must be too.
+enum SerializeWork {
+    // Serialize this node; `include_self` mirrors the old recursive param.
+    Node(NodeId, bool),
+    // Emit a previously-opened element's closing tag, after its children.
+    CloseTag(String),
+}
+
 impl DomTree {
     pub fn outer_html(&self, node_id: NodeId) -> String {
         let mut buf = String::new();
-        self.serialize_node(node_id, true, &mut buf);
+        self.serialize_worklist(vec![SerializeWork::Node(node_id, true)], &mut buf);
         buf
     }
 
     pub fn inner_html(&self, node_id: NodeId) -> String {
         let mut buf = String::new();
-        self.serialize_children(node_id, &mut buf);
+        self.serialize_worklist(self.child_work(node_id), &mut buf);
         buf
     }
 
-    fn serialize_node(&self, node_id: NodeId, include_self: bool, buf: &mut String) {
-        let node = match self.get_node(node_id) {
-            Some(n) => n,
-            None => return,
-        };
-
-        match &node.data {
-            NodeData::Document => {
-                self.serialize_children(node_id, buf);
-            }
-            NodeData::Doctype { name, .. } => {
-                buf.push_str("<!DOCTYPE ");
-                buf.push_str(name);
-                buf.push('>');
-            }
-            NodeData::Element { name, attrs, .. } => {
-                let tag = name.local.as_ref();
-                if include_self {
-                    buf.push('<');
-                    buf.push_str(tag);
-                    for attr in attrs {
-                        buf.push(' ');
-                        let attr_name = attr.name.local.as_ref();
-                        buf.push_str(attr_name);
-                        buf.push_str("=\"");
-                        escape_attr(&attr.value, buf);
-                        buf.push('"');
-                    }
-                    buf.push('>');
-                }
-
-                if !is_void_element(tag) {
-                    self.serialize_children(node_id, buf);
-                    if include_self {
-                        buf.push_str("</");
-                        buf.push_str(tag);
-                        buf.push('>');
-                    }
-                }
-            }
-            NodeData::Text { contents } => {
-                let parent_is_raw = node.parent
-                    .and_then(|pid| {
-                        self.with_node(pid, |p| {
-                            p.as_element()
-                                .map(|name| is_raw_text_element(name.local.as_ref()))
-                                .unwrap_or(false)
-                        })
-                    })
-                    .unwrap_or(false);
-
-                if parent_is_raw {
-                    buf.push_str(contents);
-                } else {
-                    escape_text(contents, buf);
-                }
-            }
-            NodeData::Comment { contents } => {
-                buf.push_str("<!--");
-                // The HTML parser can never produce a comment whose data contains
-                // "-->" (that ends the comment), but script can via
-                // document.createComment("a-->b"). Emitting it verbatim would close
-                // the comment early and let the trailing text escape into markup.
-                // Neutralize the terminator so the serialized output round-trips as
-                // a single comment.
-                if contents.contains("-->") {
-                    buf.push_str(&contents.replace("-->", "--&gt;"));
-                } else {
-                    buf.push_str(contents);
-                }
-                buf.push_str("-->");
-            }
-            NodeData::ProcessingInstruction { target, data } => {
-                buf.push_str("<?");
-                buf.push_str(target);
-                buf.push(' ');
-                buf.push_str(data);
-                buf.push('>');
-            }
-        }
+    // Children as work items in document order (top of the LIFO stack first).
+    fn child_work(&self, node_id: NodeId) -> Vec<SerializeWork> {
+        self.children(node_id)
+            .into_iter()
+            .rev()
+            .map(|c| SerializeWork::Node(c, true))
+            .collect()
     }
 
-    fn serialize_children(&self, node_id: NodeId, buf: &mut String) {
-        for child_id in self.children(node_id) {
-            self.serialize_node(child_id, true, buf);
+    fn serialize_worklist(&self, mut stack: Vec<SerializeWork>, buf: &mut String) {
+        // Defense in depth, mirroring descendants(): a well-formed subtree emits
+        // at most one Node plus one CloseTag per node, so 2*nodes.len() work
+        // items bound a valid walk. Exceeding it means the graph is cyclic (the
+        // append_child / insert_before guards prevent that); stop rather than
+        // spin forever. On a valid tree this bound is never reached.
+        let max_steps = self
+            .node_slot_count()
+            .saturating_mul(2)
+            .saturating_add(16);
+        let mut steps = 0usize;
+
+        while let Some(work) = stack.pop() {
+            steps += 1;
+            if steps > max_steps {
+                eprintln!("obscura: serialize worklist cap hit - tree has a cycle");
+                break;
+            }
+
+            let (node_id, include_self) = match work {
+                SerializeWork::CloseTag(tag) => {
+                    buf.push_str("</");
+                    buf.push_str(&tag);
+                    buf.push('>');
+                    continue;
+                }
+                SerializeWork::Node(node_id, include_self) => (node_id, include_self),
+            };
+
+            let node = match self.get_node(node_id) {
+                Some(n) => n,
+                None => continue,
+            };
+
+            match &node.data {
+                NodeData::Document => {
+                    for w in self.child_work(node_id) {
+                        stack.push(w);
+                    }
+                }
+                NodeData::Doctype { name, .. } => {
+                    buf.push_str("<!DOCTYPE ");
+                    buf.push_str(name);
+                    buf.push('>');
+                }
+                NodeData::Element { name, attrs, .. } => {
+                    let tag = name.local.as_ref();
+                    if include_self {
+                        buf.push('<');
+                        buf.push_str(tag);
+                        for attr in attrs {
+                            buf.push(' ');
+                            let attr_name = attr.name.local.as_ref();
+                            buf.push_str(attr_name);
+                            buf.push_str("=\"");
+                            escape_attr(&attr.value, buf);
+                            buf.push('"');
+                        }
+                        buf.push('>');
+                    }
+
+                    if !is_void_element(tag) {
+                        // Push the closing tag first so it pops after all the
+                        // children we push next.
+                        if include_self {
+                            stack.push(SerializeWork::CloseTag(tag.to_string()));
+                        }
+                        for w in self.child_work(node_id) {
+                            stack.push(w);
+                        }
+                    }
+                }
+                NodeData::Text { contents } => {
+                    let parent_is_raw = node.parent
+                        .and_then(|pid| {
+                            self.with_node(pid, |p| {
+                                p.as_element()
+                                    .map(|name| is_raw_text_element(name.local.as_ref()))
+                                    .unwrap_or(false)
+                            })
+                        })
+                        .unwrap_or(false);
+
+                    if parent_is_raw {
+                        buf.push_str(contents);
+                    } else {
+                        escape_text(contents, buf);
+                    }
+                }
+                NodeData::Comment { contents } => {
+                    buf.push_str("<!--");
+                    // The HTML parser can never produce a comment whose data contains
+                    // "-->" (that ends the comment), but script can via
+                    // document.createComment("a-->b"). Emitting it verbatim would close
+                    // the comment early and let the trailing text escape into markup.
+                    // Neutralize the terminator so the serialized output round-trips as
+                    // a single comment.
+                    if contents.contains("-->") {
+                        buf.push_str(&contents.replace("-->", "--&gt;"));
+                    } else {
+                        buf.push_str(contents);
+                    }
+                    buf.push_str("-->");
+                }
+                NodeData::ProcessingInstruction { target, data } => {
+                    buf.push_str("<?");
+                    buf.push_str(target);
+                    buf.push(' ');
+                    buf.push_str(data);
+                    buf.push('>');
+                }
+            }
         }
     }
 }

crates/obscura-dom/src/tree.rs

@@ -659,20 +659,27 @@ impl DomTree {
     }
 
     fn import_node_from(&self, parent_id: NodeId, source: &DomTree, source_node_id: NodeId) {
-        let node_data = {
-            let source_inner = source.inner.borrow();
-            match source_inner.nodes.get(source_node_id.index()) {
-                Some(Some(node)) => node.data.clone(),
-                _ => return,
+        // Iterative DFS with an explicit (dest_parent, source_node) stack so a
+        // deeply nested source tree cannot overflow the thread stack and abort
+        // the process. Children are pushed in reverse so they are appended in
+        // document order (append_child always appends to the end, so each level
+        // keeps the source ordering).
+        let mut stack = vec![(parent_id, source_node_id)];
+        while let Some((dest_parent, src_id)) = stack.pop() {
+            let node_data = {
+                let source_inner = source.inner.borrow();
+                match source_inner.nodes.get(src_id.index()) {
+                    Some(Some(node)) => node.data.clone(),
+                    _ => continue,
+                }
+            };
+
+            let new_id = self.new_node(node_data);
+            self.append_child(dest_parent, new_id);
+
+            for child_id in source.children(src_id).into_iter().rev() {
+                stack.push((new_id, child_id));
             }
-        };
-
-        let new_id = self.new_node(node_data);
-        self.append_child(parent_id, new_id);
-
-        let children = source.children(source_node_id);
-        for child_id in children {
-            self.import_node_from(new_id, source, child_id);
         }
     }
 
@@ -680,6 +687,14 @@ impl DomTree {
         self.inner.borrow().nodes.iter().filter(|n| n.is_some()).count()
     }
 
+    // Number of node slots (live plus freed), i.e. the same upper bound
+    // descendants() uses to cap a tree walk. A well-formed subtree has at most
+    // this many nodes, so it is a safe ceiling for iterative walkers that need a
+    // cycle backstop.
+    pub(crate) fn node_slot_count(&self) -> usize {
+        self.inner.borrow().nodes.len()
+    }
+
     pub fn is_empty(&self) -> bool {
         self.len() <= 1
     }
@@ -696,7 +711,28 @@ impl DomTree {
 }
 
 fn collect_text_inner(inner: &DomTreeInner, node_id: NodeId, buf: &mut String) {
-    if let Some(Some(node)) = inner.nodes.get(node_id.index()) {
+    // Iterative pre-order walk on an explicit heap stack. The recursive form
+    // overflowed the thread stack and aborted the process on deeply nested
+    // trees; descendants() is iterative + capped for the same reason. A valid
+    // subtree visits at most nodes.len() nodes, so exceeding that means the
+    // graph is cyclic (prevented by the append_child / insert_before guards);
+    // stop rather than spin forever.
+    let max_steps = inner.nodes.len().saturating_add(16);
+    let mut steps = 0usize;
+    let mut stack = vec![node_id];
+
+    while let Some(id) = stack.pop() {
+        steps += 1;
+        if steps > max_steps {
+            eprintln!("obscura: collect_text_inner cap hit - tree has a cycle");
+            break;
+        }
+
+        let node = match inner.nodes.get(id.index()) {
+            Some(Some(n)) => n,
+            _ => continue,
+        };
+
         match &node.data {
             NodeData::Text { contents } => buf.push_str(contents),
             // Comment and ProcessingInstruction are intentionally NOT
@@ -704,13 +740,23 @@ fn collect_text_inner(inner: &DomTreeInner, node_id: NodeId, buf: &mut String) {
             // on an Element only includes Text descendants. Direct
             // textContent on a Comment/PI is handled by the caller.
             _ => {
+                // Collect children, then push them in reverse so they pop in
+                // document order.
+                let mut kids = Vec::new();
                 let mut child = node.first_child;
                 while let Some(child_id) = child {
-                    collect_text_inner(inner, child_id, buf);
+                    kids.push(child_id);
+                    if kids.len() > inner.nodes.len() {
+                        eprintln!("obscura: collect_text_inner sibling cap hit - cycle");
+                        break;
+                    }
                     child = inner.nodes.get(child_id.index())
                         .and_then(|n| n.as_ref())
                         .and_then(|n| n.next_sibling);
                 }
+                for child_id in kids.into_iter().rev() {
+                    stack.push(child_id);
+                }
             }
         }
     }
@@ -933,4 +979,67 @@ mod tests {
         tree.remove(div);
         assert_eq!(tree.len(), 1);
     }
+
+    // Builds a chain of `depth` nested <div> elements under the document and
+    // returns (outermost_div, innermost_div). Depth this large overflows a
+    // recursive tree walk and aborts the process, so it guards the iterative
+    // serialize / text_content / import paths against regressing to recursion.
+    fn build_deep_chain(tree: &DomTree, depth: usize) -> (NodeId, NodeId) {
+        let mk_div = || {
+            tree.new_node(NodeData::Element {
+                name: QualName::new(None, ns!(html), local_name!("div")),
+                attrs: vec![],
+                template_contents: None,
+                mathml_annotation_xml_integration_point: false,
+            })
+        };
+        let root = mk_div();
+        tree.append_child(tree.document(), root);
+        let mut cur = root;
+        for _ in 1..depth {
+            let next = mk_div();
+            tree.append_child(cur, next);
+            cur = next;
+        }
+        (root, cur)
+    }
+
+    #[test]
+    fn test_outer_html_deeply_nested_does_not_overflow() {
+        let tree = DomTree::new();
+        let (root, leaf) = build_deep_chain(&tree, 100_000);
+        let marker = tree.new_node(NodeData::Text {
+            contents: "leaf".into(),
+        });
+        tree.append_child(leaf, marker);
+
+        let html = tree.outer_html(root);
+        assert!(html.starts_with("<div>"));
+        assert!(html.contains("leaf"));
+        assert!(html.ends_with("</div>"));
+    }
+
+    #[test]
+    fn test_text_content_deeply_nested_does_not_overflow() {
+        let tree = DomTree::new();
+        let (root, leaf) = build_deep_chain(&tree, 100_000);
+        let marker = tree.new_node(NodeData::Text {
+            contents: "deep".into(),
+        });
+        tree.append_child(leaf, marker);
+
+        assert_eq!(tree.text_content(root), "deep");
+    }
+
+    #[test]
+    fn test_import_deeply_nested_does_not_overflow() {
+        let source = DomTree::new();
+        build_deep_chain(&source, 100_000);
+
+        let dest = DomTree::new();
+        let dest_doc = dest.document();
+        dest.import_children_from(dest_doc, &source, source.document());
+
+        assert!(dest.len() >= 100_000);
+    }
 }