From d112e19979e56d504c0f3b8aa798002d1c21aa7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 28 Nov 2024 09:05:16 +0100 Subject: [PATCH] [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 | 18 +- .../__snapshots__/error_handling.test.ts.snap | 129 ++++++++++++ tests/components/error_handling.test.ts | 194 ++++++++++++++++++ 3 files changed, 340 insertions(+), 1 deletion(-) diff --git a/src/runtime/fibers.ts b/src/runtime/fibers.ts index 13b2124da..7dea4a466 100644 --- a/src/runtime/fibers.ts +++ b/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 }); } diff --git a/tests/components/__snapshots__/error_handling.test.ts.snap b/tests/components/__snapshots__/error_handling.test.ts.snap index 45f1bf43d..20ff00465 100644 --- a/tests/components/__snapshots__/error_handling.test.ts.snap +++ b/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 ) { diff --git a/tests/components/error_handling.test.ts b/tests/components/error_handling.test.ts index 703b29183..672544277 100644 --- a/tests/components/error_handling.test.ts +++ b/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`; + static components = { Child, Boom }; + setup() { + useLogLifecycle(); + } + } + + class Root extends Component { + static template = xml``; + + 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`; + static components = { Child, Boom }; + setup() { + useLogLifecycle(); + } + } + + class Root extends Component { + static template = xml`R`; + + 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", + ] + `); + }); });