Use-after-free with create_effect

[ChangeCaps]

Describe the bug

/// Internal implementation for `create_effect`. Use dynamic dispatch to reduce code-bloat.
fn _create_effect<'a>(cx: Scope<'a>, mut f: Box<(dyn FnMut() + 'a)>) {
    // SAFETY: We do not access the scope in the Drop implementation for EffectState. <<<< HERE
    let effect = unsafe { create_ref_unsafe(cx, RefCell::new(None::<EffectState<'a>>)) };

This is not always true, the FnMut() might own some type that accesses the Scope in it's drop implementation, which could lead to use-after-free.

Environment

• Sycamore: master

[lukechu10]

Yikes. I'm not sure if it's possible to fix this without severely limiting the power of the reactivity system. I'm starting to think that perhaps the whole lifetime gymnastics was a mistake to begin with and that the idea is fundamentally inconsistent with how lifetimes work in Rust.

Fixing this will unfortunately mean we'll probably have to make some pretty invasive changes to reactivity. I think the options are as follows:

1. Go back to the old (pre v0.8) system. This is definitely a no-go for me since the improvements in ergonomics is way to big to simply go back to .clone() hell.
2. Try to fix our current implementation. One way which I can imagine this could be fixed is to have the internal arena hand out a wrapper Ref<T> instead of simply &T. This way, we can perform a runtime check every time Ref<T> is derefed to check if the backing Scope has been Droped yet. However, for this to work, we'll need some kind of Rc system which points back to the Scope so that we can access it inside Ref<T>. Perhaps there is a better way of doing this but I don't see it.
3. Go the Leptos route and get rid of lifetimes completely. Instead, make everything index based and therefore Copyable. I'm actually quite inclined to proceed with this path although I don't wish Sycamore to become a Leptos clone. Among other benefits, this would also make it easier to properly implement change propagation across the reactivity graph unlike what we're doing now which is simply a crude event handler system.

As for whether 3. will make Sycamore too similar to Leptos, I don't think it's too big of an issue because whereas Leptos seems to be more web-focused right now, I intend Sycamore to be a more general UI framework that supports not just web but also potentially native and TUI in the future.

[wingertge]

I think option 3 is by far the most preferable. I don't think copying leptos is a problem in the first place, and sycamore's much more pleasant to work with view syntax (in my opinion) still makes it stand out.

I've also spent a lot of time writing library code for sycamore and while the lifetimes are a nice touch in theory they become an ergonomic nightmare once you start passing around signals or storing them in structs.

[gbj]

Just to put in my two cents re: Option 3: I think if you want to go this route, you should consider whether using leptos_reactive itself for the reactive layer would make sense, and if it needs to be refactored at all I'm very happy to work on that with you. (And if we want to rename it to something like reactive or whatever crate name is available so it's not a weird branding thing that's fine.) I recently rewrote it so that it is a bit more modern than it was, so solves the diamond problem etc. (I've been doing some more exploratory work recently to see whether I can drop explicit Scope types entirely in favor of a system like's Solid's reactive ownership.) Sycamore and Leptos could become two separate frameworks sharing the same reactive core, with plenty that's distinct about them, even beyond the superficial parts like the different view macros.

This would unlock a lot of possibilities, like the two communities collaborating on a reactive-web crate that does reactive wrappers for Web APIs, etc., and could be shared across the two because they're both using the same reactive types.

(We did the same thing so that Dioxus and Leptos could share the same server function code. One of the things I love above the Rust community vs. JS is the tendency to coalesce around a smaller number of shared, primitive crates.)

It should go without saying given the MIT license and nature of open source but if you want to go Option 3 without collaborating so directly, of course feel free to use whatever parts of Leptos code make sense to use.

While I don't think there's really anything left in Leptos at this point that has a direct lineage from Sycamore, Sycamore was a huge source for me to learn how to implement the SolidJS approach in Rust originally, and I have a huge amount of respect for you and your work on it!

[lukechu10]

Thanks for the kind words @gbj! Your work on Leptos is awesome and it's great to see Rust WASM becoming more and more widespread!

I think for now I would rather keep the two implementations separate just for flexibility's sake. This way, we can iterate quickly on both sides without trying to coordinate changes between the two frameworks.

I spent quite a bit of time looking at the Leptos implementation of reactivity and the index solution is quite clever and clean! It definitely looks nothing like the tangled lifetime mess I've gotten myself into here.

The lineage of these Rust UI frameworks is definitely very interesting. Originally, I created Sycamore based on Yew's new functional components (back when struct components were still the default), combined with the reactive system from SolidJS. However, this brought over the all the clones as well from Yew. When Dioxus appeared, they solved clone-hell by introducing cx: Scope and it seems like this approach has spread out pretty quickly. I'm really excited to see where everything goes from here!

[lukechu10]

Also on the topic of removing cx completely, I actually tried doing that in Sycamore before the current lifetime solution: https://github.com/sycamore-rs/sycamore/pull/178

The idea was actually surprisingly similar to the index approach although I did not think of using a global Runtime struct to keep track of everything. The result was an even bigger mess and a never-ending list of bugs to fix.

However, with the current architecture that the Leptos reactive system uses, I believe this could actually work pretty well. Hopefully you'll have more luck than I had if you do decide to go down this route!

[arctic-hen7]

I'll just add that I believe gloo has been wanting a low-level reactivity library for a while now. I think they wanted a vDOM system from memory (not sure if they ever built one), but a Sycamore-Leptos collaboration could even reach out to the gloo maintainers and make this accessible to everyone at that level. Although @lukechu10's point on flexibility is important.

[ChangeCaps]

Is it not possible to ensure that all effects are dropped before everything in the arena?

[lukechu10]

Is it not possible to ensure that all effects are dropped before everything in the arena?

Yes I that's a possible solution as well which I completely missed, although the more I think about it, the more I'm quite inclined to move to an index based approach instead.

[lukechu10]

Fixed by #612 and #626