fix(native-dom): clear node_id_mapping when nodes are removed

[nikhiljha]

NOTE: I wasn't able to write a minimal reproducer, but this was the problem
I observed in LLDB. Applying this change fixed the panic!() in my app. I
don't really understand if there's supposed to be a mechanism to avoid this race.

When remove_node or replace_node_with removes a node from the DOM, the
corresponding entry in node_id_mapping was not being cleared.

This is normally fine if set_id_mapping is called, but if this races with
remove_node_if_unparented then the app will panic.

[nicoburns]

Hmm.... I think as-is this will leak nodes. Because we're relying on set_id_mapping to clean them up when it replaces a mapping. This is intended to match the behaviour of the interpreter in dioxus-web

[nicoburns]

Hmm.... I think as-is this will leak nodes. Because we're relying on set_id_mapping to clean them up when it replaces a mapping. This is intended to match the behaviour of the interpreter in dioxus-web

[nikhiljha]

Thanks! Yeah that makes sense, I figured this wouldn't be the real solution. How do you feel about immediately gcing the nodes on delete (like this)?

I'm pretty sure this is still not the actual bug (when/if this moves to an actual gc) but it does hide it. Not ideal :(

I can spend more time thinking about what to do in a bit.

[nicoburns]

How do you feel about immediately gcing the nodes on delete (like this)?

It was a while ago now, but I'm pretty sure that's how I originally implemented this, but we ended up with the current solution because this was causing panics. IIRC the reason was that Dioxus Core expects the underlying DOM node to stick around for longer so that it can reuse those DOM nodes rather than creating new ones (as a performance optimisation).

I think some careful testing is warranted here. Including with example code that creates nodes, then deletes them, then recreates them (todomvc should work I think).

Also, do you have a stack trace for the panic you are trying to fix here?

[nikhiljha]

Yep, although I think the stack trace was pretty unhelpful when I was debugging this:

I'm happy to pull up any related state in LLDB (I can reproduce this like 30% reliably/nondeterministically with my entire app) if it helps.

thread 'main' panicked at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/blitz-dom-0.2.4/src/mutator.rs:438:44:
  invalid key

  stack backtrace:
     0: __rustc::rust_begin_unwind
        at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/std/src/panicking.rs:698:5
     1: core::panicking::panic_fmt
        at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/core/src/panicking.rs:80:14
     2: core::panicking::panic
        at /rustc/ded5c06cf21d2b93bffd5d884aa6e96934ee4234/library/core/src/panicking.rs:150:5
     3: <slab::Slab<T> as core::ops::index::IndexMut<usize>>::index_mut
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/slab-0.4.11/src/lib.rs:1275:18
     4: blitz_dom::mutator::DocumentMutator::add_children_to_parent
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/blitz-dom-0.2.4/src/mutator.rs:438:44
     5: blitz_dom::mutator::DocumentMutator::insert_nodes_before
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/blitz-dom-0.2.4/src/mutator.rs:410:14
     6: blitz_dom::mutator::DocumentMutator::replace_node_with
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/blitz-dom-0.2.4/src/mutator.rs:483:14
     7: <dioxus_native_dom::mutation_writer::MutationWriter as dioxus_core::mutations::WriteMutations>::replace_placeholder_with_nodes
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/dioxus-native-dom-0.7.2/src/mutation_writer.rs:157:19
     8: dioxus_core::diff::node::<impl dioxus_core::nodes::VNode>::load_placeholders
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/dioxus-core-0.7.2/src/diff/node.rs:776:24
     9: dioxus_core::diff::node::<impl dioxus_core::nodes::VNode>::create::{{closure}}
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/dioxus-core-0.7.2/src/diff/node.rs:623:34
    10: core::iter::adapters::map::map_fold::{{closure}}
        at ~/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/adapters/map.rs:88:28
    ...
    53: <dioxus_native_dom::dioxus_document::DioxusDocument as blitz_dom::document::Document>::poll
        at ~/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/dioxus-native-dom-0.7.2/src/dioxus_document.rs:197:19