fix: loop-guard scope leak

sveltejs · Dec 9, 2019 · 2c9d864 · 2c9d864
1 parent 6a4956b
commit 2c9d864
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 63 deletions.
diff --git a/src/compiler/compile/Component.ts b/src/compiler/compile/Component.ts
@@ -344,15 +344,16 @@ export default class Component {
 		};
 	}
 
-	get_unique_name(name: string): Identifier {
+	get_unique_name(name: string, scope?: Scope): Identifier {
 		if (test) name = `${name}$`;
 		let alias = name;
 		for (
 			let i = 1;
 			reserved.has(alias) ||
 			this.var_lookup.has(alias) ||
 			this.used_names.has(alias) ||
-			this.globally_used_names.has(alias);
+			this.globally_used_names.has(alias) ||
+			(scope && scope.has(alias));
 			alias = `${name}_${i++}`
 		);
 		this.used_names.add(alias);
@@ -707,8 +708,7 @@ export default class Component {
 		const remove = (parent, prop, index) => {
 			to_remove.unshift([parent, prop, index]);
 		};
-
-		const to_insert = new Map();
+		let scope_updated = false;
 
 		walk(content, {
 			enter(node, parent, prop, index) {
@@ -735,37 +735,21 @@ export default class Component {
 				}
 
 				component.warn_on_undefined_store_value_references(node, parent, scope);
+			},
 
+			leave(node) {
+				// do it on leave, to prevent infinite loop
 				if (component.compile_options.dev && component.compile_options.loopGuardTimeout > 0) {
-					const to_insert_for_loop_protect = component.loop_protect(node, prop, index, component.compile_options.loopGuardTimeout);
-					if (to_insert_for_loop_protect) {
-						if (!Array.isArray(parent[prop])) {
-							parent[prop] = {
-								type: 'BlockStatement',
-								body: [to_insert_for_loop_protect.node, node],
-							};
-						} else {
-							// can't insert directly, will screw up the index in the for-loop of estree-walker
-							if (!to_insert.has(parent)) {
-								to_insert.set(parent, []);
-							}
-							to_insert.get(parent).push(to_insert_for_loop_protect);
-						}
+					const to_replace_for_loop_protect = component.loop_protect(node, scope, component.compile_options.loopGuardTimeout);
+					if (to_replace_for_loop_protect) {
+						this.replace(to_replace_for_loop_protect);
+						scope_updated = true;
 					}
 				}
-			},
 
-			leave(node) {
 				if (map.has(node)) {
 					scope = scope.parent;
-				}
-				if (to_insert.has(node)) {
-					const nodes_to_insert = to_insert.get(node);
-					for (const { index, prop, node: node_to_insert } of nodes_to_insert.reverse()) {
-						node[prop].splice(index, 0, node_to_insert);
-					}
-					to_insert.delete(node);
-				}
+				}				
 			},
 		});
 
@@ -778,6 +762,12 @@ export default class Component {
 				}
 			}
 		}
+
+		if (scope_updated) {
+			const { scope, map } = create_scopes(script.content);
+			this.instance_scope = scope;
+			this.instance_scope_map = map;
+		}
 	}
 
 	track_references_and_mutations() {
@@ -849,15 +839,12 @@ export default class Component {
 		}
 	}
 
-	loop_protect(node, prop, index, timeout) {
+	loop_protect(node, scope: Scope, timeout: number): Node | null {
 		if (node.type === 'WhileStatement' ||
 			node.type === 'ForStatement' ||
 			node.type === 'DoWhileStatement') {
-			const guard = this.get_unique_name('guard');
-			this.add_var({
-				name: guard.name,
-				internal: true,
-			});
+			const guard = this.get_unique_name('guard', scope);
+			this.used_names.add(guard.name);
 
 			const before = b`const ${guard} = @loop_guard(${timeout})`;
 			const inside = b`${guard}();`;
@@ -870,7 +857,14 @@ export default class Component {
 				};
 			}
 			node.body.body.push(inside[0]);
-			return { index, prop, node: before[0] };
+
+			return {
+				type: 'BlockStatement',
+				body: [
+					before[0],
+					node,
+				],
+			};
 		}
 		return null;
 	}

diff --git a/test/js/samples/loop-protect/expected.js b/test/js/samples/loop-protect/expected.js
@@ -1,8 +1,13 @@
 /* generated by Svelte vX.Y.Z */
 import {
 	SvelteComponentDev,
+	add_location,
+	binding_callbacks,
+	detach_dev,
 	dispatch_dev,
+	element,
 	init,
+	insert_dev,
 	loop_guard,
 	noop,
 	safe_not_equal
@@ -11,16 +16,27 @@ import {
 const file = undefined;
 
 function create_fragment(ctx) {
+	let div;
+
 	const block = {
-		c: noop,
+		c: function create() {
+			div = element("div");
+			add_location(div, file, 22, 0, 288);
+		},
 		l: function claim(nodes) {
 			throw new Error("options.hydrate only works if the component was compiled with the `hydratable: true` option");
 		},
-		m: noop,
+		m: function mount(target, anchor) {
+			insert_dev(target, div, anchor);
+			/*div_binding*/ ctx[1](div);
+		},
 		p: noop,
 		i: noop,
 		o: noop,
-		d: noop
+		d: function destroy(detaching) {
+			if (detaching) detach_dev(div);
+			/*div_binding*/ ctx[1](null);
+		}
 	};
 
 	dispatch_dev("SvelteRegisterBlock", {
@@ -34,62 +50,91 @@ function create_fragment(ctx) {
 	return block;
 }
 
-function instance($$self) {
-	const guard = loop_guard(100);
+function foo() {
+	const guard = "foo";
+
+	{
+		const guard_1 = loop_guard(100);
 
-	while (true) {
-		foo();
-		guard();
+		while (true) {
+			console.log(guard);
+			guard_1();
+		}
 	}
+}
+
+function instance($$self, $$props, $$invalidate) {
+	let node;
 
-	const guard_1 = loop_guard(100);
+	{
+		const guard = loop_guard(100);
 
-	for (; ; ) {
-		foo();
-		guard_1();
+		while (true) {
+			foo();
+			guard();
+		}
 	}
 
-	const guard_2 = loop_guard(100);
+	{
+		const guard_2 = loop_guard(100);
 
-	while (true) {
-		foo();
-		guard_2();
+		for (; ; ) {
+			foo();
+			guard_2();
+		}
 	}
 
-	const guard_4 = loop_guard(100);
+	{
+		const guard_3 = loop_guard(100);
 
-	do {
-		foo();
-		guard_4();
-	} while (true);
+		while (true) {
+			foo();
+			guard_3();
+		}
+	}
+
+	{
+		const guard_5 = loop_guard(100);
+
+		do {
+			foo();
+			guard_5();
+		} while (true);
+	}
+
+	function div_binding($$value) {
+		binding_callbacks[$$value ? "unshift" : "push"](() => {
+			$$invalidate(0, node = $$value);
+		});
+	}
 
 	$$self.$capture_state = () => {
 		return {};
 	};
 
 	$$self.$inject_state = $$props => {
-
+		if ("node" in $$props) $$invalidate(0, node = $$props.node);
 	};
 
 	$: {
-		const guard_3 = loop_guard(100);
+		const guard_4 = loop_guard(100);
 
 		while (true) {
 			foo();
-			guard_3();
+			guard_4();
 		}
 	}
 
 	$: {
-		const guard_5 = loop_guard(100);
+		const guard_6 = loop_guard(100);
 
 		do {
 			foo();
-			guard_5();
+			guard_6();
 		} while (true);
 	}
 
-	return [];
+	return [node, div_binding];
 }
 
 class Component extends SvelteComponentDev {

diff --git a/test/js/samples/loop-protect/input.svelte b/test/js/samples/loop-protect/input.svelte
@@ -1,4 +1,13 @@
 <script>
+  let node;
+
+  function foo() {
+    const guard = 'foo';
+    while(true) {
+      console.log(guard);
+    }
+  }
+
   while(true) {
     foo();
   }
@@ -9,4 +18,6 @@
   $: while(true) foo();
   do foo(); while(true);
   $: do foo(); while(true);
-</script>
+</script>
+
+<div bind:this={node}></div>
diff --git a/test/runtime/samples/loop-protect-inner-function/_config.js b/test/runtime/samples/loop-protect-inner-function/_config.js
@@ -0,0 +1,7 @@
+export default {
+	html: '<div></div>',
+	compileOptions: {
+		dev: true,
+		loopGuardTimeout: 100,
+	}
+};
diff --git a/test/runtime/samples/loop-protect-inner-function/main.svelte b/test/runtime/samples/loop-protect-inner-function/main.svelte
@@ -0,0 +1,7 @@
+<script>
+  let node;
+  (function () {
+    while(false) ;
+  }());
+</script>
+<div bind:this={node}></div>