[FIX] component: error handling in class inheritance

Before this commit, class inheritance when using the onError hook was unclear nay wrong.

After this commit, error handlers are called from the bottom up  in the inheritance hierarchy.
If a handler doesn't rethrow the error, the handling stops there and no other handler is called.
If a handler does rethrow, the handlers declared in a parent class are executed.

src/app/app.ts

@@ -1,9 +1,10 @@
-import { onError, onMounted } from "../component/lifecycle_hooks";
+import { onMounted } from "../component/lifecycle_hooks";
 import { Component } from "../component/component";
 import { ComponentNode } from "../component/component_node";
 import { MountOptions } from "../component/fibers";
 import { Scheduler } from "../component/scheduler";
 import { TemplateSet } from "./template_set";
+import { nodeErrorHandlers } from "../component/error_handling";
 
 // reimplement dev mode stuff see last change in 0f7a8289a6fb8387c3c1af41c6664b2a8448758f
 
@@ -67,7 +68,14 @@ export class App<T extends typeof Component = any> extends TemplateSet {
     const node = new ComponentNode(this.Root, this.props, this);
     const promise: any = new Promise((resolve, reject) => {
       onMounted(() => resolve(node.component));
-      onError((e) => {
+
+      // Manually add the last resort error handler on the node
+      let handlers = nodeErrorHandlers.get(node);
+      if (!handlers) {
+        handlers = [];
+        nodeErrorHandlers.set(node, handlers);
+      }
+      handlers.unshift((e) => {
         reject(e);
         throw e;
       });

src/component/error_handling.ts

@@ -21,9 +21,10 @@ function _handleError(node: ComponentNode | null, error: any, isFirstRound = fal
     }
 
     let stopped = false;
-    for (const h of errorHandlers) {
+    // execute in the opposite order
+    for (let i = errorHandlers.length - 1; i >= 0; i--) {
       try {
-        h(error);
+        errorHandlers[i](error);
         stopped = true;
         break;
       } catch (e) {

src/component/lifecycle_hooks.ts

@@ -59,14 +59,13 @@ export function onRendered(fn: () => void | any) {
   };
 }
 
-export function onError(fn: (error: Error) => void | any) {
+type OnErrorCallback = (error: any) => void | any;
+export function onError(callback: OnErrorCallback) {
   const node = getCurrent()!;
   let handlers = nodeErrorHandlers.get(node);
-  if (handlers) {
-    handlers.push(fn);
-  } else {
+  if (!handlers) {
     handlers = [];
-    handlers.push(fn);
     nodeErrorHandlers.set(node, handlers);
   }
+  handlers.push(callback);
 }

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

@@ -768,6 +768,66 @@ exports[`can catch errors error in mounted on a component with a sibling (proper
 }"
 `;
 
+exports[`can catch errors onError in class inheritance is called if rethrown 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div><block-child-0/><block-child-1/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    if (!ctx['state'].error) {
+      b2 = text(this.will.crash);
+    } else {
+      b3 = text(ctx['state'].error);
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors onError in class inheritance is called if rethrown 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`Concrete\`, {}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`can catch errors onError in class inheritance is not called if no rethrown 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div><block-child-0/><block-child-1/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b3;
+    if (!ctx['state'].error) {
+      b2 = text(this.will.crash);
+    } else {
+      b3 = text(ctx['state'].error);
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors onError in class inheritance is not called if no rethrown 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`Concrete\`, {}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
 exports[`errors and promises a rendering error in a sub component will reject the mount promise 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/error_handling.test.ts

@@ -870,4 +870,91 @@ describe("can catch errors", () => {
     await mount(Parent, fixture);
     expect(fixture.innerHTML).toBe("<div>Error</div>");
   });
+
+  test("onError in class inheritance is not called if no rethrown", async () => {
+    const steps: string[] = [];
+
+    class Abstract extends Component {
+      static template = xml`<div>
+          <t t-if="!state.error">
+            <t t-esc="this.will.crash" />
+          </t>
+          <t t-else="">
+            <t t-esc="state.error"/>
+          </t>
+        </div>`;
+      state: any;
+      setup() {
+        this.state = useState({});
+        onError(() => {
+          steps.push("Abstract onError");
+          this.state.error = "Abstract";
+        });
+      }
+    }
+
+    class Concrete extends Abstract {
+      setup() {
+        super.setup();
+        onError(() => {
+          steps.push("Concrete onError");
+          this.state.error = "Concrete";
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static components = { Concrete };
+      static template = xml`<Concrete />`;
+    }
+
+    await mount(Parent, fixture);
+
+    expect(steps).toStrictEqual(["Concrete onError"]);
+    expect(fixture.innerHTML).toBe("<div>Concrete</div>");
+  });
+
+  test("onError in class inheritance is called if rethrown", async () => {
+    const steps: string[] = [];
+
+    class Abstract extends Component {
+      static template = xml`<div>
+          <t t-if="!state.error">
+            <t t-esc="this.will.crash" />
+          </t>
+          <t t-else="">
+            <t t-esc="state.error"/>
+          </t>
+        </div>`;
+      state: any;
+      setup() {
+        this.state = useState({});
+        onError(() => {
+          steps.push("Abstract onError");
+          this.state.error = "Abstract";
+        });
+      }
+    }
+
+    class Concrete extends Abstract {
+      setup() {
+        super.setup();
+        onError((error) => {
+          steps.push("Concrete onError");
+          this.state.error = "Concrete";
+          throw error;
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static components = { Concrete };
+      static template = xml`<Concrete />`;
+    }
+
+    await mount(Parent, fixture);
+
+    expect(steps).toStrictEqual(["Concrete onError", "Abstract onError"]);
+    expect(fixture.innerHTML).toBe("<div>Abstract</div>");
+  });
 });