From d849558546bca5d29846b1b7a5bfb578e2ab7f0f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 8 Apr 2024 12:42:19 +0200 Subject: [PATCH] fix: ensure deep mutation ownership widening Previously, ownership widening/removal was only done if the immediate object that was encountered was state. This isn't always the case. It also didn't take into account classes with state on it (which turn into getters). This change takes both these cases into account and now always traverses the given object deeply. fixes #11060 --- .changeset/mighty-frogs-obey.md | 5 ++ .../src/internal/client/dev/ownership.js | 85 ++++++++++++++++--- .../_config.js | 35 ++++++++ .../main.svelte | 25 ++++++ .../sub.svelte | 11 +++ 5 files changed, 148 insertions(+), 13 deletions(-) create mode 100644 .changeset/mighty-frogs-obey.md create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte diff --git a/.changeset/mighty-frogs-obey.md b/.changeset/mighty-frogs-obey.md new file mode 100644 index 000000000000..0321c60463ac --- /dev/null +++ b/.changeset/mighty-frogs-obey.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: ensure deep mutation ownership widening diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 358b56df224b..fde316c8401a 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -2,6 +2,7 @@ import { STATE_SYMBOL } from '../constants.js'; import { untrack } from '../runtime.js'; +import { get_descriptors } from '../utils.js'; /** @type {Record>} */ const boundaries = {}; @@ -91,49 +92,107 @@ export function mark_module_end() { } } +let add_owner_visited = new Set(); + /** * * @param {any} object * @param {any} owner */ export function add_owner(object, owner) { - untrack(() => { - add_owner_to_object(object, owner); - }); + // Needed because ownership addition can invoke getters on a proxy, + // calling add_owner anew, so just keeping the set as part of + // add_owner_to_object would not be enough. + const prev = add_owner_visited; + try { + add_owner_visited = new Set(add_owner_visited); + untrack(() => { + add_owner_to_object(object, owner, add_owner_visited); + }); + } finally { + add_owner_visited = prev; + } } /** * @param {any} object * @param {Function} owner + * @param {Set} visited */ -function add_owner_to_object(object, owner) { +function add_owner_to_object(object, owner, visited) { + if (visited.has(object)) return; + visited.add(object); + if (object?.[STATE_SYMBOL]?.o && !object[STATE_SYMBOL].o.has(owner)) { object[STATE_SYMBOL].o.add(owner); - - for (const key in object) { - add_owner_to_object(object[key], owner); - } } + // Not inside previous if-block; there could be normal objects in-between + traverse_for_owners(object, (nested) => add_owner_to_object(nested, owner, visited)); } +let strip_owner_visited = new Set(); + /** * @param {any} object */ export function strip_owner(object) { - untrack(() => { - strip_owner_from_object(object); - }); + // Needed because ownership stripping can invoke getters on a proxy, + // calling strip_owner anew, so just keeping the set as part of + // strip_owner_from_object would not be enough. + const prev = strip_owner_visited; + try { + untrack(() => { + strip_owner_from_object(object, strip_owner_visited); + }); + } finally { + strip_owner_visited = prev; + } } /** * @param {any} object + * @param {Set} visited */ -function strip_owner_from_object(object) { +function strip_owner_from_object(object, visited) { + if (visited.has(object)) return; + visited.add(object); + if (object?.[STATE_SYMBOL]?.o) { object[STATE_SYMBOL].o = null; + } + // Not inside previous if-block; there could be normal objects in-between + traverse_for_owners(object, (nested) => strip_owner_from_object(nested, visited)); +} +/** + * @param {any} object + * @param {(obj: any) => void} cb + */ +function traverse_for_owners(object, cb) { + if (typeof object === 'object' && object !== null && !(object instanceof EventTarget)) { for (const key in object) { - strip_owner(object[key]); + cb(object[key]); + } + // deal with state on classes + const proto = Object.getPrototypeOf(object); + if ( + proto !== Object.prototype && + proto !== Array.prototype && + proto !== Map.prototype && + proto !== Set.prototype && + proto !== Date.prototype + ) { + const descriptors = get_descriptors(proto); + for (let key in descriptors) { + const get = descriptors[key].get; + if (get) { + try { + cb(object[key]); + } catch (e) { + // continue + } + } + } } } } diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js new file mode 100644 index 000000000000..e2b31a2e64fb --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js @@ -0,0 +1,35 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +/** @type {typeof console.warn} */ +let warn; + +/** @type {any[]} */ +let warnings = []; + +export default test({ + compileOptions: { + dev: true + }, + + before_test: () => { + warn = console.warn; + + console.warn = (...args) => { + warnings.push(...args); + }; + }, + + after_test: () => { + console.warn = warn; + warnings = []; + }, + + async test({ assert, target }) { + const btn = target.querySelector('button'); + + await btn?.click(); + await tick(); + assert.deepEqual(warnings.length, 0); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte new file mode 100644 index 000000000000..ffe9735e21f9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte @@ -0,0 +1,25 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte new file mode 100644 index 000000000000..0f60a4584576 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte @@ -0,0 +1,11 @@ + + +