From 81af21a02564614d38e9ab89b10d191ef1f768ea Mon Sep 17 00:00:00 2001
From: "Lucas Perais (lpe)" <lpe@odoo.com>
Date: Tue, 1 Sep 2020 09:35:08 +0200
Subject: [PATCH] [FIX] component, fiber: update props with virtual node should
 not crash

Have a hierarchy of A, B, C components where:

```xml
<div t-name="A">
  <div>
    <B t-key="key1"/>
  </div>
</div>

<t t-name="B">
  <C t-key="key2"/>
</t>

<div t-name="C">
  <div><t t-esc="keys_as_props" /></div>
</div>
```

The subtility of the issues lies in B, which doesn't have its own
concrete DOM element, rather, it borrows it from C.

With the sequence of events:
- change key2
C1 is destroyed and replaced by another instance, and another node.
B1 has its props updated and is patched with the C2's node (CRITICAL)
A1 is patched

- change key1 AND key2
C2 is destroyed
B1 is destroyed
A1 is patched replacing B1 by B2, and their nodes too (which at this point should be C2's to C3's)

Before this commit, at the CRITICAL point, the node representing the component itself
(technically its pvnode) was not updated with the new concrete node provided by B1 patch with C2 node
i.e. it held the previous node still
The second array of steps crashed because at A1 patch, the new B2 node would replace B1, which
was out of the DOM (removed because C1 was destroyed long before),
and therefore without a viable parent to insert B2 node.

After this commit, we update the component's pvnode after the patch which elm had possibly changed
There is no crash anymore for this use case.
---
 src/component/fiber.ts            |  6 ++++
 tests/component/component.test.ts | 49 +++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/src/component/fiber.ts b/src/component/fiber.ts
index 84d11cfa..dc6b2ba3 100644
--- a/src/component/fiber.ts
+++ b/src/component/fiber.ts
@@ -238,6 +238,12 @@ export class Fiber {
       } else {
         if (fiber.shouldPatch) {
           component.__patch(component.__owl__.vnode!, fiber.vnode!);
+          // When updating a Component's props (in directive),
+          // the component has a pvnode AND should be patched.
+          // However, its pvnode.elm may have changed if it is a High Order Component
+          if (component.__owl__.pvnode) {
+            component.__owl__.pvnode.elm = component.__owl__.vnode!.elm;
+          }
         } else {
           component.__patch(document.createElement(fiber.vnode!.sel!), fiber.vnode!);
           component.__owl__.pvnode!.elm = component.__owl__.vnode!.elm;
diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts
index d3dafbc6..3500c2b7 100644
--- a/tests/component/component.test.ts
+++ b/tests/component/component.test.ts
@@ -3015,6 +3015,55 @@ describe("random stuff/miscellaneous", () => {
     expect(env.qweb.templates[Parent.template].fn.toString()).toMatchSnapshot();
     expect(fixture.innerHTML).toBe("<div><span>42</span></div>");
   });
+
+  test("update props of component without concrete own node", async () => {
+    class Custom extends Component {
+      static template = xml`
+        <div class="widget-subkey">
+          <t t-esc="props.key"/>__<t t-esc="props.subKey"/>
+        </div>`
+    }
+    class Child extends Component {
+      static components = { Custom };
+      static template = xml`
+        <t t-component="Custom"
+          t-key="props.subKey"
+          key="props.key"
+          subKey="props.subKey"/>`
+    }
+
+    class Parent extends Component {
+      static components = { Child };
+      static template = xml`
+        <div>
+          <Child t-key="childProps.key" t-props="childProps"/>
+        </div>`;
+      childProps = {
+        key: 1,
+        subKey: 1,
+      };
+    }
+    const parent = new Parent(null);
+    await parent.mount(fixture);
+    expect(fixture.textContent!.trim()).toBe('1__1');
+
+    // First step: change the Custom's instance
+    Object.assign(parent.childProps, {
+        subKey: 2,
+    });
+    parent.render();
+    await nextTick();
+    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,
+    });
+    parent.render();
+    await nextTick();
+    expect(fixture.textContent!.trim()).toBe('2__3');
+  });
 });
 
 describe("widget and observable state", () => {