Skip to content

Conversation

@joeleaver
Copy link
Contributor

Problem

When nodes are removed from the document via remove_node or remove_and_drop_node, several pieces of internal state can retain references to the removed node IDs:

  1. hover_node_id - If a node is removed while it's being hovered, hover_node_id still points to the deleted node ID
  2. snapshots - If a snapshot was created for a node (e.g., during hover state change or attribute modification), the snapshot persists after the node is removed
  3. layout_children - The parent's cached layout children list may still contain the removed node's ID

These stale references cause panics when later operations try to access the deleted nodes:

  • traversal.rs - node_layout_ancestors() panics with "invalid key" when accessing self.nodes[stale_id]
  • selector_parser.rs - Stylo's style invalidation code panics when processing snapshots for deleted nodes

When This Happens

This issue is triggered when:

  1. A node has hover state or a snapshot attached to it
  2. The node is removed from the document
  3. Before the next resolve() call, either:
    • The mouse moves (triggering set_hover_to which calls maybe_node_layout_ancestors with the stale hover_node_id)
    • resolve() is called (Stylo tries to process the stale snapshot)

This is most likely to occur in applications that:

  • Perform direct DOM manipulation (as opposed to virtual DOM diffing)
  • Remove nodes that are currently hovered or have recently had attributes modified
  • Have the incremental feature enabled for style invalidation

Solution

Clean up all internal references when nodes are removed. In process_removed_subtree:

  1. Clear hover state if the removed node was being hovered:
if doc.hover_node_id == Some(node_id) {
    doc.hover_node_id = None;
    doc.hover_node_is_text = false;
}
  1. Remove snapshots for removed nodes:
if node.has_snapshot {
    let opaque_id = style::dom::TNode::opaque(&&*node);
    doc.snapshots.remove(&opaque_id);
    node.has_snapshot = false;
}
  1. Clear parent's layout_children cache in removal functions:
*parent.layout_children.borrow_mut() = None;

Changes

  • mutator.rs:

    • remove_node() - Clear parent's layout_children cache
    • remove_and_drop_node() - Clear parent's layout_children cache
    • remove_and_drop_all_children() - Clear parent's layout_children cache
    • process_removed_subtree() - Clear hover state and remove snapshots for removed nodes
  • document.rs:

    • snapshot_node() - Use empty Vec instead of None for non-element node attrs (prevents panic in Stylo's get_attr())
    • Added unit tests for the fixes

Tests

Added three new tests:

  • removing_hovered_node_clears_hover_state - Verifies hover state is cleared when hovered node is removed
  • removing_node_clears_snapshot - Verifies snapshots are removed when node is removed
  • stale_layout_children_do_not_panic - Verifies traversal handles stale layout_children gracefully

All existing tests continue to pass.

joeleaver and others added 2 commits January 27, 2026 23:24
When nodes are removed, clean up all internal references:
- Clear hover_node_id if the removed node was being hovered
- Remove snapshots for removed nodes to prevent stale snapshot panics
- Clear parent's layout_children cache to prevent stale child ID panics

Also fix snapshot_node() to use empty Vec instead of None for
non-element node attrs, preventing panics in Stylo's get_attr().

Added tests:
- removing_hovered_node_clears_hover_state
- removing_node_clears_snapshot
- stale_layout_children_do_not_panic

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snapshot and hover_node_id changes make sense to me. active_node_id has the same issue as hover_node_id (although it's less likely to be triggered). So we should probably handle that too.

The layout_children does not make sense sense to me. The function that is described as panicking does not access layout_children (it does use layout_parent however). The test probably passes, but I think it probably passes on main too. What was your reasoning for making this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants