[FIX] component: improve error handling

In the following situation: A parent of B, B parent of C, with an error
when C is mounted, caught by B and retriggering a rendering in B, then
the onMounted hook of A wasn't properly called. This commit fixes this
problem.

src/component/component_node.ts

@@ -221,6 +221,28 @@ export class ComponentNode<T extends typeof Component = typeof Component>
     this._render(fiber);
   }
 
+  /**
+   * Finds a child that has dom that is not yet updated, and update it. This
+   * method is meant to be used only in the context of repatching the dom after
+   * a mounted hook failed and was handled.
+   */
+  updateDom() {
+    if (this.bdom === this.fiber!.bdom) {
+      // If the error was handled by some child component, we need to find it to
+      // apply its change
+      for (let k in this.children) {
+        const child = this.children[k];
+        child.updateDom();
+      }
+    } else {
+      // if we get here, this is the component that handled the error and rerendered
+      // itself, so we can simply patch the dom
+      this.bdom!.patch(this.fiber!.bdom, false);
+      this.fiber!.appliedToDom = true;
+      this.fiber = null;
+    }
+  }
+
   // ---------------------------------------------------------------------------
   // Block DOM methods
   // ---------------------------------------------------------------------------

src/component/error_handling.ts

@@ -45,6 +45,14 @@ export function handleError(params: ErrorParams) {
   const node = "node" in params ? params.node : params.fiber.node;
   const fiber = "fiber" in params ? params.fiber : node.fiber!;
 
+  // resets the fibers on components if possible. This is important so that
+  // new renderings can be properly included in the initial one, if any.
+  let current: Fiber | null = fiber;
+  do {
+    current.node.fiber = current;
+    current = current.parent;
+  } while (current);
+
   fibersInError.set(fiber.root, error);
 
   const handled = _handleError(node, error, true);

src/component/fibers.ts

@@ -1,8 +1,7 @@
-import type { BDom } from "../blockdom";
+import { BDom, mount } from "../blockdom";
-import { mount } from "../blockdom";
 import type { ComponentNode } from "./component_node";
-import { STATUS } from "./status";
 import { fibersInError, handleError } from "./error_handling";
+import { STATUS } from "./status";
 
 export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber {
   let current = node.fiber;
@@ -30,6 +29,7 @@ export function makeRootFiber(node: ComponentNode): Fiber {
     if (fibersInError.has(current)) {
       fibersInError.delete(current);
       fibersInError.delete(root);
+      current.appliedToDom = false;
     }
     return current;
   }
@@ -168,12 +168,20 @@ export class MountFiber extends RootFiber {
     let current: Fiber | undefined = this;
     try {
       const node = this.node;
-      node.bdom = this.bdom;
+      if (node.bdom) {
-      if (this.position === "last-child" || this.target.childNodes.length === 0) {
+        // this is a complicated situation: if we mount a fiber with an existing
-        mount(node.bdom!, this.target);
+        // bdom, this means that this same fiber was already completed, mounted,
+        // but a crash occurred in some mounted hook. Then, it was handled and
+        // the new rendering is being applied.
+        node.updateDom();
       } else {
-        const firstChild = this.target.childNodes[0];
+        node.bdom = this.bdom;
-        mount(node.bdom!, this.target, firstChild);
+        if (this.position === "last-child" || this.target.childNodes.length === 0) {
+          mount(node.bdom!, this.target);
+        } else {
+          const firstChild = this.target.childNodes[0];
+          mount(node.bdom!, this.target, firstChild);
+        }
       }
       node.status = STATUS.MOUNTED;
       this.appliedToDom = true;

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

@@ -364,6 +364,101 @@ exports[`can catch errors can catch an error in the initial call of a component
 }"
 `;
 
+exports[`can catch errors can catch an error in the mounted call (in child of child) 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>Some text</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the mounted call (in child of child) 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  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(\`Error handled\`);
+    } else {
+      b3 = component(\`Boom\`, {}, key + \`__1\`, node, ctx);
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the mounted call (in child of child) 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div><block-child-0/></div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2 = component(\`C\`, {}, key + \`__1\`, node, ctx);
+    return block1([], [b2]);
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the mounted call (in child of child) 4`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return component(\`B\`, {}, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the mounted call (in root component) 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  let block1 = createBlock(\`<div>Some text</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`can catch errors can catch an error in the mounted call (in root component) 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  let { withDefault, getTemplate, prepareList, withKey, zero, call, callSlot, capture, isBoundary, shallowEqual, setContextValue, toNumber, safeOutput } = helpers;
+  
+  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(\`Error handled\`);
+    } else {
+      b3 = component(\`ErrorComponent\`, {}, key + \`__1\`, node, ctx);
+    }
+    return block1([], [b2, b3]);
+  }
+}"
+`;
+
 exports[`can catch errors can catch an error in the mounted call 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/error_handling.test.ts

@@ -9,7 +9,13 @@ import {
   useState,
 } from "../../src/index";
 import { xml } from "../../src/tags";
-import { makeTestFixture, nextTick, snapshotEverything } from "../helpers";
+import {
+  logStep,
+  makeTestFixture,
+  nextTick,
+  snapshotEverything,
+  useLogLifecycle,
+} from "../helpers";
 
 let fixture: HTMLElement;
 
@@ -550,7 +556,9 @@ describe("can catch errors", () => {
     class ErrorComponent extends Component {
       static template = xml`<div>Some text</div>`;
       setup() {
+        useLogLifecycle();
         onMounted(() => {
+          logStep("boom");
           throw new Error("NOOOOO");
         });
       }
@@ -563,20 +571,156 @@ describe("can catch errors", () => {
       state = useState({ error: false });
 
       setup() {
+        useLogLifecycle();
         onError(() => (this.state.error = true));
       }
     }
-    class App extends Component {
+    class Root extends Component {
       static template = xml`<div>
         <ErrorBoundary><ErrorComponent /></ErrorBoundary>
       </div>`;
       static components = { ErrorBoundary, ErrorComponent };
+      setup() {
+        useLogLifecycle();
+      }
     }
-    await mount(App, fixture);
+    await mount(Root, fixture);
-    await nextTick();
-    await nextTick();
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><div>Error handled</div></div>");
+    expect([
+      "Root:setup",
+      "Root:willStart",
+      "Root:willRender",
+      "ErrorBoundary:setup",
+      "ErrorBoundary:willStart",
+      "Root:rendered",
+      "ErrorBoundary:willRender",
+      "ErrorComponent:setup",
+      "ErrorComponent:willStart",
+      "ErrorBoundary:rendered",
+      "ErrorComponent:willRender",
+      "ErrorComponent:rendered",
+      "ErrorComponent:mounted",
+      "boom",
+      "ErrorBoundary:willRender",
+      "ErrorBoundary:rendered",
+      "ErrorBoundary:mounted",
+      "Root:mounted",
+    ]).toBeLogged();
+  });
+
+  test("can catch an error in the mounted call (in root component)", async () => {
+    class ErrorComponent extends Component {
+      static template = xml`<div>Some text</div>`;
+      setup() {
+        useLogLifecycle();
+        onMounted(() => {
+          logStep("boom");
+          throw new Error("NOOOOO");
+        });
+      }
+    }
+    class Root extends Component {
+      static template = xml`<div>
+       <t t-if="state.error">Error handled</t>
+       <t t-else=""><ErrorComponent /></t>
+      </div>`;
+      static components = { ErrorComponent };
+      state = useState({ error: false });
+
+      setup() {
+        useLogLifecycle();
+        onError(() => (this.state.error = true));
+      }
+    }
+    await mount(Root, fixture);
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div>Error handled</div>");
+    expect([
+      "Root:setup",
+      "Root:willStart",
+      "Root:willRender",
+      "ErrorComponent:setup",
+      "ErrorComponent:willStart",
+      "Root:rendered",
+      "ErrorComponent:willRender",
+      "ErrorComponent:rendered",
+      "ErrorComponent:mounted",
+      "boom",
+      "Root:willRender",
+      "Root:rendered",
+      "Root:mounted",
+    ]).toBeLogged();
+  });
+
+  test("can catch an error in the mounted call (in child of child)", async () => {
+    class Boom extends Component {
+      static template = xml`<div>Some text</div>`;
+      setup() {
+        useLogLifecycle();
+        onMounted(() => {
+          logStep("boom");
+          throw new Error("NOOOOO");
+        });
+      }
+    }
+
+    class C extends Component {
+      static template = xml`<div>
+       <t t-if="state.error">Error handled</t>
+       <t t-else=""><Boom/></t>
+      </div>`;
+      static components = { Boom };
+      state = useState({ error: false });
+
+      setup() {
+        useLogLifecycle();
+        onError(() => (this.state.error = true));
+      }
+    }
+
+    class B extends Component {
+      static template = xml`<div><C/></div>`;
+      static components = { C };
+      setup() {
+        useLogLifecycle();
+      }
+    }
+    class A extends Component {
+      static template = xml`<B/>`;
+      static components = { B };
+      setup() {
+        useLogLifecycle();
+      }
+    }
+    await mount(A, fixture);
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div><div>Error handled</div></div>");
+    expect([
+      "A:setup",
+      "A:willStart",
+      "A:willRender",
+      "B:setup",
+      "B:willStart",
+      "A:rendered",
+      "B:willRender",
+      "C:setup",
+      "C:willStart",
+      "B:rendered",
+      "C:willRender",
+      "Boom:setup",
+      "Boom:willStart",
+      "C:rendered",
+      "Boom:willRender",
+      "Boom:rendered",
+      "Boom:mounted",
+      "boom",
+      "C:willRender",
+      "C:rendered",
+      "C:mounted",
+      "B:mounted",
+      "A:mounted",
+    ]).toBeLogged();
   });
 
   test("can catch an error in the willPatch call", async () => {