"closure invoked recursively or destroyed" in todomvc example

[mc1098]

Describe the bug
The "closure invoked recursively or destroyed already" bug we have all come to know and love in wasm.

I can fix this by changing the handle_submit closure (added for convenience):

sycamore/examples/todomvc/src/main.rs

Lines 266 to 279 in 215a015

	let handle_submit = move |event: Event| {
	let event: KeyboardEvent = event.unchecked_into();
	match event.key().as_str() {
	"Enter" => handle_blur(),
	"Escape" => {
	input_ref
	.get::<DomNode>()
	.unchecked_into::<HtmlInputElement>()
	.set_value(&title());
	editing.set(false);
	}
	_ => {}
	}
	};

If you call HtmlElement::blur on the input element from the NodeRef for both match arms then this will no longer
panic. This would suggest that changing the editing value causes the render and deallocates event handler for blur
but the blur event still seems to fire causing the exception?

I think although the above change fixes the error, I think its pretty subtle and that Sycamore should be more robust
against errors like this if possible, which is why I'm submitting this issue.

To Reproduce

1. Go to https://sycamore-rs.netlify.app/examples/todomvc/
2. Create an item
3. Double click the item to edit
4. Press either the Escape or Enter key

Expected behavior
Not to throw exception.
The logic remains correct so unless you have the console open you wouldn't notice.

Screenshots

Environment

• Sycamore: master

[mc1098]

Ok, so with some more investigation - I found another bug of a similar type (do you want a new issue for this?) and I think this has got me to the conclusion that because Sycamore doesn't remove event listeners from the elements before they go out of scope that this allows events fired at specific times to call listeners that no longer exist in memory.

I think a possible solution then could be to have something similar to gloo-events::EventListener but from a quick look the API doesn't seem to match Sycamore so might have to roll its own version?

[lukechu10]

I think a possible solution then could be to have something similar to gloo-events::EventListener but from a quick look the API doesn't seem to match Sycamore so might have to roll its own version?

Sycamore already does something very similar. The problem is that the event handlers are tied to the reactive scope, otherwise the handler couldn't possibly reference reactive variables from the scope. However, it will always be possible to create a long-living reference to a DOM node (in both JS and Rust) and manually call the event handler, even after it has been removed from the tree!

[mc1098]

Sycamore already does something very similar. The problem is that the event handlers are tied to the reactive scope, otherwise the handler couldn't possibly reference reactive variables from the scope.

I don't think Sycamore removes event listeners from the node before the reactive scope ends and thus deallocates the handler before it's called on the JS side which is why the exception gets raised.

sycamore/packages/sycamore-web/src/dom_node.rs

Lines 330 to 335 in 53ec292

	let handler: Box<dyn FnMut(Self::EventType) + 'static> =
	unsafe { std::mem::transmute(boxed) };
	let closure = create_ref(cx, Closure::wrap(handler));
	self.node
	.add_event_listener_with_callback(intern(name), closure.as_ref().unchecked_ref())
	.unwrap_throw();

I'm looking at this part of the code and seeing that the Closure is essentially bound to the reactive scope which would just drop the Closure at the end of the reactive scope which still leaves the event listener pointing to a freed handler.

However, it will always be possible to create a long-living reference to a DOM node (in both JS and Rust) and manually call the event handler, even after it has been removed from the tree!

I'm not saying remove event listeners when removing the node from the DOM but when the reactive scope is being deallocated (including the event handlers) that Sycamore should remove the event listeners before the handler gets deallocated.

I did play with a simple solution similar to gloos struct that removes the event listener on drop and that fixed these errors but maybe I'm missing something?

[lukechu10]

I'm not saying remove event listeners when removing the node from the DOM but when the reactive scope is being deallocated (including the event handlers) that Sycamore should remove the event listeners before the handler gets deallocated.

Ah I understand what you mean. This could be easily solved using on_cleanup instead of create_ref and call remove_event_listener before dropping the Closure inside the cleanup.

The problem with calling remove_event_listener is that every time a node is removed, we must make an additional call to js which would not have been there before. I'm not sure that would be worth it since the current behavior which shows an error doesn't actually affect the running application anyways.

[mc1098]

The problem with calling remove_event_listener is that every time a node is removed, we must make an additional call to js which would not have been there before. I'm not sure that would be worth it since the current behavior which shows an error doesn't actually affect the running application anyways.

The behaviour in this case is unaffected, I'm just not sure if that would always be true - if a user is relying on the event handler to fire correctly even if the node is being removed from memory (atleast on the wasm side) then this could actually lead to a real issue where a solution could be very subtle of difficult for the user to implement.

As you say it would effect performance to do this on every call and might only be worth doing something about it once we reach an instance where this is causing an issue with the behaviour of the application - this said it is probably worth keeping this issue around or making some documentation note on this decision. The nice part here is that the fix even if it affects performance is not a breaking change so would be easy to provide a fix and minor version bump in the future if/when required.

I think in the future this may actually be solved by a couple of different things:

1. WeakRef in JS may allow Sycamore to just let JS deal with GC'ing the Closure.
2. Synthetic Event Delegation #176 may provide a nice solution to this problem.

[dullbananas]

I had this problem and it caused a slider to freeze.

I had to do this to make it work: https://codeberg.org/dullbananas/font-generator/commit/5fcc105e077fa758e03e1a978693d5292a20d501

[austintheriot]

Hello, I am seeing this same issue when navigating between routes with the sycamore-router. As far as I'm aware, I'm not using any event listeners or closures--just rendering a list of data. @dullbananas were you able to resolve this issue by changing the way you wrote your code?

Edit: fwiw using:

sycamore = "0.8"
sycamore-router = "0.8"

[lukechu10]

Curiously enough, I can't reproduce this in Firefox but it definitely happens in Chrome...

[lukechu10]

Also I was digging into wasm-bindgen internals and it seems like they do account for the case where the Closure is dropped from inside itself. So I'm lost at what the issue is here.

https://github.com/rustwasm/wasm-bindgen/blob/84b735c117737e084089d3e2d17af2b774ab26a5/src/closure.rs#L546-L558

[lukechu10]

A quick fix would be to use Closure::into_js_value() now that weak-refs is the default in wasm-bindgen. However, I would like to get to the bottom of this so I'll continue leaving this issue open.