Skip to content

Commit

Permalink
fix: remove memory leak from retaining old DOM elements (#11197)
Browse files Browse the repository at this point in the history
* fix: remove memory leak from retaining old DOM elements

* missing logic

* fix dynamic html bug
  • Loading branch information
trueadm committed Apr 17, 2024
1 parent 63456f1 commit 777527b
Show file tree
Hide file tree
Showing 6 changed files with 186 additions and 23 deletions.
5 changes: 5 additions & 0 deletions .changeset/rich-plums-thank.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: remove memory leak from retaining old DOM elements
44 changes: 40 additions & 4 deletions packages/svelte/src/internal/client/dom/blocks/html.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,30 @@
import { derived } from '../../reactivity/deriveds.js';
import { render_effect } from '../../reactivity/effects.js';
import { get } from '../../runtime.js';
import { current_effect, get } from '../../runtime.js';
import { is_array } from '../../utils.js';
import { hydrate_nodes, hydrating } from '../hydration.js';
import { create_fragment_from_html, remove } from '../reconciler.js';
import { push_template_node } from '../template.js';

/**
* @param {import('#client').Effect} effect
* @param {(Element | Comment | Text)[]} to_remove
* @returns {void}
*/
function remove_from_parent_effect(effect, to_remove) {
const dom = effect.dom;

if (is_array(dom)) {
for (let i = dom.length - 1; i >= 0; i--) {
if (to_remove.includes(dom[i])) {
dom.splice(i, 1);
break;
}
}
} else if (dom !== null && to_remove.includes(dom)) {
effect.dom = null;
}
}

/**
* @param {Element | Text | Comment} anchor
Expand All @@ -11,13 +33,19 @@ import { create_fragment_from_html, remove } from '../reconciler.js';
* @returns {void}
*/
export function html(anchor, get_value, svg) {
const parent_effect = anchor.parentNode !== current_effect?.dom ? current_effect : null;
let value = derived(get_value);

render_effect(() => {
var dom = html_to_dom(anchor, get(value), svg);
var dom = html_to_dom(anchor, parent_effect, get(value), svg);

if (dom) {
return () => remove(dom);
return () => {
if (parent_effect !== null) {
remove_from_parent_effect(parent_effect, is_array(dom) ? dom : [dom]);
}
remove(dom);
};
}
});
}
Expand All @@ -27,11 +55,12 @@ export function html(anchor, get_value, svg) {
* inserts it before the target anchor and returns the new nodes.
* @template V
* @param {Element | Text | Comment} target
* @param {import('#client').Effect | null} effect
* @param {V} value
* @param {boolean} svg
* @returns {Element | Comment | (Element | Comment | Text)[]}
*/
function html_to_dom(target, value, svg) {
function html_to_dom(target, effect, value, svg) {
if (hydrating) return hydrate_nodes;

var html = value + '';
Expand All @@ -49,6 +78,9 @@ function html_to_dom(target, value, svg) {
if (node.childNodes.length === 1) {
var child = /** @type {Text | Element | Comment} */ (node.firstChild);
target.before(child);
if (effect !== null) {
push_template_node(effect, child);
}
return child;
}

Expand All @@ -62,5 +94,9 @@ function html_to_dom(target, value, svg) {
target.before(node);
}

if (effect !== null) {
push_template_node(effect, nodes);
}

return nodes;
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { is_array } from '../../utils.js';
import { set_should_intro } from '../../render.js';
import { current_each_item, set_current_each_item } from './each.js';
import { current_effect } from '../../runtime.js';
import { push_template_node } from '../template.js';

/**
* @param {import('#client').Effect} effect
Expand Down Expand Up @@ -131,6 +132,8 @@ export function element(anchor, get_tag, is_svg, render_fn) {
if (prev_element) {
swap_block_dom(parent_effect, prev_element, element);
prev_element.remove();
} else if (!hydrating) {
push_template_node(parent_effect, element);
}
});
}
Expand Down
88 changes: 69 additions & 19 deletions packages/svelte/src/internal/client/dom/template.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,36 @@ import { create_fragment_from_html } from './reconciler.js';
import { current_effect } from '../runtime.js';
import { TEMPLATE_FRAGMENT, TEMPLATE_USE_IMPORT_NODE } from '../../../constants.js';
import { effect } from '../reactivity/effects.js';
import { is_array } from '../utils.js';

/**
* @param {import("#client").Effect} effect
* @param {import("#client").TemplateNode | import("#client").TemplateNode[]} dom
*/
export function push_template_node(effect, dom) {
var current_dom = effect.dom;
if (current_dom === null) {
effect.dom = dom;
} else {
if (!is_array(current_dom)) {
current_dom = effect.dom = [current_dom];
}
var anchor;
// If we're working with an anchor, then remove it and put it at the end.
if (current_dom[0].nodeType === 8) {
anchor = current_dom.pop();
}
if (is_array(dom)) {
current_dom.push(...dom);
} else {
current_dom.push(dom);
}
if (anchor !== undefined) {
current_dom.push(anchor);
}
}
return dom;
}

/**
* @param {string} content
Expand All @@ -19,16 +49,31 @@ export function template(content, flags) {
var node;

return () => {
var effect = /** @type {import('#client').Effect} */ (current_effect);
if (hydrating) {
return is_fragment ? hydrate_nodes : /** @type {Node} */ (hydrate_nodes[0]);
var hydration_content = push_template_node(
effect,
is_fragment ? hydrate_nodes : hydrate_nodes[0]
);
return /** @type {Node} */ (hydration_content);
}

if (!node) {
node = create_fragment_from_html(content);
if (!is_fragment) node = /** @type {Node} */ (node.firstChild);
}
var clone = use_import_node ? document.importNode(node, true) : clone_node(node, true);

if (is_fragment) {
push_template_node(
effect,
/** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes])
);
} else {
push_template_node(effect, /** @type {import('#client').TemplateNode} */ (clone));
}

return use_import_node ? document.importNode(node, true) : clone_node(node, true);
return clone;
};
}

Expand Down Expand Up @@ -70,8 +115,13 @@ export function svg_template(content, flags) {
var node;

return () => {
var effect = /** @type {import('#client').Effect} */ (current_effect);
if (hydrating) {
return is_fragment ? hydrate_nodes : /** @type {Node} */ (hydrate_nodes[0]);
var hydration_content = push_template_node(
effect,
is_fragment ? hydrate_nodes : hydrate_nodes[0]
);
return /** @type {Node} */ (hydration_content);
}

if (!node) {
Expand All @@ -87,7 +137,18 @@ export function svg_template(content, flags) {
}
}

return clone_node(node, true);
var clone = clone_node(node, true);

if (is_fragment) {
push_template_node(
effect,
/** @type {import('#client').TemplateNode[]} */ ([...clone.childNodes])
);
} else {
push_template_node(effect, /** @type {import('#client').TemplateNode} */ (clone));
}

return clone;
};
}

Expand Down Expand Up @@ -152,7 +213,8 @@ function run_scripts(node) {
*/
/*#__NO_SIDE_EFFECTS__*/
export function text(anchor) {
if (!hydrating) return empty();
var effect = /** @type {import('#client').Effect} */ (current_effect);
if (!hydrating) return push_template_node(effect, empty());

var node = hydrate_nodes[0];

Expand All @@ -162,7 +224,7 @@ export function text(anchor) {
anchor.before((node = empty()));
}

return node;
return push_template_node(effect, node);
}

export const comment = template('<!>', TEMPLATE_FRAGMENT);
Expand All @@ -174,19 +236,7 @@ export const comment = template('<!>', TEMPLATE_FRAGMENT);
* @param {import('#client').Dom} dom
*/
export function append(anchor, dom) {
var current = dom;

if (!hydrating) {
var node = /** @type {Node} */ (dom);

if (node.nodeType === 11) {
// if hydrating, `dom` is already an array of nodes, but if not then
// we need to create an array to store it on the current effect
current = /** @type {import('#client').Dom} */ ([...node.childNodes]);
}

anchor.before(node);
anchor.before(/** @type {Node} */ (dom));
}

/** @type {import('#client').Effect} */ (current_effect).dom = current;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { flushSync } from '../../../../src/index-client';
import { test } from '../../test';

export default test({
html: `<button>add item</button><button>make span</button><button>reverse</button>`,

async test({ assert, target }) {
const [btn1, btn2, btn3] = target.querySelectorAll('button');

flushSync(() => {
btn1?.click();
btn1?.click();
btn1?.click();
});

assert.htmlEqual(
target.innerHTML,
`<button>add item</button><button>make span</button><button>reverse</button><div>Item 1</div><div>Item 2</div><div>Item 3</div>`
);

flushSync(() => {
btn2?.click();
});

assert.htmlEqual(
target.innerHTML,
`<button>add item</button><button>make span</button><button>reverse</button><span>Item 1</span><span>Item 2</span><span>Item 3</span>`
);

flushSync(() => {
btn3?.click();
});

assert.htmlEqual(
target.innerHTML,
`<button>add item</button><button>make span</button><button>reverse</button><span>Item 3</span><span>Item 2</span><span>Item 1</span>`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<script>
let items = $state([]);
function add_item() {
items.push({
id: items.length,
text: 'Item ' + (items.length + 1),
html: '<div>Item ' + (items.length + 1) + '</div>',
dom: null,
})
}
function make_span() {
items.forEach(item => {
item.html = item.html.replace(/div/g, 'span')
})
}
function reverse() {
items.reverse();
}
</script>

<button on:click={add_item}>add item</button>
<button on:click={make_span}>make span</button>
<button on:click={reverse}>reverse</button>

{#each items as item (item.id)}
{@html item.html}
{/each}

0 comments on commit 777527b

Please sign in to comment.