fix: prevent ownership validation from infering with component context (

#11438) Ownership validation had a false positive when rendering a component as slotted content of another component. To fix this, #11401 did set the current component context to the context the snippet was declared in, not to the context it is rendered in. This was flawed because it means that component context was altered in a way that setContext/getContext failed because the parent chain was incorrect. This fixes that by introducing a separate global (dev time only) which tracks the component function the ownership needs. fixes #11429
sveltejs · May 3, 2024 · 6e5ab2e · 6e5ab2e
1 parent dfb30aa
commit 6e5ab2e
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 25 deletions.
diff --git a/.changeset/two-candles-move.md b/.changeset/two-candles-move.md
@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: prevent ownership validation from infering with component context
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
@@ -746,12 +746,7 @@ function serialize_inline_component(node, component_name, context) {
 
 				if (context.state.options.dev) {
 					binding_initializers.push(
-						b.stmt(
-							b.call(
-								b.id('$.user_pre_effect'),
-								b.thunk(b.call(b.id('$.add_owner'), expression, b.id(component_name)))
-							)
-						)
+						b.stmt(b.call(b.id('$.add_owner_effect'), b.thunk(expression), b.id(component_name)))
 					);
 				}
 

diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js
@@ -1,8 +1,8 @@
 /** @typedef {{ file: string, line: number, column: number }} Location */
 
 import { STATE_SYMBOL } from '../constants.js';
-import { render_effect } from '../reactivity/effects.js';
-import { current_component_context, untrack } from '../runtime.js';
+import { render_effect, user_pre_effect } from '../reactivity/effects.js';
+import { dev_current_component_function, set_dev_current_component_function } from '../runtime.js';
 import { get_prototype_of } from '../utils.js';
 import * as w from '../warnings.js';
 
@@ -109,8 +109,7 @@ export function mark_module_end(component) {
  */
 export function add_owner(object, owner, global = false) {
 	if (object && !global) {
-		// @ts-expect-error
-		const component = current_component_context.function;
+		const component = dev_current_component_function;
 		const metadata = object[STATE_SYMBOL];
 		if (metadata && !has_owner(metadata, component)) {
 			let original = get_owner(metadata);
@@ -124,6 +123,20 @@ export function add_owner(object, owner, global = false) {
 	add_owner_to_object(object, owner, new Set());
 }
 
+/**
+ * @param {() => unknown} get_object
+ * @param {any} Component
+ */
+export function add_owner_effect(get_object, Component) {
+	var component = dev_current_component_function;
+	user_pre_effect(() => {
+		var prev = dev_current_component_function;
+		set_dev_current_component_function(component);
+		add_owner(get_object(), Component);
+		set_dev_current_component_function(prev);
+	});
+}
+
 /**
  * @param {import('#client').ProxyMetadata<any> | null} from
  * @param {import('#client').ProxyMetadata<any>} to

diff --git a/packages/svelte/src/internal/client/dom/blocks/snippet.js b/packages/svelte/src/internal/client/dom/blocks/snippet.js
@@ -1,7 +1,11 @@
 import { add_snippet_symbol } from '../../../shared/validate.js';
 import { EFFECT_TRANSPARENT } from '../../constants.js';
 import { branch, block, destroy_effect } from '../../reactivity/effects.js';
-import { current_component_context, set_current_component_context } from '../../runtime.js';
+import {
+	current_component_context,
+	dev_current_component_function,
+	set_dev_current_component_function
+} from '../../runtime.js';
 
 /**
  * @template {(node: import('#client').TemplateNode, ...args: any[]) => import('#client').Dom} SnippetFn
@@ -38,17 +42,17 @@ export function snippet(get_snippet, node, ...args) {
  * @returns
  */
 export function wrap_snippet(fn) {
-	let component = current_component_context;
+	let component = /** @type {import('#client').ComponentContext} */ (current_component_context);
 
 	return add_snippet_symbol(
 		(/** @type {import('#client').TemplateNode} */ node, /** @type {any[]} */ ...args) => {
-			var previous_component_context = current_component_context;
-			set_current_component_context(component);
+			var previous_component_function = dev_current_component_function;
+			set_dev_current_component_function(component.function);
 
 			try {
 				return fn(node, ...args);
 			} finally {
-				set_current_component_context(previous_component_context);
+				set_dev_current_component_function(previous_component_function);
 			}
 		}
 	);

diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js
@@ -1,5 +1,11 @@
 export { hmr } from './dev/hmr.js';
-export { ADD_OWNER, add_owner, mark_module_start, mark_module_end } from './dev/ownership.js';
+export {
+	ADD_OWNER,
+	add_owner,
+	mark_module_start,
+	mark_module_end,
+	add_owner_effect
+} from './dev/ownership.js';
 export { inspect } from './dev/inspect.js';
 export { await_block as await } from './dom/blocks/await.js';
 export { if_block as if } from './dom/blocks/if.js';

diff --git a/packages/svelte/src/internal/client/proxy.js b/packages/svelte/src/internal/client/proxy.js
@@ -75,8 +75,7 @@ export function proxy(value, immutable = true, parent = null) {
 				value[STATE_SYMBOL].owners =
 					parent === null
 						? current_component_context !== null
-							? // @ts-expect-error
-								new Set([current_component_context.function])
+							? new Set([current_component_context.function])
 							: null
 						: new Set();
 			}

diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
@@ -113,6 +113,26 @@ export let current_component_context = null;
 /** @param {import('#client').ComponentContext | null} context */
 export function set_current_component_context(context) {
 	current_component_context = context;
+	if (DEV) {
+		dev_current_component_function = context?.function;
+	}
+}
+
+/**
+ * The current component function. Different from current component context:
+ * ```html
+ * <!-- App.svelte -->
+ * <Foo>
+ *   <Bar /> <!-- context == Foo.svelte, function == App.svelte -->
+ * </Foo>
+ * ```
+ * @type {import('#client').ComponentContext['function']}
+ */
+export let dev_current_component_function = null;
+
+/** @param {import('#client').ComponentContext['function']} fn */
+export function set_dev_current_component_function(fn) {
+	dev_current_component_function = fn;
 }
 
 /** @returns {boolean} */
@@ -400,7 +420,7 @@ export function execute_effect(effect) {
 	var previous_component_context = current_component_context;
 
 	current_effect = effect;
-	current_component_context = component_context;
+	set_current_component_context(component_context);
 
 	try {
 		if ((flags & BLOCK_EFFECT) === 0) {
@@ -412,7 +432,7 @@ export function execute_effect(effect) {
 		effect.teardown = typeof teardown === 'function' ? teardown : null;
 	} finally {
 		current_effect = previous_effect;
-		current_component_context = previous_component_context;
+		set_current_component_context(previous_component_context);
 	}
 }
 
@@ -885,8 +905,8 @@ export function getContext(key) {
 	const result = /** @type {T} */ (context_map.get(key));
 
 	if (DEV) {
-		// @ts-expect-error
-		const fn = current_component_context.function;
+		const fn = /** @type {import('#client').ComponentContext} */ (current_component_context)
+			.function;
 		if (fn) {
 			add_owner(result, fn, true);
 		}
@@ -940,7 +960,6 @@ export function getAllContexts() {
 	const context_map = get_or_init_context_map('getAllContexts');
 
 	if (DEV) {
-		// @ts-expect-error
 		const fn = current_component_context?.function;
 		if (fn) {
 			for (const value of context_map.values()) {
@@ -1066,8 +1085,8 @@ export function push(props, runes = false, fn) {
 
 	if (DEV) {
 		// component function
-		// @ts-expect-error
 		current_component_context.function = fn;
+		dev_current_component_function = fn;
 	}
 }
 
@@ -1089,7 +1108,7 @@ export function pop(component) {
 				effect(effects[i]);
 			}
 		}
-		current_component_context = context_stack_item.p;
+		set_current_component_context(context_stack_item.p);
 		context_stack_item.m = true;
 	}
 	// Micro-optimization: Don't set .a above to the empty object

diff --git a/packages/svelte/src/internal/client/types.d.ts b/packages/svelte/src/internal/client/types.d.ts
@@ -49,6 +49,10 @@ export type ComponentContext = {
 		/** This tracks whether `$:` statements have run in the current cycle, to ensure they only run once */
 		r2: Source<boolean>;
 	};
+	/**
+	 * dev mode only: the component function
+	 */
+	function?: any;
 };
 
 export type ComponentContextLegacy = ComponentContext & {

diff --git a/packages/svelte/tests/runtime-legacy/samples/context-api/_config.js b/packages/svelte/tests/runtime-legacy/samples/context-api/_config.js
@@ -1,6 +1,9 @@
 import { test } from '../../test';
 
 export default test({
+	compileOptions: {
+		dev: true // to ensure dev mode does not break context in some way
+	},
 	html: `
 		<div class="tabs">
 			<div class="tab-list">