diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 01db8af..8ca2475 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -469,12 +469,20 @@ impl DomTree { inner.id_index.remove(&id_str); } + // Only free slots that are currently live. Freeing an out-of-range id + // would panic on direct indexing, and freeing an already-freed slot + // would push it onto the free list a second time — later handing the + // same NodeId to two live nodes (aliasing). for desc_id in descendants { - inner.nodes[desc_id.index()] = None; - inner.free_list.push(desc_id.0); + if matches!(inner.nodes.get(desc_id.index()), Some(Some(_))) { + inner.nodes[desc_id.index()] = None; + inner.free_list.push(desc_id.0); + } + } + if matches!(inner.nodes.get(node_id.index()), Some(Some(_))) { + inner.nodes[node_id.index()] = None; + inner.free_list.push(node_id.0); } - inner.nodes[node_id.index()] = None; - inner.free_list.push(node_id.0); } pub fn children(&self, node_id: NodeId) -> Vec { @@ -932,6 +940,30 @@ mod tests { assert!(node.is_document()); } + #[test] + fn remove_out_of_range_id_is_a_noop() { + let tree = DomTree::new(); + // Direct indexing into `nodes` panicked out-of-bounds for an id past the + // end of the slot vector; it must be a no-op instead. + tree.remove(NodeId::new(9999)); + assert_eq!(tree.len(), 1); + } + + #[test] + fn remove_twice_does_not_alias_slots() { + let tree = DomTree::new(); + let doc = tree.document(); + let a = tree.new_node(NodeData::Text { contents: "a".into() }); + tree.append_child(doc, a); + tree.remove(a); + // Removing the already-freed node again must not push its slot onto the + // free list a second time, or two later allocations alias one slot. + tree.remove(a); + let x = tree.new_node(NodeData::Text { contents: "x".into() }); + let y = tree.new_node(NodeData::Text { contents: "y".into() }); + assert_ne!(x, y, "double-free aliased two live nodes onto the same slot"); + } + #[test] fn test_append_child() { let tree = DomTree::new();