fix: memory leak in unmount where document event listeners are not being removed

[markpsiano]

Event listeners for handling event propagation are not being removed from the document on component unmounts, leading to a memory leak as there are unused event listeners leftover after components unmount.

This can be verified by using chrome devtools, and taking heap snapshots at various points throughout page navigation/components mounting and unmounting, where the number of event listeners increases over time. It can also be verified in elements -> event listeners tab in devtools, where there are an increasing number of listeners upon mounting and unmounting components.

[changeset-bot]

🦋 Changeset detected

Latest commit: f6ff7bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[trueadm]

Ah I fixed the same thing in #12105.

[markpsiano]

any reason you had to steal my change?

[trueadm]

@markpsiano Completely coincidental actually. Great minds think alike clearly!

[markpsiano]

Sure thing. It seems like you just don't want 'memory leak' in your commit history, especially fixed by someone who doesn't work on the project and in a core part of the rendering code. Especially given there was not even an apology or a thank you after you saw my earlier PR.

On top of that, you'd need a magic ability to inspect code and find memory leaks to find this issue without doing any profiling. Given your commit was made 9 hours ago, that would be around 9 am your time. I find it hard to believe that the first thing you chose to do in the morning was stare at core rendering code until you could find leaks, without doing any profiling. And on top of that, you conveniently did not check any issues or PRs until after you made your change and merged it.

Before plagiarizing a contributor's change, without any expression of gratitude or an apology, you should think about the fact that you also represent Vercel. The fact that this is turning away potential contributors to the framework is very disappointing.

[trueadm]

@markpsiano As I commented in the issue #12102:

Thanks also for putting up a fix! Sorry I missed it originally – it's quite rare for someone to put up an issue and link to the fix at the same time :)

That's literally all that has happened here. I saw your issue, looked at the code and came up with the only fix possible. There is literally no other fix – we simply forgot to remove the event listeners from the document in cleanup function. Given the logic for removing event listeners from the root was the line above, and given that logic was written by me, I think it's very likely we'd come up with the same solution.

I want to be clear here, I respect your contribution and also I thank you for using and wanting to help improve Svelte. I certainly don't want to discourage any future contributions from you. I also have no issue with memory leaks being flagged or being in our commit history – I've both created and fixed plenty of memory leaks on Svelte, especially around signals!

[Rich-Harris]

It turns out that the merged PR had a typo: the wrong event listener was being removed, meaning the memory leak still exists. Hopefully this puts to rest any notion that this PR was plagiarized (it was not), but it also means that we can gratefully accept this PR — thank you

There are some other fixes that need to be made around this stuff but I'll open a separate PR for those

[markpsiano]

Got it. I will accept that, and apology to @trueadm for accusing you of of taking my change.

[rmunn]

@markpsiano - I noticed that you created the PR first, then created the issue that the PR fixes. To avoid a situation in the future where your PR might get missed, you may want to reverse that order: if your bugfix is ready to push, create the issue then create the PR afterwards. That way as you're writing the PR description, you can put the words "Fixes #XXX" in the PR description, which automatically links the PR to the issue and makes the PR show up at the top of the issue banner with the words "May be fixed by #NNN". Open-source project maintainers are used to seeing that "May be fixed by #NNN" in the banner and look for it there, and if it's not there, they can easily miss the fact that there's a PR link somewhere in the issue text.

Note that the PR number has to immediately follow the word "fix" or "fixes"; wording like "fix is here: #NNN" does not cause GitHub to auto-link the PR to the issue. https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests has the details of the syntax required.

Glad it's all straightened out now; this is just a (hopefully helpful) suggestion for how to avoid this kind of situation next time.