[FIX] component: protect against errors in onWillDestroy

src/component/component_node.ts

@@ -80,10 +80,7 @@ export function component<P extends object>(
   let isDynamic = typeof name !== "string";
 
   if (node) {
-    if (node.status < STATUS.MOUNTED) {
+    if (node.status === STATUS.DESTROYED) {
-      node.destroy();
-      node = undefined;
-    } else if (node.status === STATUS.DESTROYED) {
       node = undefined;
     }
   }
@@ -194,7 +191,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
 
   async render(deep: boolean = false) {
     let current = this.fiber;
-    if (current && current.root!.locked) {
+    if (current && (current.root!.locked || (current as any).bdom === true)) {
       await Promise.resolve();
       // situation may have changed after the microtask tick
       current = this.fiber;
@@ -216,6 +213,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     const fiber = makeRootFiber(this);
     fiber.deep = deep;
     this.fiber = fiber;
+
     this.app.scheduler.addFiber(fiber);
     await Promise.resolve();
     if (this.status === STATUS.DESTROYED) {
@@ -255,9 +253,15 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     for (let child of Object.values(this.children)) {
       child._destroy();
     }
+    if (this.willDestroy.length) {
+      try {
         for (let cb of this.willDestroy) {
           cb.call(component);
         }
+      } catch (e) {
+        handleError({ error: e, node: this });
+      }
+    }
     this.status = STATUS.DESTROYED;
   }
 

src/component/error_handling.ts

@@ -5,7 +5,7 @@ import type { Fiber } from "./fibers";
 export const fibersInError: WeakMap<Fiber, any> = new WeakMap();
 export const nodeErrorHandlers: WeakMap<ComponentNode, ((error: any) => void)[]> = new WeakMap();
 
-function _handleError(node: ComponentNode | null, error: any, isFirstRound = false): boolean {
+function _handleError(node: ComponentNode | null, error: any): boolean {
   if (!node) {
     return false;
   }
@@ -16,23 +16,19 @@ function _handleError(node: ComponentNode | null, error: any, isFirstRound = fal
 
   const errorHandlers = nodeErrorHandlers.get(node);
   if (errorHandlers) {
-    let stopped = false;
+    let handled = false;
     // execute in the opposite order
     for (let i = errorHandlers.length - 1; i >= 0; i--) {
       try {
         errorHandlers[i](error);
-        stopped = true;
+        handled = true;
         break;
       } catch (e) {
         error = e;
       }
     }
 
-    if (stopped) {
+    if (handled) {
-      if (isFirstRound && fiber && fiber.node.fiber) {
-        const root = fiber.root!;
-        root.setCounter(root.counter - 1);
-      }
       return true;
     }
   }
@@ -55,7 +51,7 @@ export function handleError(params: ErrorParams) {
 
   fibersInError.set(fiber.root!, error);
 
-  const handled = _handleError(node, error, true);
+  const handled = _handleError(node, error);
   if (!handled) {
     console.warn(`[Owl] Unhandled error. Destroying the root component`);
     try {

src/component/fibers.ts

@@ -124,11 +124,12 @@ export class Fiber {
     const root = this.root;
     if (root) {
       try {
+        (this.bdom as any) = true;
         this.bdom = node.renderFn();
-        root.setCounter(root.counter - 1);
       } catch (e) {
         handleError({ node, error: e });
       }
+      root.setCounter(root.counter - 1);
     }
   }
 }

tests/components/__snapshots__/error_handling.test.ts.snap

@@ -85,6 +85,64 @@ exports[`basics simple catchError 2`] = `
 }"
 `;
 
+exports[`can catch errors an error in onWillDestroy 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    b2 = text(ctx['state'].value);
+    if (ctx['state'].hasChild) {
+      b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx);
+    }
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors an error in onWillDestroy 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>abc</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors an error in onWillDestroy, variation 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    b2 = text(ctx['state'].value);
+    if (ctx['state'].hasChild) {
+      b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx);
+    }
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors an error in onWillDestroy, variation 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>abc</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`can catch errors calling a hook outside setup should crash 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/concurrency.test.ts

@@ -172,6 +172,11 @@ test("destroying/recreating a subcomponent, other scenario", async () => {
 
   await nextTick();
   expect([
+    "Parent:willRender",
+    "Child:setup",
+    "Child:willStart",
+    "Parent:rendered",
+    "Child:willDestroy",
     "Parent:willRender",
     "Child:setup",
     "Child:willStart",

tests/components/error_handling.test.ts

@@ -1,4 +1,4 @@
-import { Component, mount } from "../../src";
+import { Component, mount, onWillDestroy } from "../../src";
 import {
   onError,
   onMounted,
@@ -1197,4 +1197,126 @@ describe("can catch errors", () => {
     expect(fixture.innerHTML).toBe("<div>Child 2</div>");
     expect(steps).toEqual(["Error Component"]);
   });
+
+  test("an error in onWillDestroy", async () => {
+    class Child extends Component {
+      static template = xml`<div>abc</div>`;
+      setup() {
+        useLogLifecycle();
+        onWillDestroy(() => {
+          throw new Error("boom");
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <t t-esc="state.value"/>
+        <t t-if="state.hasChild"><Child/></t>`;
+      static components = { Child };
+
+      state = useState({ value: 1, hasChild: true });
+      setup() {
+        useLogLifecycle();
+        onError(() => {
+          this.state.value++;
+        });
+      }
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("1<div>abc</div>");
+    expect([
+      "Parent:setup",
+      "Parent:willStart",
+      "Parent:willRender",
+      "Child:setup",
+      "Child:willStart",
+      "Parent:rendered",
+      "Child:willRender",
+      "Child:rendered",
+      "Child:mounted",
+      "Parent:mounted",
+    ]).toBeLogged();
+    parent.state.hasChild = false;
+    await nextTick();
+    await nextTick();
+    await nextTick();
+    await nextTick();
+    expect([
+      "Parent:willRender",
+      "Parent:rendered",
+      "Parent:willPatch",
+      "Child:willUnmount",
+      "Child:willDestroy",
+      "Parent:willRender",
+      "Parent:rendered",
+      "Parent:willPatch",
+      "Parent:patched",
+    ]).toBeLogged();
+    expect(fixture.innerHTML).toBe("2");
+  });
+
+  test("an error in onWillDestroy, variation", async () => {
+    class Child extends Component {
+      static template = xml`<div>abc</div>`;
+      setup() {
+        useLogLifecycle();
+        onWillDestroy(() => {
+          throw new Error("boom");
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <t t-esc="state.value"/>
+        <t t-if="state.hasChild"><Child/></t>`;
+      static components = { Child };
+
+      state = useState({ value: 1, hasChild: false });
+      setup() {
+        useLogLifecycle();
+        onError(() => {
+          this.state.value++;
+        });
+      }
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("1");
+
+    expect([
+      "Parent:setup",
+      "Parent:willStart",
+      "Parent:willRender",
+      "Parent:rendered",
+      "Parent:mounted",
+    ]).toBeLogged();
+
+    parent.state.hasChild = true;
+    await nextMicroTick();
+    await nextMicroTick();
+    await nextMicroTick();
+    await nextMicroTick();
+    await nextMicroTick();
+    expect([
+      "Parent:willRender",
+      "Child:setup",
+      "Child:willStart",
+      "Parent:rendered",
+      "Child:willRender",
+      "Child:rendered",
+    ]).toBeLogged();
+    parent.state.hasChild = false;
+    await nextTick();
+    expect([
+      "Child:willDestroy",
+      "Parent:willRender",
+      "Parent:rendered",
+      "Parent:willPatch",
+      "Parent:patched",
+    ]).toBeLogged();
+    expect(fixture.innerHTML).toBe("2");
+  });
 });