[FIX] component: subtle issue with unmounted children

Owl has to manage a lot of interesting situations.  One of them is when
a rendering is initiated, which creates a sub component, but then
another rendering starts, which invalidate the previous one, and will
create another sub component.  Since the first sub component was not
ever in the DOM, we cannot rely on the vdom patching process to remove
it, so we have to do it manually.

Sadly, this is actually a very tricky situation, since there are other
subtle situations where the code that remove an unmounted widget could
be executed, in particular when the parent component is unmounted, then
remounted, then modified to trigger yet another rendering.

In this commit, we handle this case more carefully by making sure that
the destroyed subcomponent properly configures its pvnode so the patch
process happens as expected.

joint work with the framework team, and in particular LPE for his work on
finding a testcase!

closes #724, #731

src/component/component.ts

@@ -656,9 +656,28 @@ export class Component<Props extends {} = any, T extends Env = Env> {
       // destroyed right now, because they are not in the DOM, and thus we won't
       // be notified later on (when patching), that they are removed from the DOM
       for (let childKey in __owl__.children) {
-        let child = __owl__.children[childKey];
-        if (!child.__owl__.isMounted && child.__owl__.parentLastFiberId < fiber.id) {
-          child.destroy();
+        const child = __owl__.children[childKey];
+        const childOwl = child.__owl__;
+        if (!childOwl.isMounted && childOwl.parentLastFiberId < fiber.id) {
+          // we only do here a "soft" destroy, meaning that we leave the child
+          // dom node alone, without removing it.  Most of the time, it does not
+          // matter, because the child component is already unmounted.  However,
+          // if some of its parent have been unmounted, the child could actually
+          // still be attached to its parent, and this may be important if we
+          // want to remount the parent, because the vdom need to match the
+          // actual DOM
+          child.__destroy(childOwl.parent);
+          if (childOwl.pvnode) {
+            // we remove the key here to make sure that the patching algorithm
+            // is able to make the difference between this pvnode and an eventual
+            // other instance of the same component
+            delete childOwl.pvnode.key;
+            // Since the component has been unmounted, we do not want to actually
+            // call a remove hook.  This is pretty important, since the t-component
+            // directive actually disabled it, so the vdom algorithm will just
+            // not remove the child elm if we don't remove the hook.
+            delete childOwl.pvnode.data!.hook!.remove;
+          }
         }
       }
       if (!vnode) {

src/qweb/base_directives.ts

@@ -275,16 +275,12 @@ QWeb.addDirective({
     const parentNode = ctx.parentNode ? `c${ctx.parentNode}` : "result";
     const extra = `Object.assign({}, extra, {parentNode: ${parentNode}, parent: ${parentComponent}, key: ${key}})`;
     if (ctx.parentNode) {
-      ctx.addLine(
-        `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`
-      );
+      ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`);
     } else {
       // this is a t-call with no parentnode, we need to extract the result
       ctx.rootContext.shouldDefineResult = true;
       ctx.addLine(`result = []`);
-      ctx.addLine(
-        `this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`
-      );
+      ctx.addLine(`this.constructor.subTemplates['${subId}'].call(this, scope, ${extra});`);
       ctx.addLine(`result = result[0]`);
     }
 

src/qweb/qweb.ts

@@ -216,8 +216,8 @@ export class QWeb extends EventBus {
   // id, and a (global) mapping from an id to the compiled function.  This is
   // necessary to ensure that global templates can be called with more than one
   // QWeb instance.
-  subTemplates: {[key: string]: number} = {};
-  static subTemplates: {[id: number]: Function} = {};
+  subTemplates: { [key: string]: number } = {};
+  static subTemplates: { [id: number]: Function } = {};
 
   isUpdating: boolean = false;
   translateFn?: QWebConfig["translateFn"];

tests/component/component.test.ts

@@ -3021,7 +3021,7 @@ describe("random stuff/miscellaneous", () => {
       static template = xml`
         <div class="widget-subkey">
           <t t-esc="props.key"/>__<t t-esc="props.subKey"/>
-        </div>`
+        </div>`;
     }
     class Child extends Component {
       static components = { Custom };
@@ -3029,7 +3029,7 @@ describe("random stuff/miscellaneous", () => {
         <t t-component="Custom"
           t-key="props.subKey"
           key="props.key"
-          subKey="props.subKey"/>`
+          subKey="props.subKey"/>`;
     }
 
     class Parent extends Component {
@@ -3045,24 +3045,24 @@ describe("random stuff/miscellaneous", () => {
     }
     const parent = new Parent(null);
     await parent.mount(fixture);
-    expect(fixture.textContent!.trim()).toBe('1__1');
+    expect(fixture.textContent!.trim()).toBe("1__1");
 
     // First step: change the Custom's instance
     Object.assign(parent.childProps, {
-        subKey: 2,
+      subKey: 2,
     });
     parent.render();
     await nextTick();
-    expect(fixture.textContent!.trim()).toBe('1__2');
+    expect(fixture.textContent!.trim()).toBe("1__2");
 
     // Second step, change both Child's and Custom's instance
     Object.assign(parent.childProps, {
-        key: 2,
-        subKey: 3,
+      key: 2,
+      subKey: 3,
     });
     parent.render();
     await nextTick();
-    expect(fixture.textContent!.trim()).toBe('2__3');
+    expect(fixture.textContent!.trim()).toBe("2__3");
   });
 });
 

tests/component/un_mounting.test.ts

@@ -509,4 +509,103 @@ describe("unmounting and remounting", () => {
     expect(error).toBeDefined();
     expect(error.message).toBe("Cannot mount a destroyed component");
   });
+
+  test("destroying a sub-component cleans itself from parent's vnode", async () => {
+    class C1 extends Component {
+      static template = xml`<div><div><t t-esc="props.a"/></div></div>`;
+    }
+    class P extends Component {
+      static components = { C1 };
+      static template = xml`<div><div><C1 t-props="state" t-if="state.a"/></div></div>`;
+      state = {
+        a: "first",
+      };
+    }
+    const parent = new P();
+    await parent.mount(fixture);
+    expect(fixture.textContent).toBe("first");
+    parent.unmount();
+    parent.state.a = "";
+    parent.mount(fixture);
+    parent.state.a = "fixed";
+    await parent.render();
+    expect(fixture.textContent).toBe("fixed");
+  });
+
+  test("destroying a sub-component cleans itself from parent's vnode, part 2", async () => {
+    class C1 extends Component {
+      static template = xml`<div><div><t t-esc="props.a"/></div></div>`;
+    }
+    class P extends Component {
+      static components = { C1 };
+      static template = xml`<div><div><C1 t-props="state" t-if="state.a"/>some text</div></div>`;
+      state = {
+        a: "first",
+      };
+    }
+    const parent = new P();
+    await parent.mount(fixture);
+    expect(fixture.textContent).toBe("firstsome text");
+    parent.unmount();
+    parent.state.a = "";
+    parent.mount(fixture);
+    parent.state.a = "fixed";
+    await parent.render();
+    expect(fixture.textContent).toBe("fixedsome text");
+  });
+
+  test("destroying a sub-component cleans itself from parent's vnode, part 3", async () => {
+    class C1 extends Component {
+      static template = xml`<div><div><t t-esc="props.a"/></div></div>`;
+    }
+
+    class C2 extends Component {
+      static template = xml`<C1 a="props.a"/>`;
+      static components = { C1 };
+    }
+
+    class P extends Component {
+      static components = { C2 };
+      static template = xml`<div><div><C2 t-props="state" t-if="state.a"/></div></div>`;
+      state = {
+        a: "first",
+      };
+    }
+    const parent = new P();
+    await parent.mount(fixture);
+    expect(fixture.textContent).toBe("first");
+    parent.unmount();
+    parent.state.a = "";
+    parent.mount(fixture);
+    parent.state.a = "fixed";
+    await parent.render();
+    expect(fixture.textContent).toBe("fixed");
+  });
+
+  test("destroying a sub-component cleans itself from parent's vnode, part 4", async () => {
+    class C1 extends Component {
+      static template = xml`<div><div><t t-esc="props.a"/></div></div>`;
+    }
+
+    class C2 extends Component {
+      static template = xml`<C1 a="props.a"/>`;
+      static components = { C1 };
+    }
+    class P extends Component {
+      static components = { C2 };
+      static template = xml`<div><div><C2 t-props="state" t-if="state.a"/>some text</div></div>`;
+      state = {
+        a: "first",
+      };
+    }
+    const parent = new P();
+    await parent.mount(fixture);
+    expect(fixture.textContent).toBe("firstsome text");
+    parent.unmount();
+    parent.state.a = "";
+    parent.mount(fixture);
+    parent.state.a = "fixed";
+    await parent.render();
+    expect(fixture.textContent).toBe("fixedsome text");
+  });
 });

tests/vdom.test.ts

@@ -1107,9 +1107,9 @@ describe("html to vdom", function () {
   });
 
   test("svg", function () {
-      const nodeList = htmlToVDOM(`<svg></svg>`);
-      expect(nodeList).toHaveLength(1);
-      elm = patch(vnode0, nodeList[0]).elm;
-      expect(elm).toBeInstanceOf(SVGSVGElement);
+    const nodeList = htmlToVDOM(`<svg></svg>`);
+    expect(nodeList).toHaveLength(1);
+    elm = patch(vnode0, nodeList[0]).elm;
+    expect(elm).toBeInstanceOf(SVGSVGElement);
   });
 });