Drop nodes removed by Dioxus in dioxus-native-dom

[nicoburns]

Summary

dioxus-native-dom leaked every node that Dioxus removed: BlitzBackend::remove and replace called DocumentMutator::remove_node, which only detaches the node from its parent. The node (and its subtree) stayed in the document's node storage for the lifetime of the document.

 fn replace(&mut self, target: NodeId, replacements: &[NodeId]) {
-    self.docm.replace_node_with(target, replacements);
+    self.docm.insert_nodes_before(target, replacements);
+    self.docm.remove_and_drop_node(target);
 }

 fn remove(&mut self, node: NodeId) {
-    self.docm.remove_node(node);
+    self.docm.remove_and_drop_node(node);
 }

Dropping is safe because core never refers to a renderer node again after removing it: remove is emitted only from reclaim_roots and remove_dynamic_node (packages/core/src/diff/node.rs), and both immediately reclaim the node's ElementId. Suspense keeps component state alive but recreates the DOM nodes on resolve (recreate_with_mount), and keyed moves are emitted as inserts.

User code can still hold a MountedData for a removed element. get_scroll_offset and get_scroll_size went through NodeHandle::node(), which panics if the node no longer exists; they now return node_not_exist_err() like get_client_rect, scroll_to and scroll.

Tests

Four tests in dioxus_document.rs, all of which fail without the change:

• removed_nodes_are_dropped: mounts/unmounts a 50 item keyed list repeatedly and asserts the node count is stable.
• suspended_nodes_are_dropped_and_recreated: a child under a SuspenseBoundary suspends and resolves repeatedly, checking the rendered text at each step and a stable node count.
• todo_like_stress: 3000 pseudo-random add / remove / toggle / edit / filter / clear steps on a TodoMVC-shaped tree, asserting the node count after clearing is stable. This is the test that reproduces the invalid key panic an earlier attempt at this change hit on older Blitz (stale ElementId mappings aliasing reused slab slots; no longer possible with generational node ids).
• mounted_data_of_removed_element_reports_error: the MountedData getters and scroll methods return an error once the element is removed.

Also verified locally (not included): driving the TodoMVC example with real pointer/keyboard events through a copy of Blitz's test harness, where the node count per round went from growing by ~106 to flat.

The equivalent change for the Blitz copy of this crate is DioxusLabs/blitz#1185.

Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/e4941556041d45bdb2bca571ee99129c
Open in Devin Desktop: https://dioxus.staging.devinenterprise.com/desktop/session/e4941556041d45bdb2bca571ee99129c?variant=devin-insiders
Requested by: @nicoburns

[staging-devin-ai-integration]

I'll fix CI failures and address comments from users with write access that start with 'Devin'.

• Disable automatic comment, CI, and merge conflict monitoring

[LucaCappelletti94]

You should make get_scroll_offset and get_scroll_size return node_not_exist_err() when the node is gone, as get_client_rect, scroll_to and scroll already do.

At this time, when removed nodes are dropped, a MountedData kept from onmounted panics in NodeHandle::node() once its element unmounts, where main returns Ok.

I have added a red test for this regression.

     fn get_scroll_offset(&self) -> Pin<Box<dyn Future<Output = MountedResult<PixelsVector2D>>>> {
-        let scroll_offset = *self.node().scroll_offset();
+        let Some(scroll_offset) = self
+            .doc()
+            .get_node(self.node_id)
+            .map(|n| *n.scroll_offset())
+        else {
+            return self.node_not_exist_err();
+        };
         Box::pin(async move { Ok(PixelsVector2D::new(scroll_offset.x, scroll_offset.y)) })
     }
 
     fn get_scroll_size(&self) -> Pin<Box<dyn Future<Output = MountedResult<PixelsSize>>>> {
-        let node = self.node();
-        let scroll_width = node.final_layout().scroll_width() as f64;
-        let scroll_height = node.final_layout().scroll_height() as f64;
+        let Some(layout) = self.doc().get_node(self.node_id).map(|n| *n.final_layout()) else {
+            return self.node_not_exist_err();
+        };
+        let scroll_width = layout.scroll_width() as f64;
+        let scroll_height = layout.scroll_height() as f64;
         Box::pin(async move { Ok(PixelsSize::new(scroll_width, scroll_height)) })
     }

[LucaCappelletti94]

You would suggest to assert on the node count in todo_like_stress instead of printing it. As written it passes on main too (7928 nodes after the final clear, against 24 on this branch), so it does not catch the leak. The version below compares two cleared rounds after a warm-up round and fails on main with left: 5342, right: 7928.

Suggested change

	let mut next_id = 0;
	for _ in 0..3000 {
	let r = rng();
	ITEMS.with(|items| {
	let mut items = items.borrow_mut();
	let len = items.len();
	match r % 8 {
	0 | 1 => {
	items.push(Item {
	id: next_id,
	checked: false,
	editing: false,
	});
	next_id += 1;
	}
	2 if len > 0 => {
	items.remove((r >> 8) as usize % len);
	}
	3 if len > 0 => {
	let i = (r >> 8) as usize % len;
	items[i].checked ^= true;
	}
	4 if len > 0 => {
	let i = (r >> 8) as usize % len;
	items[i].editing ^= true;
	}
	5 => FILTER.set(((r >> 8) % 3) as u8),
	6 => items.retain(|i| !i.checked),
	7 if len > 12 => items.clear(),
	_ => {}
	}
	});
	doc.vdom.mark_dirty(ScopeId::APP);
	doc.poll(None);
	}
	ITEMS.with(|items| items.borrow_mut().clear());
	doc.vdom.mark_dirty(ScopeId::APP);
	doc.poll(None);
	eprintln!(
	"STRESS final node count {}",
	doc.inner.borrow().tree().len()
	);
	let mut next_id = 0;
	let mut counts = Vec::new();
	for step in 1..=3000 {
	let r = rng();
	ITEMS.with(|items| {
	let mut items = items.borrow_mut();
	let len = items.len();
	match r % 8 {
	0 | 1 => {
	items.push(Item {
	id: next_id,
	checked: false,
	editing: false,
	});
	next_id += 1;
	}
	2 if len > 0 => {
	items.remove((r >> 8) as usize % len);
	}
	3 if len > 0 => {
	let i = (r >> 8) as usize % len;
	items[i].checked ^= true;
	}
	4 if len > 0 => {
	let i = (r >> 8) as usize % len;
	items[i].editing ^= true;
	}
	5 => FILTER.set(((r >> 8) % 3) as u8),
	6 => items.retain(|i| !i.checked),
	7 if len > 12 => items.clear(),
	_ => {}
	}
	});
	doc.vdom.mark_dirty(ScopeId::APP);
	doc.poll(None);
	if step % 1000 == 0 {
	ITEMS.with(|items| items.borrow_mut().clear());
	doc.vdom.mark_dirty(ScopeId::APP);
	doc.poll(None);
	counts.push(node_count(&doc));
	}
	}
	// The first round creates the retained template prototype nodes.
	assert_eq!(counts[1], counts[2]);

[staging-devin-ai-integration]

Good catch, thanks. Applied in 38ec167: the test now clears every 1000 steps and asserts the second and third counts are equal (fails on main with left: 5342, right: 7928).

Written by Devin

[LucaCappelletti94]

Red test for the MountedData panic.

Suggested change

	}
	}
	#[test]
	fn mounted_data_of_removed_element_reports_error() {
	thread_local! {
	static SHOW: Cell<bool> = const { Cell::new(true) };
	static MOUNTED: RefCell<Option<Rc<MountedData>>> = const { RefCell::new(None) };
	}
	fn app() -> Element {
	rsx!(if SHOW.get() {
	div {
	onmounted: |e| MOUNTED.set(Some(e.data())),
	}
	})
	}
	let mut doc = DioxusDocument::new(VirtualDom::new(app), DocumentConfig::default());
	doc.initial_build();
	let mounted = MOUNTED.take().unwrap();
	SHOW.set(false);
	rerender(&mut doc);
	let mut cx = std::task::Context::from_waker(std::task::Waker::noop());
	let offset = std::pin::pin!(mounted.get_scroll_offset()).poll(&mut cx);
	assert!(matches!(offset, std::task::Poll::Ready(Err(_))));
	let size = std::pin::pin!(mounted.get_scroll_size()).poll(&mut cx);
	assert!(matches!(size, std::task::Poll::Ready(Err(_))));
	}

[staging-devin-ai-integration]

Reproduced the panic with this test, thanks. Fixed in 38ec167: get_scroll_offset and get_scroll_size now return node_not_exist_err() when the node is gone. The test is included (using root context instead of thread_local!, and also covering get_client_rect, scroll_to and scroll). The same fix is in the Blitz copy (DioxusLabs/blitz#1185).

Written by Devin