From c221721d7f89ce00a151886827d2e38917c911a9 Mon Sep 17 00:00:00 2001 From: "Lucas Perais (lpe)" Date: Tue, 11 Jan 2022 17:30:56 +0100 Subject: [PATCH] [FIX] components: avoid leaks when children are outdated/destroyed Every use case involving some sort of key set on a component would give birth to a leak in an async context: - If a key of a component changed, the outdated one was never destroyed. - destroyed component were never removed from their parent's reference map. This commit solves both issues, that are tightly linked anyway. --- src/compiler/code_generator.ts | 1 + src/component/component_node.ts | 18 ++ src/component/fibers.ts | 1 + .../__snapshots__/basics.test.ts.snap | 53 ++++++ .../__snapshots__/concurrency.test.ts.snap | 93 ++++++++++ tests/components/basics.test.ts | 71 ++++++- tests/components/concurrency.test.ts | 175 ++++++++++++++++++ 7 files changed, 411 insertions(+), 1 deletion(-) diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts index 22196a8e..4acb752c 100644 --- a/src/compiler/code_generator.ts +++ b/src/compiler/code_generator.ts @@ -644,6 +644,7 @@ export class CodeGenerator { index: block!.childNumber, forceNewBlock: false, isLast: ctx.isLast && i === children.length - 1, + tKeyExpr: ctx.tKeyExpr, }); this.compileAST(child, subCtx); } diff --git a/src/component/component_node.ts b/src/component/component_node.ts index 22481b68..24c1fce5 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -276,6 +276,7 @@ export class ComponentNode patch() { this.bdom!.patch(this!.fiber!.bdom!, false); + this.cleanOutdatedChildren(); this.fiber!.appliedToDom = true; this.fiber = null; } @@ -287,4 +288,21 @@ export class ComponentNode remove() { this.bdom!.remove(); } + + cleanOutdatedChildren() { + const childrenEntries = Object.entries(this.children); + if (!childrenEntries.length) { + return; + } + const children = this.children; + for (const [key, node] of childrenEntries) { + const status = node.status; + if (status !== STATUS.MOUNTED) { + delete children[key]; + if (status !== STATUS.DESTROYED) { + node.destroy(); + } + } + } + } } diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 8037d4ae..ed647368 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -144,6 +144,7 @@ export class RootFiber extends Fiber { // Step 2: patching the dom node.bdom!.patch(this.bdom!, Object.keys(node.children).length > 0); + node.cleanOutdatedChildren(); this.appliedToDom = true; this.locked = false; diff --git a/tests/components/__snapshots__/basics.test.ts.snap b/tests/components/__snapshots__/basics.test.ts.snap index 694811c8..9b905ca9 100644 --- a/tests/components/__snapshots__/basics.test.ts.snap +++ b/tests/components/__snapshots__/basics.test.ts.snap @@ -1124,6 +1124,59 @@ exports[`t-out in components can render list of t-out 1`] = ` }" `; +exports[`t-out in components component children doesn't leak (if case) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2; + if (ctx['ifVar']) { + b2 = component(\`Child\`, {}, key + \`__1\`, node, ctx); + } + return multi([b2]); + } +}" +`; + +exports[`t-out in components component children doesn't leak (if case) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + +exports[`t-out in components component children doesn't leak (t-key case) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + const tKey_1 = ctx['keyVar']; + return toggler(tKey_1, component(\`Child\`, {}, tKey_1 + key + \`__1\`, node, ctx)); + } +}" +`; + +exports[`t-out in components component children doesn't leak (t-key case) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + exports[`t-out in components update properly on state changes 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index a0c98bda..b7fc5a0c 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -1143,6 +1143,99 @@ exports[`rendering component again in next microtick 2`] = ` }" `; +exports[`t-foreach with dynamic async component 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + let { prepareList, withKey } = helpers; + + return function template(ctx, node, key = \\"\\") { + ctx = Object.create(ctx); + const [k_block1, v_block1, l_block1, c_block1] = prepareList(ctx['list']); + for (let i1 = 0; i1 < l_block1; i1++) { + ctx[\`arr\`] = v_block1[i1]; + ctx[\`arr_index\`] = i1; + let key1 = ctx['arr_index']; + let b3; + if (ctx['arr']) { + let Comp1 = ctx['myComp']; + b3 = toggler(Comp1, component(Comp1, {key: ctx['arr'][0]}, key + \`__1__\${key1}\`, node, ctx)); + } + c_block1[i1] = withKey(multi([b3]), key1); + } + return list(c_block1); + } +}" +`; + +exports[`t-foreach with dynamic async component 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let txt1 = ctx['props'].key; + return block1([txt1]); + } +}" +`; + +exports[`t-key on dom node having a component 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + const tKey_1 = ctx['key']; + let Comp1 = ctx['myComp']; + let b2 = toggler(tKey_1, toggler(Comp1, component(Comp1, {key: ctx['key']}, tKey_1 + key + \`__1\`, node, ctx))); + return toggler(tKey_1, block1([], [b2])); + } +}" +`; + +exports[`t-key on dom node having a component 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(ctx['props'].key); + } +}" +`; + +exports[`t-key on dynamic async component (toggler is never patched) 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + const tKey_1 = ctx['key']; + let Comp1 = ctx['myComp']; + return toggler(tKey_1, toggler(Comp1, component(Comp1, {key: ctx['key']}, tKey_1 + key + \`__1\`, node, ctx))); + } +}" +`; + +exports[`t-key on dynamic async component (toggler is never patched) 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + let txt1 = ctx['props'].key; + return block1([txt1]); + } +}" +`; + exports[`two renderings initiated between willPatch and patched 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/basics.test.ts b/tests/components/basics.test.ts index b590384c..62bfd1db 100644 --- a/tests/components/basics.test.ts +++ b/tests/components/basics.test.ts @@ -1,5 +1,5 @@ import { App, Component, mount, status, useState, xml } from "../../src"; -import { elem, makeTestFixture, nextTick, snapshotEverything } from "../helpers"; +import { elem, makeTestFixture, nextTick, snapshotEverything, useLogLifecycle } from "../helpers"; import { markup } from "../../src/utils"; let fixture: HTMLElement; @@ -900,4 +900,73 @@ describe("t-out in components", () => { "
<b>one</b>one<b>two</b>two<b>tree</b>tree
" ); }); + + test("component children doesn't leak (if case)", async () => { + class Child extends Component { + static template = xml`
`; + setup() { + useLogLifecycle(); + } + } + class Parent extends Component { + static components = { Child }; + static template = xml``; + ifVar = true; + } + + const parent = await mount(Parent, fixture); + expect(Object.keys(parent.__owl__.children).length).toStrictEqual(1); + expect([ + "Child:setup", + "Child:willStart", + "Child:willRender", + "Child:rendered", + "Child:mounted", + ]).toBeLogged(); + + parent.ifVar = false; + parent.render(); + await nextTick(); + expect(Object.keys(parent.__owl__.children).length).toStrictEqual(0); + expect(["Child:willUnmount", "Child:willDestroy"]).toBeLogged(); + }); + + test("component children doesn't leak (t-key case)", async () => { + // This test should encompass the t-foreach and t-call cases too (because they also use a flavor of some key) + class Child extends Component { + static template = xml`
`; + setup() { + useLogLifecycle(); + } + } + class Parent extends Component { + static components = { Child }; + static template = xml``; + keyVar = 1; + } + + const parent = await mount(Parent, fixture); + expect(Object.keys(parent.__owl__.children).length).toStrictEqual(1); + expect([ + "Child:setup", + "Child:willStart", + "Child:willRender", + "Child:rendered", + "Child:mounted", + ]).toBeLogged(); + + parent.keyVar = 2; + parent.render(); + await nextTick(); + expect(Object.keys(parent.__owl__.children).length).toStrictEqual(1); + expect([ + "Child:setup", + "Child:willStart", + "Child:willRender", + "Child:rendered", + "Child:willUnmount", + "Child:willDestroy", + "Child:mounted", + ]).toBeLogged(); + }); }); diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index bdb78b0d..d95470c8 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -2907,6 +2907,181 @@ test("two sequential renderings before an animation frame", async () => { // we check here that the willPatch and patched hooks are called only once expect(["Parent:willPatch", "Child:willPatch", "Child:patched", "Parent:patched"]).toBeLogged(); }); + +test("t-key on dom node having a component", async () => { + let def: any; + class Child extends Component { + static template = xml``; + setup() { + onWillStart(() => def); + useLogLifecycle(this.props.key); + } + } + + class Parent extends Component { + key = 1; + myComp = Child; + static template = xml`
`; + } + + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("
1
"); + + def = makeDeferred(); + parent.key = 2; + parent.render(); + + await nextTick(); + expect([ + "Child (1):setup", + "Child (1):willStart", + "Child (1):willRender", + "Child (1):rendered", + "Child (1):mounted", + "Child (2):setup", + "Child (2):willStart", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("
1
"); + parent.key = 3; + parent.render(); + + const prevDef = def; + def = undefined; + + parent.key = 3; + parent.render(); + prevDef.resolve(); + await nextTick(); + + expect(fixture.innerHTML).toBe("
3
"); + expect([ + "Child (3):setup", + "Child (3):willStart", + "Child (3):willRender", + "Child (3):rendered", + "Child (1):willUnmount", + "Child (1):willDestroy", + "Child (2):willDestroy", + "Child (3):mounted", + ]).toBeLogged(); +}); + +test("t-key on dynamic async component (toggler is never patched)", async () => { + let def: any; + class Child extends Component { + static template = xml`
`; + setup() { + onWillStart(() => def); + useLogLifecycle(this.props.key); + } + } + + class Parent extends Component { + key = 1; + myComp = Child; + static template = xml``; + } + + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("
1
"); + + def = makeDeferred(); + parent.key = 2; + parent.render(); + + await nextTick(); + expect([ + "Child (1):setup", + "Child (1):willStart", + "Child (1):willRender", + "Child (1):rendered", + "Child (1):mounted", + "Child (2):setup", + "Child (2):willStart", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("
1
"); + parent.key = 3; + parent.render(); + + const prevDef = def; + def = undefined; + + parent.key = 3; + parent.render(); + prevDef.resolve(); + await nextTick(); + + expect(fixture.innerHTML).toBe("
3
"); + expect([ + "Child (3):setup", + "Child (3):willStart", + "Child (3):willRender", + "Child (3):rendered", + "Child (1):willUnmount", + "Child (1):willDestroy", + "Child (2):willDestroy", + "Child (3):mounted", + ]).toBeLogged(); +}); + +test("t-foreach with dynamic async component", async () => { + let def: any; + class Child extends Component { + static template = xml`
`; + setup() { + onWillStart(() => def); + useLogLifecycle(this.props.key); + } + } + + class Parent extends Component { + list: any = [[1]]; + myComp = Child; + static template = xml` + + `; + } + + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("
1
"); + + def = makeDeferred(); + parent.list = [, [2]]; + parent.render(); + + await nextTick(); + expect([ + "Child (1):setup", + "Child (1):willStart", + "Child (1):willRender", + "Child (1):rendered", + "Child (1):mounted", + "Child (2):setup", + "Child (2):willStart", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("
1
"); + parent.list = [, , [3]]; + parent.render(); + + const prevDef = def; + def = undefined; + + parent.render(); + prevDef.resolve(); + await nextTick(); + + expect(fixture.innerHTML).toBe("
3
"); + expect([ + "Child (3):setup", + "Child (3):willStart", + "Child (3):willRender", + "Child (3):rendered", + "Child (1):willUnmount", + "Child (1):willDestroy", + "Child (2):willDestroy", + "Child (3):mounted", + ]).toBeLogged(); +}); // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 };