From 9a5ff7a619b46c38a875852c574425eeb0071e33 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Mon, 17 Apr 2023 10:12:16 +0200
Subject: [PATCH] [FIX] components: solve missing update (concurrency issue)

Before this commit, in some specific situations, owl could skip updating
the DOM, even though it should. Here is what could happen:

- we have a parent component (A) and a child component (B)
- B depends on some reactive props from A
- we update some state that results in A being rendered, and B updated
- before this render is applied to the dom, we update some state again,
which causes A to be rendered again
- however, this new render is such that the B props are now identical,
so it will skip rendering B
- it will then apply the result of the render to the DOM => only A is
updated but B should also be updated!

The problem comes from the fact that Owl committed the props to the
component right after the rendering, to the new props were used in the
props comparison method to decide if B should be updated. This is
incorrect, we should always use the current props to decide what to do,
and only commit them to the component after it has been patched.
---
 src/runtime/component_node.ts                 |  5 +-
 .../__snapshots__/concurrency.test.ts.snap    | 25 +++++++
 tests/components/concurrency.test.ts          | 74 +++++++++++++++++++
 .../__snapshots__/shadow_dom.test.ts.snap     | 42 +++++++++++
 4 files changed, 144 insertions(+), 2 deletions(-)
 create mode 100644 tests/shadow_dom/__snapshots__/shadow_dom.test.ts.snap

diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts
index 4a3adf02..62128757 100644
--- a/src/runtime/component_node.ts
+++ b/src/runtime/component_node.ts
@@ -73,6 +73,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
   forceNextRender: boolean = false;
   parentKey: string | null;
   props: P;
+  nextProps: P | null = null;
 
   renderFn: Function;
   parent: ComponentNode | null;
@@ -220,7 +221,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
   }
 
   async updateAndRender(props: P, parentFiber: Fiber) {
-    const rawProps = props;
+    this.nextProps = props;
     props = Object.assign({}, props);
     // update
     const fiber = makeChildFiber(this, parentFiber);
@@ -245,7 +246,6 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
       return;
     }
     component.props = props;
-    this.props = rawProps;
     fiber.render();
     const parentRoot = parentFiber.root!;
     if (this.willPatch.length) {
@@ -327,6 +327,7 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
       // by the component will be patched independently in the appropriate
       // fiber.complete
       this._patch();
+      this.props = this.nextProps!;
     }
   }
   _patch() {
diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap
index 9ab6733a..a430f6c2 100644
--- a/tests/components/__snapshots__/concurrency.test.ts.snap
+++ b/tests/components/__snapshots__/concurrency.test.ts.snap
@@ -1252,6 +1252,31 @@ exports[`delayed render does not go through when t-component value changed 3`] =
 }"
 `;
 
+exports[`delayed render is not cancelled by upcoming render 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`B\`, true, false, false, [\\"state\\",\\"isEmpty\\"]);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return comp1({state: ctx['state'],isEmpty: ctx['state'].groups.length===0}, key + \`__1\`, node, this, null);
+  }
+}"
+`;
+
+exports[`delayed render is not cancelled by upcoming render 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(ctx['props'].state.groups.length);
+    const b3 = text(ctx['props'].state.config.test);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
 exports[`delayed rendering, but then initial rendering is cancelled by yet another render 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts
index ca93182d..c23ff6dc 100644
--- a/tests/components/concurrency.test.ts
+++ b/tests/components/concurrency.test.ts
@@ -4126,6 +4126,80 @@ test("delayed render does not go through when t-component value changed", async
   ]).toBeLogged();
 });
 
+test("delayed render is not cancelled by upcoming render", async () => {
+  let b: any;
+
+  class B extends Component {
+    static template = xml`
+        <t t-esc="props.state.groups.length"/>
+        <t t-esc="props.state.config.test"/>`;
+
+    setup() {
+      useLogLifecycle();
+      b = this;
+    }
+  }
+
+  class A extends Component {
+    static components = { B };
+    static template = xml`<B state="state" isEmpty="state.groups.length === 0"/>`;
+
+    state = useState({ groups: [], config: { test: "initial" } });
+    setup() {
+      useLogLifecycle();
+    }
+  }
+
+  await mount(A, fixture);
+
+  expect([
+    "A:setup",
+    "A:willStart",
+    "A:willRender",
+    "B:setup",
+    "B:willStart",
+    "A:rendered",
+    "B:willRender",
+    "B:rendered",
+    "B:mounted",
+    "A:mounted",
+  ]).toBeLogged();
+
+  expect(fixture.innerHTML).toBe("0initial");
+
+  b.props.state.config.test = "red";
+  b.props.state.groups.push(1);
+  expect([]).toBeLogged();
+  await Promise.resolve();
+  await Promise.resolve();
+  await Promise.resolve();
+  await Promise.resolve();
+  await Promise.resolve();
+  b.props.state.config.test = "black";
+  b.props.state.groups.push(1);
+  expect([
+    "A:willRender",
+    "B:willUpdateProps",
+    "A:rendered",
+    "B:willRender",
+    "B:rendered",
+  ]).toBeLogged();
+
+  await nextTick();
+  expect(fixture.innerHTML).toBe("2black");
+  expect([
+    "A:willRender",
+    "B:willUpdateProps",
+    "A:rendered",
+    "B:willRender",
+    "B:rendered",
+    "A:willPatch",
+    "B:willPatch",
+    "B:patched",
+    "A:patched",
+  ]).toBeLogged();
+});
+
 //   test.skip("components with shouldUpdate=false", async () => {
 //     const state = { p: 1, cc: 10 };
 
diff --git a/tests/shadow_dom/__snapshots__/shadow_dom.test.ts.snap b/tests/shadow_dom/__snapshots__/shadow_dom.test.ts.snap
new file mode 100644
index 00000000..60644d42
--- /dev/null
+++ b/tests/shadow_dom/__snapshots__/shadow_dom.test.ts.snap
@@ -0,0 +1,42 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`shadow_dom can bind event handler 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<button block-handler-0=\\"click\\">Click</button>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let hdlr1 = [ctx['add'], ctx];
+    return block1([hdlr1]);
+  }
+}"
+`;
+
+exports[`shadow_dom can mount app 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div class=\\"my-div\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`shadow_dom useRef hook 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div class=\\"my-div\\" block-ref=\\"0\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let ref1 = (el) => this.__owl__.setRef((\`refName\`), el);
+    return block1([ref1]);
+  }
+}"
+`;