[FIX] runtime: make error recovery more robust

The error recovery process is necessary, but is particularly subtle. An
error may occur basically any time some user code is called, which may
leave the internal owl state in an invalid situation.

In this commit, we try to improve Owl behaviour when a crash occurs in a
onMounted hook. Before this commit, it was possible for other component
mounted hooks to be called even though the component was not mounted, or
for willUnmount hooks to be called even though the component was
mounted, but its onMounted hooks had not been called.

All these situations need to be handled when we catch an error. In this
commit, we try to clean more internal state in the error handling code,
to make Owl more consistent.

src/runtime/fibers.ts

@@ -30,6 +30,13 @@ export function makeRootFiber(node: ComponentNode): Fiber {
       fibersInError.delete(current);
       fibersInError.delete(root);
       current.appliedToDom = false;
+      if (current instanceof RootFiber) {
+        // it is possible that this fiber is a fiber that crashed while being
+        // mounted, so the mounted list is possibly corrupted. We restore it to
+        // its normal initial state (which is empty list or a list with a mount
+        // fiber.
+        current.mounted = current instanceof MountFiber ? [current] : [];
+      }
     }
     return current;
   }
@@ -152,6 +159,7 @@ export class RootFiber extends Fiber {
     const node = this.node;
     this.locked = true;
     let current: Fiber | undefined = undefined;
+    let mountedFibers = this.mounted;
     try {
       // Step 1: calling all willPatch lifecycle hooks
       for (current of this.willPatch) {
@@ -173,7 +181,6 @@ export class RootFiber extends Fiber {
       this.locked = false;
 
       // Step 4: calling all mounted lifecycle hooks
-      let mountedFibers = this.mounted;
       while ((current = mountedFibers.pop())) {
         current = current;
         if (current.appliedToDom) {
@@ -194,6 +201,15 @@ export class RootFiber extends Fiber {
         }
       }
     } catch (e) {
+      // if mountedFibers is not empty, this means that a crash occured while
+      // calling the mounted hooks of some component. So, there may still be
+      // some component that have been mounted, but for which the mounted hooks
+      // have not been called. Here, we remove the willUnmount hooks for these
+      // specific component to prevent a worse situation (willUnmount being
+      // called even though mounted has not been called)
+      for (let fiber of mountedFibers) {
+        fiber.node.willUnmount = [];
+      }
       this.locked = false;
       node.app.handleError({ fiber: current || this, error: e });
     }

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

@@ -1191,6 +1191,135 @@ exports[`can catch errors error in mounted on a component with a sibling (proper
 }"
 `;
 
+exports[`can catch errors error in onMounted, graceful recovery 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(null, false, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const Comp1 = ctx['component'];
+    return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, []);
+  const comp2 = app.createComponent(\`Boom\`, true, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`parent\`);
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    const b4 = comp2({}, key + \`__2\`, node, this, null);
+    return multi([b2, b3, b4]);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`abc\`);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery 4`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`boom\`);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery 5`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`def\`);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery, variation 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(null, false, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2, b3;
+    b2 = text(\`R\`);
+    if (ctx['state'].gogogo) {
+      const Comp1 = ctx['component'];
+      b3 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
+    }
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery, variation 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, []);
+  const comp2 = app.createComponent(\`Boom\`, true, false, false, []);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`parent\`);
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    const b4 = comp2({}, key + \`__2\`, node, this, null);
+    return multi([b2, b3, b4]);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery, variation 4`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`abc\`);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery, variation 5`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`boom\`);
+  }
+}"
+`;
+
+exports[`can catch errors error in onMounted, graceful recovery, variation 6`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`def\`);
+  }
+}"
+`;
+
 exports[`can catch errors onError in class inheritance is called if rethrown 1`] = `
 "function anonymous(app, bdom, helpers
 ) {

tests/components/error_handling.test.ts

@@ -1678,4 +1678,198 @@ describe("can catch errors", () => {
     `);
     expect(fixture.innerHTML).toBe("2");
   });
+
+  test("error in onMounted, graceful recovery", async () => {
+    class Child extends Component {
+      static template = xml`abc`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class OtherChild extends Component {
+      static template = xml`def`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class Boom extends Component {
+      static template = xml`boom`;
+      setup() {
+        useLogLifecycle();
+        onMounted(() => {
+          throw new Error("boom");
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`parent<Child/><Boom/>`;
+      static components = { Child, Boom };
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class Root extends Component {
+      static template = xml`<t t-component="component"/>`;
+
+      component: any = Parent;
+      setup() {
+        useLogLifecycle();
+        onError(() => {
+          logStep("error");
+          this.component = OtherChild;
+          this.render();
+        });
+      }
+    }
+
+    await mount(Root, fixture);
+    expect(fixture.innerHTML).toBe("def");
+
+    expect(steps.splice(0)).toMatchInlineSnapshot(`
+      Array [
+        "Root:setup",
+        "Root:willStart",
+        "Root:willRender",
+        "Parent:setup",
+        "Parent:willStart",
+        "Root:rendered",
+        "Parent:willRender",
+        "Child:setup",
+        "Child:willStart",
+        "Boom:setup",
+        "Boom:willStart",
+        "Parent:rendered",
+        "Child:willRender",
+        "Child:rendered",
+        "Boom:willRender",
+        "Boom:rendered",
+        "Boom:mounted",
+        "error",
+        "Root:willRender",
+        "OtherChild:setup",
+        "OtherChild:willStart",
+        "Root:rendered",
+        "OtherChild:willRender",
+        "OtherChild:rendered",
+        "OtherChild:mounted",
+        "Root:mounted",
+      ]
+    `);
+  });
+
+  test("error in onMounted, graceful recovery, variation", async () => {
+    class Child extends Component {
+      static template = xml`abc`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class OtherChild extends Component {
+      static template = xml`def`;
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class Boom extends Component {
+      static template = xml`boom`;
+      setup() {
+        useLogLifecycle();
+        onMounted(() => {
+          throw new Error("boom");
+        });
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`parent<Child/><Boom/>`;
+      static components = { Child, Boom };
+      setup() {
+        useLogLifecycle();
+      }
+    }
+
+    class Root extends Component {
+      static template = xml`R<t t-if="state.gogogo" t-component="component"/>`;
+
+      component: any = Parent;
+      state = useState({ gogogo: false });
+
+      setup() {
+        useLogLifecycle();
+        onError(() => {
+          logStep("error");
+          this.component = OtherChild;
+          this.render();
+        });
+      }
+    }
+
+    const root = await mount(Root, fixture);
+    expect(fixture.innerHTML).toBe("R");
+
+    // standard mounting process
+    expect(steps.splice(0)).toMatchInlineSnapshot(`
+      Array [
+        "Root:setup",
+        "Root:willStart",
+        "Root:willRender",
+        "Root:rendered",
+        "Root:mounted",
+      ]
+    `);
+
+    root.state.gogogo = true;
+    await nextTick();
+
+    expect(fixture.innerHTML).toBe("Rparentabcboom");
+    // rerender, root creates sub components, it crashes, tries to recover
+    expect(steps.splice(0)).toMatchInlineSnapshot(`
+      Array [
+        "Root:willRender",
+        "Parent:setup",
+        "Parent:willStart",
+        "Root:rendered",
+        "Parent:willRender",
+        "Child:setup",
+        "Child:willStart",
+        "Boom:setup",
+        "Boom:willStart",
+        "Parent:rendered",
+        "Child:willRender",
+        "Child:rendered",
+        "Boom:willRender",
+        "Boom:rendered",
+        "Root:willPatch",
+        "Boom:mounted",
+        "error",
+        "Root:willRender",
+        "OtherChild:setup",
+        "OtherChild:willStart",
+        "Root:rendered",
+      ]
+    `);
+
+    await nextTick();
+    expect(fixture.innerHTML).toBe("Rdef");
+
+    expect(steps.splice(0)).toMatchInlineSnapshot(`
+      Array [
+        "OtherChild:willRender",
+        "OtherChild:rendered",
+        "Root:willPatch",
+        "Child:willDestroy",
+        "Boom:willUnmount",
+        "Boom:willDestroy",
+        "Parent:willDestroy",
+        "OtherChild:mounted",
+        "Root:patched",
+      ]
+    `);
+  });
 });