fix: remove memory leak from bind:this (#11194)

* fix: remove memory leak from bind:this * alternative approach * add error * tidy * tidy * add TODO * add TODO * alternative approach
sveltejs · Apr 17, 2024 · 63456f1 · 63456f1
1 parent 9aebae8
commit 63456f1
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 16 deletions.
diff --git a/.changeset/itchy-eels-marry.md b/.changeset/itchy-eels-marry.md
@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: remove memory leak from bind:this
diff --git a/packages/svelte/src/internal/client/dom/elements/bindings/this.js b/packages/svelte/src/internal/client/dom/elements/bindings/this.js
@@ -1,6 +1,7 @@
 import { STATE_SYMBOL } from '../../../constants.js';
 import { effect, render_effect } from '../../../reactivity/effects.js';
 import { untrack } from '../../../runtime.js';
+import { queue_task } from '../../task.js';
 
 /**
  * @param {any} bound_value
@@ -47,7 +48,8 @@ export function bind_this(element_or_component, update, get_value, get_parts) {
 		});
 
 		return () => {
-			effect(() => {
+			// We cannot use effects in the teardown phase, we we use a microtask instead.
+			queue_task(() => {
 				if (parts && is_bound_this(get_value(...parts), element_or_component)) {
 					update(null, ...parts);
 				}

diff --git a/packages/svelte/src/internal/client/dom/task.js b/packages/svelte/src/internal/client/dom/task.js
@@ -1,12 +1,9 @@
 import { run_all } from '../../shared/utils.js';
 
 let is_task_queued = false;
-let is_raf_queued = false;
 
 /** @type {Array<() => void>} */
 let current_queued_tasks = [];
-/** @type {Array<() => void>} */
-let current_raf_tasks = [];
 
 function process_task() {
 	is_task_queued = false;
@@ -15,11 +12,15 @@ function process_task() {
 	run_all(tasks);
 }
 
-function process_raf_task() {
-	is_raf_queued = false;
-	const tasks = current_raf_tasks.slice();
-	current_raf_tasks = [];
-	run_all(tasks);
+/**
+ * @param {() => void} fn
+ */
+export function queue_task(fn) {
+	if (!is_task_queued) {
+		is_task_queued = true;
+		queueMicrotask(process_task);
+	}
+	current_queued_tasks.push(fn);
 }
 
 /**
@@ -29,7 +30,4 @@ export function flush_tasks() {
 	if (is_task_queued) {
 		process_task();
 	}
-	if (is_raf_queued) {
-		process_raf_task();
-	}
 }
diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js
@@ -7,9 +7,11 @@ import {
 	destroy_effect_children,
 	execute_effect,
 	get,
+	is_destroying_effect,
 	is_flushing_effect,
 	remove_reactions,
 	schedule_effect,
+	set_is_destroying_effect,
 	set_is_flushing_effect,
 	set_signal_status,
 	untrack
@@ -109,6 +111,12 @@ export function user_effect(fn) {
 				(DEV ? ': The Svelte $effect rune can only be used during component initialisation.' : '')
 		);
 	}
+	if (is_destroying_effect) {
+		throw new Error(
+			'ERR_SVELTE_EFFECT_IN_TEARDOWN' +
+				(DEV ? ': The Svelte $effect rune can not be used in the teardown phase of an effect.' : '')
+		);
+	}
 
 	// Non-nested `$effect(...)` in a component should be deferred
 	// until the component is mounted
@@ -140,6 +148,14 @@ export function user_pre_effect(fn) {
 					: '')
 		);
 	}
+	if (is_destroying_effect) {
+		throw new Error(
+			'ERR_SVELTE_EFFECT_IN_TEARDOWN' +
+				(DEV
+					? ': The Svelte $effect.pre rune can not be used in the teardown phase of an effect.'
+					: '')
+		);
+	}
 
 	return render_effect(fn);
 }
@@ -228,6 +244,22 @@ export function branch(fn) {
 	return create_effect(RENDER_EFFECT | BRANCH_EFFECT, fn, true);
 }
 
+/**
+ * @param {import("#client").Effect} effect
+ */
+export function execute_effect_teardown(effect) {
+	var teardown = effect.teardown;
+	if (teardown !== null) {
+		const previously_destroying_effect = is_destroying_effect;
+		set_is_destroying_effect(true);
+		try {
+			teardown.call(null);
+		} finally {
+			set_is_destroying_effect(previously_destroying_effect);
+		}
+	}
+}
+
 /**
  * @param {import('#client').Effect} effect
  * @returns {void}
@@ -249,7 +281,7 @@ export function destroy_effect(effect) {
 		}
 	}
 
-	effect.teardown?.call(null);
+	execute_effect_teardown(effect);
 
 	var parent = effect.parent;
 

diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
@@ -8,7 +8,12 @@ import {
 	object_prototype
 } from './utils.js';
 import { snapshot } from './proxy.js';
-import { destroy_effect, effect, user_pre_effect } from './reactivity/effects.js';
+import {
+	destroy_effect,
+	effect,
+	execute_effect_teardown,
+	user_pre_effect
+} from './reactivity/effects.js';
 import {
 	EFFECT,
 	RENDER_EFFECT,
@@ -37,12 +42,18 @@ let current_scheduler_mode = FLUSH_MICROTASK;
 // Used for handling scheduling
 let is_micro_task_queued = false;
 export let is_flushing_effect = false;
+export let is_destroying_effect = false;
 
 /** @param {boolean} value */
 export function set_is_flushing_effect(value) {
 	is_flushing_effect = value;
 }
 
+/** @param {boolean} value */
+export function set_is_destroying_effect(value) {
+	is_destroying_effect = value;
+}
+
 // Used for $inspect
 export let is_batching_effect = false;
 let is_inspecting_signal = false;
@@ -406,7 +417,7 @@ export function execute_effect(effect) {
 			destroy_effect_children(effect);
 		}
 
-		effect.teardown?.call(null);
+		execute_effect_teardown(effect);
 		var teardown = execute_reaction_fn(effect);
 		effect.teardown = typeof teardown === 'function' ? teardown : null;
 	} finally {
@@ -658,11 +669,11 @@ export function flush_sync(fn, flush_previous = true) {
 
 		var result = fn?.();
 
+		flush_tasks();
 		if (current_queued_root_effects.length > 0 || root_effects.length > 0) {
 			flush_sync();
 		}
 
-		flush_tasks();
 		flush_count = 0;
 
 		return result;