Reclaim element id for all removed nodes

[gammahead]

element ids associated with nodes removed via fn remove_nested_dyn_nodes are never reclaimed. This causes references to them to hang around, leading to unbounded growth of detached DOM nodes for applications that dynamically add and remove nodes frequently

fixes #3760

[gammahead]

@ealmloff can you give this a quick look? It's small and I think you'll easily be able to tell me if it's a step in the wrong direction.

[ealmloff]

I think the None mutations variant is only used in suspense (for example here). It disables writing the mutations to the dom while the component is suspended. You could check if the bug is caused by suspense by removing any suspense components in your reproduction and disabling the root suspense component

[gammahead]

@ealmloff I'm not explicitly using suspense boundaries anywhere. The None mutations variant is also used in remove_nested_dyn_nodes which is where the problem arrises for me. My understanding is that, since this recurses through the entire subtree of nodes with None mutations, all children element ids fail to be reclaimed regardless of suspense context.

If the Option<&mut impl WriteMutations> was intended to communicate a suspense context when it's None, then perhaps we need an enum with 3 variants ?

[ealmloff]

@ealmloff I'm not explicitly using suspense boundaries anywhere. The None mutations variant is also used in remove_nested_dyn_nodes which is where the problem arrises for me. My understanding is that, since this recurses through the entire subtree of nodes with None mutations, all children element ids fail to be reclaimed regardless of suspense context.

If the Option<&mut impl WriteMutations> was intended to communicate a suspense context when it's None, then perhaps we need an enum with 3 variants ?

Sorry it took so long to get back to you. Yes, I think that is correct there are three cases for remove_dynamic_node:

1. The nodes were suspended, so they were never actually allocated ids and we don't need to reclaim them
2. The nodes were created nested with ids. They do exist in the node slab and their ids need to be removed, but the dom nodes were already removed when the root elements were removed from the dom
3. The nodes are not suspended and are root nodes. They do exist in the node slab and we need to generate mutations to remove them from the dom.

For reclaim_roots, it could be called with None outside of suspense if it is called from remove_dynamic_node in case 2

We could add a separate "remove_from_dom" bool that is true just in case 3 or switch to a three variant enum

[gammahead]

@ealmloff picking this back up since a couple more people ran into #3760. You see a reason not to just always reclaim the ID, regardless of whether we're writing mutations?

[ealmloff]

@ealmloff picking this back up since a couple more people ran into #3760. You see a reason not to just always reclaim the ID, regardless of whether we're writing mutations?

Yes, I would expect reclaiming the element that was never created in case 1 above would cause a tracing error

[jkelleyrtp]

I spent a few hours digging into this today and then spun out a branch that properly wires the "should_write" flag into the callsites:

#4352

Suspense is a less-used feature and I'd rather have regular vdom code not leak than the extra logging in the suspense case. Seems like playwright and tests pass just fine even with suspense.

What I think is happening is that our call to remove_nested_dyn_nodes passes a None for to (the mutation writer) which then hits this case. In some cases those nodes should be reclaimed, but because we assume they will be somewhere else, we're missing the relevant case.