[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

@@ -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);
       }

src/component/component_node.ts

@@ -276,6 +276,7 @@ export class ComponentNode<T extends typeof Component = typeof Component>
 
   patch() {
     this.bdom!.patch(this!.fiber!.bdom!, false);
+    this.cleanOutdatedChildren();
     this.fiber!.appliedToDom = true;
     this.fiber = null;
   }
@@ -287,4 +288,21 @@ export class ComponentNode<T extends typeof Component = typeof Component>
   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();
+        }
+      }
+    }
+  }
 }

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;

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(\`<div/>\`);
+  
+  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(\`<div/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`t-out in components update properly on state changes 1`] = `
 "function anonymous(bdom, helpers
 ) {

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(\`<div><block-text-0/></div>\`);
+  
+  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(\`<div><block-child-0/></div>\`);
+  
+  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(\`<div><block-text-0/></div>\`);
+  
+  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
 ) {

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", () => {
       "<div>&lt;b&gt;one&lt;/b&gt;<b>one</b>&lt;b&gt;two&lt;/b&gt;<b>two</b>&lt;b&gt;tree&lt;/b&gt;<b>tree</b></div>"
     );
   });
+
+  test("component children doesn't leak (if case)", async () => {
+    class Child extends Component {
+      static template = xml`<div />`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+    class Parent extends Component {
+      static components = { Child };
+      static template = xml`<Child t-if="ifVar" />`;
+      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`<div />`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+    class Parent extends Component {
+      static components = { Child };
+      static template = xml`<Child t-key="keyVar" />`;
+      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();
+  });
 });

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`<t t-esc="props.key" />`;
+    setup() {
+      onWillStart(() => def);
+      useLogLifecycle(this.props.key);
+    }
+  }
+
+  class Parent extends Component {
+    key = 1;
+    myComp = Child;
+    static template = xml`<div t-key="key"><t t-component="myComp" key="key" /></div>`;
+  }
+
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("<div>1</div>");
+
+  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("<div>1</div>");
+  parent.key = 3;
+  parent.render();
+
+  const prevDef = def;
+  def = undefined;
+
+  parent.key = 3;
+  parent.render();
+  prevDef.resolve();
+  await nextTick();
+
+  expect(fixture.innerHTML).toBe("<div>3</div>");
+  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`<div t-esc="props.key" />`;
+    setup() {
+      onWillStart(() => def);
+      useLogLifecycle(this.props.key);
+    }
+  }
+
+  class Parent extends Component {
+    key = 1;
+    myComp = Child;
+    static template = xml`<t t-component="myComp" t-key="key" key="key" />`;
+  }
+
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("<div>1</div>");
+
+  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("<div>1</div>");
+  parent.key = 3;
+  parent.render();
+
+  const prevDef = def;
+  def = undefined;
+
+  parent.key = 3;
+  parent.render();
+  prevDef.resolve();
+  await nextTick();
+
+  expect(fixture.innerHTML).toBe("<div>3</div>");
+  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`<div t-esc="props.key" />`;
+    setup() {
+      onWillStart(() => def);
+      useLogLifecycle(this.props.key);
+    }
+  }
+
+  class Parent extends Component {
+    list: any = [[1]];
+    myComp = Child;
+    static template = xml`<t t-foreach="list" t-as="arr" t-key="arr_index">
+        <t t-if="arr" t-component="myComp" key="arr[0]" />
+      </t>`;
+  }
+
+  const parent = await mount(Parent, fixture);
+  expect(fixture.innerHTML).toBe("<div>1</div>");
+
+  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("<div>1</div>");
+  parent.list = [, , [3]];
+  parent.render();
+
+  const prevDef = def;
+  def = undefined;
+
+  parent.render();
+  prevDef.resolve();
+  await nextTick();
+
+  expect(fixture.innerHTML).toBe("<div>3</div>");
+  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 };