Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reclaim element id for all removed nodes #3782

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gammahead
Copy link

@gammahead gammahead commented Feb 21, 2025

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 gammahead requested a review from a team as a code owner February 21, 2025 04:18
@gammahead gammahead changed the title reclaim el id for nested dyn nodes Reclaim element id for all removed nodes Feb 24, 2025
@gammahead
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

gammahead commented Feb 25, 2025

@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
Copy link
Member

ealmloff commented Mar 3, 2025

@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

@ealmloff ealmloff added bug Something isn't working core relating to the core implementation of the virtualdom labels Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core relating to the core implementation of the virtualdom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak/Detached DOM nodes when node is removed
2 participants