From b1a3b322eeafb6a42f9565af7c592d310fa10860 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Wed, 19 Jul 2023 16:54:56 +0200
Subject: [PATCH] [FIX] app: sync destroy everythig when app is destroyed

Before this commit, the App destroy method would not destroy everything
synchronously. The code scheduled a flush, which is asynchronous, to
finally clean up the task lists, and destroy the cancelled nodes. But
this is not really good for tests: we need a stronger guarantee that
nothing will happen after the destroy.

With this commit, we simply call the processTask method immediately
after destroying the root component, so all cancelled nodes will be
destroyed immediately.
---
 src/runtime/app.ts                       |  2 +-
 tests/app/__snapshots__/app.test.ts.snap | 28 +++++++++++++
 tests/app/app.test.ts                    | 52 +++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/src/runtime/app.ts b/src/runtime/app.ts
index d12bc624..3abba10f 100644
--- a/src/runtime/app.ts
+++ b/src/runtime/app.ts
@@ -136,8 +136,8 @@ export class App<
 
   destroy() {
     if (this.root) {
-      this.scheduler.flush();
       this.root.destroy();
+      this.scheduler.processTasks();
     }
     window.__OWL_DEVTOOLS__.apps.delete(this);
   }
diff --git a/tests/app/__snapshots__/app.test.ts.snap b/tests/app/__snapshots__/app.test.ts.snap
index bbb6fabe..309c2dda 100644
--- a/tests/app/__snapshots__/app.test.ts.snap
+++ b/tests/app/__snapshots__/app.test.ts.snap
@@ -15,6 +15,34 @@ exports[`app App supports env with getters/setters 1`] = `
 }"
 `;
 
+exports[`app app: clear scheduler tasks and destroy cancelled nodes immediately on destroy 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`B\`, true, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    b2 = text(\`A\`);
+    if (ctx['state'].value) {
+      b3 = comp1({}, key + \`__1\`, node, this, null);
+    }
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`app app: clear scheduler tasks and destroy cancelled nodes immediately on destroy 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`B\`);
+  }
+}"
+`;
+
 exports[`app can configure an app with props 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/app/app.test.ts b/tests/app/app.test.ts
index ec2f5eee..ce219e4f 100644
--- a/tests/app/app.test.ts
+++ b/tests/app/app.test.ts
@@ -1,6 +1,14 @@
-import { App, Component, mount, xml } from "../../src";
+import { App, Component, mount, onWillStart, useState, xml } from "../../src";
 import { status } from "../../src/runtime/status";
-import { makeTestFixture, snapshotEverything, nextTick, elem } from "../helpers";
+import {
+  makeTestFixture,
+  snapshotEverything,
+  nextTick,
+  elem,
+  useLogLifecycle,
+  makeDeferred,
+  nextMicroTick,
+} from "../helpers";
 
 let fixture: HTMLElement;
 
@@ -94,4 +102,44 @@ describe("app", () => {
     expect(iframeDoc.contains(div)).toBe(false);
     expect(status(comp)).toBe("destroyed");
   });
+
+  test("app: clear scheduler tasks and destroy cancelled nodes immediately on destroy", async () => {
+    let def = makeDeferred();
+    class B extends Component {
+      static template = xml`B`;
+      setup() {
+        useLogLifecycle();
+        onWillStart(() => def);
+      }
+    }
+    class A extends Component {
+      static template = xml`A<t t-if="state.value"><B/></t>`;
+      static components = { B };
+      state = useState({ value: false });
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    const app = new App(A);
+    const comp = await app.mount(fixture);
+    expect(["A:setup", "A:willStart", "A:willRender", "A:rendered", "A:mounted"]).toBeLogged();
+
+    comp.state.value = true;
+    await nextTick();
+    expect(["A:willRender", "B:setup", "B:willStart", "A:rendered"]).toBeLogged();
+
+    // rerender to force the instantiation of a new B component (and cancelling the first)
+    comp.render();
+    await nextMicroTick();
+    expect(["A:willRender", "B:setup", "B:willStart", "A:rendered"]).toBeLogged();
+
+    app.destroy();
+    expect([
+      "A:willUnmount",
+      "B:willDestroy",
+      "A:willDestroy",
+      "B:willDestroy", // make sure the 2 B instances have been destroyed synchronously
+    ]).toBeLogged();
+  });
 });