Reused element binding (`bind:this`) updates to `null` long after destruction

kaizjen commented 2 months ago

Describe the bug

This might be too convoluted, see the REPL for a better explanation.

A component, after being destroyed, retains its bindings to the elements inside (via bind:this={element}). This wouldn't be a problem, but:

if this component somehow updates its binding after it's been destroyed and
it reuses its binding, either inside an {#each} block with the binding dependent on the index, or just reuses the variable then the component's element will be updated after the element has been destroyed and shouldn't have any rights to do so.

The 1st condition happens if the element has a outro transition. After the transition is done, the component schedules a binding callback via its {tag}_binding function. If, at this point, any update happens in the component, the destroyed element will update its binding with the component, which is unexpected behavior, since after the element is destroyed it shouldn't cause any updates in its component.

In the REPL, I proposed a solution, which may or may not be what Svelte needs. In short, it will replace the {tag}_binding function with:

let X_binding_active = true;
function X_binding($$value) {
    if (!X_binding_active) return;

    binding_callbacks[$$value ? 'unshift' : 'push'](() => {
        element = $$value;
        $$invalidate(0, element);
    });
}
$$self.$$.on_destroy.push(() => X_binding_active = false);

X is replaced by the element name. This code should be in the bind_this.js file on lines 29 and 80. I, unfortunately, haven't figured out how to make the X_binding_active variable named like that, because acorn spaghettifies its name 😅.

Anyway, to recap: You need an element, which: 1) has a outro transition on itself or one of its children, 2) has a bind:this binding And you need to 1) destroy the element, 2) update the component ($$invalidate) after the transition has played.

Edit: as I realized, you don't need multiple components

Reproduction

https://svelte.dev/repl/b1160a5db49943ac88bde30323cea348?version=4.2.17 Reproduction and even a possible solution inside the REPL (though duplicated here)

Edit: A much simpler test case can be found here.

There doesn't have to be an {#each}, i think, but this just serves as an example of why this bug can be disastrous. Sorry if I overstated the Severity field below, by the way.

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (12) x64 AMD Ryzen 5 3600 6-Core Processor
    Memory: 2.97 GB / 15.93 GB
Binaries:
    Node: 18.17.1 - ~\AppData\Roaming\node.EXE
    npm: 9.6.7 - ~\AppData\Roaming\npm.CMD

Tested both in Firefox and Chrome

Severity

application-breaking

dummdidumm commented 2 months ago

This is fixed in Svelte 5. Because this is a real edge case I'm tempted to say we're not going to backport this.

kaizjen commented 2 months ago

@dummdidumm I disagree. I'm sorry for such a convoluted test case (I didn't have enough time to test out more possibilities), but here's a much simpler and probably a more believable situation: https://svelte.dev/repl/5f80fe1b03494b5bafab5e738fee8e5e

I'd argue that reusing element bindings, although maybe uncommon, shouldn't be considered an edge case.

laszlokorte commented 1 month ago

Actually this seems to be not fixed in svelte5, at least not in production builds (vite build), whereas in dev mode (`vite dev') it seems to be working.

Here is an example showing it not working: https://static.laszlokorte.de/svelte-debug/

use the select box to switch to component D and then back to some other (A, B or C) and look into the browser console to see how a null value is set. The corresponding code can be found here: https://github.com/laszlokorte/svelte5-debug-component-binding/blob/main/src/App.svelte#L39

This might be the place where the null value is set: https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/internal/client/dom/elements/bindings/this.js#L54

But I am not sure why it only happens in production builds.

I opened a separate issue for svelte 5: https://github.com/sveltejs/svelte/issues/12287

sveltejs / svelte