From f46fe24cb4557d670d2bae2a6ee72cac3525f49c Mon Sep 17 00:00:00 2001 From: Nathan West Date: Mon, 2 Feb 2026 20:33:20 +0000 Subject: [PATCH 1/2] Fix `Node` drop recursion `Node::drop` no longer clobbers nodes that have active external handles --- rcdom/lib.rs | 111 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 100 insertions(+), 11 deletions(-) diff --git a/rcdom/lib.rs b/rcdom/lib.rs index b7e38b8a..3c7c2c03 100644 --- a/rcdom/lib.rs +++ b/rcdom/lib.rs @@ -259,17 +259,25 @@ impl Node { impl Drop for Node { fn drop(&mut self) { - let mut nodes = mem::take(&mut *self.children.borrow_mut()); - while let Some(node) = nodes.pop() { - let children = mem::take(&mut *node.children.borrow_mut()); - nodes.extend(children.into_iter()); - if let NodeData::Element { - ref template_contents, - .. - } = node.data - { - if let Some(template_contents) = template_contents.borrow_mut().take() { - nodes.push(template_contents); + // Iteratively drop nodes, to prevent stack overflows, but take care + // to only clear children for doomed nodes. + + let mut droplist = mem::take(self.children.get_mut()); + + while let Some(node) = droplist.pop() { + // Take all of the nodes out of `node` that we can get _ownership_ + // of and add them to the droplist. Ownership ensures that there are + // no external live handles to that node. + if let Some(mut node) = Rc::into_inner(node) { + droplist.extend(mem::take(node.children.get_mut())); + if let NodeData::Element { + ref mut template_contents, + .. + } = node.data + { + if let Some(node) = template_contents.get_mut().take() { + droplist.push(node); + } } } } @@ -673,3 +681,84 @@ impl Serialize for SerializableHandle { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use html5ever::{parse_document, ParseOpts}; + use tendril::TendrilSink; + + /// Assert that a node is an element and that it has a certain local name + fn assert_node_name(node: &Node, expected: &str) { + match node.data { + NodeData::Element { ref name, .. } => assert_eq!(&name.local, expected), + _ => panic!("node wasn't an element"), + } + } + + /// Assert that a node is a text node and that it has certain contents + fn assert_node_text(node: &Node, expected: &str) { + match node.data { + NodeData::Text { ref contents } => assert_eq!(contents.borrow().as_ref(), expected), + _ => panic!("node isn't a text node"), + } + } + + // Ensuse that, when a node is dropped, its children that still have handles + // in circulation are not unlinked from each other + #[test] + fn drop_node_preserves_children() { + let doc = " + \ + \ + \ + \ +
\ + \ +
\ +
\ +

Title

\ +
\ +

text 1

\ +

text 2

\ +
\ +
\ + \ + \ + "; + + // Get just the `article` element and drop everything else. Ensure + // that the article element still has all its children. + let (article, body_weak) = { + let dom = + parse_document(RcDom::default(), ParseOpts::default()).one(StrTendril::from(doc)); + + let document = dom.document; + let html = document.children.borrow()[0].clone(); + let body = html.children.borrow()[1].clone(); + let article = body.children.borrow()[1].clone(); + let body_weak = Rc::downgrade(&body); + + assert_node_name(&article, "article"); + + (article, body_weak) + }; + + // Ensure that the body was dropped, as expected + assert!(body_weak.upgrade().is_none()); + + let children = article.children.borrow(); + assert_eq!(children.len(), 2); + + let h1 = &children[0]; + assert_node_name(h1, "h1"); + assert_node_text(&h1.children.borrow()[0], "Title"); + + let section = &children[1]; + assert_node_name(section, "section"); + assert_node_name(§ion.children.borrow()[0], "p"); + assert_node_text(§ion.children.borrow()[0].children.borrow()[0], "text 1"); + assert_node_name(§ion.children.borrow()[1], "p"); + assert_node_text(§ion.children.borrow()[1].children.borrow()[0], "text 2"); + } +} From 52fb5dba8df937cd52e6bdb7e1bd4d938a4b283d Mon Sep 17 00:00:00 2001 From: Nathan West Date: Wed, 4 Feb 2026 21:05:54 +0000 Subject: [PATCH 2/2] Simplify node drop implementation - Prevent root node from recursively dropping its template - Use simpler `list.extend(opt)` --- rcdom/lib.rs | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/rcdom/lib.rs b/rcdom/lib.rs index 3c7c2c03..3444aebf 100644 --- a/rcdom/lib.rs +++ b/rcdom/lib.rs @@ -257,28 +257,31 @@ impl Node { } } +/// Get all of the recursive nodes out of a node and add them to a droplist. +/// this helps prevent unbounded recursion during drop. +fn destructure_node(droplist: &mut Vec, node: &mut Node) { + droplist.extend(mem::take(node.children.get_mut())); + if let NodeData::Element { + ref mut template_contents, + .. + } = node.data + { + droplist.extend(template_contents.get_mut().take()); + } +} + impl Drop for Node { fn drop(&mut self) { // Iteratively drop nodes, to prevent stack overflows, but take care // to only clear children for doomed nodes. - - let mut droplist = mem::take(self.children.get_mut()); + let mut droplist = Vec::new(); + destructure_node(&mut droplist, self); while let Some(node) = droplist.pop() { - // Take all of the nodes out of `node` that we can get _ownership_ - // of and add them to the droplist. Ownership ensures that there are - // no external live handles to that node. + // Destructure nodes that we can get ownership of. Ownership ensures + // that there are no external live handles to that node. if let Some(mut node) = Rc::into_inner(node) { - droplist.extend(mem::take(node.children.get_mut())); - if let NodeData::Element { - ref mut template_contents, - .. - } = node.data - { - if let Some(node) = template_contents.get_mut().take() { - droplist.push(node); - } - } + destructure_node(&mut droplist, &mut node); } } }