From b8d09e523dd08f89181004fe7173d5e65e1b231c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 18 Oct 2024 14:51:03 +0200 Subject: [PATCH] [FIX] app: make subroots more robust This commit fixes two issues with subroots: 1. creating a subroot create a new component node synchronously. This would causes issues if the creation was done in the setup of a component, since in that case, owl would reset the current component to null right after, which would cause all calls to hooks to fail. This is fixed by restoring the previous component node right after creating a root. 2. the destroy method for roots calls the scheduler processTasks. However, the processTasks method was not safe to reentrant calls, which would in some cases crashes owl. For example, if a destroy is done while a new component is mounted, the mount method would be called twice. This is fixed by ignoring the processTasks if we are currently processing tasks. It works because the "for ... of" loop will still process all new tasks in the current iteration. --- src/runtime/app.ts | 7 +- src/runtime/component_node.ts | 7 ++ src/runtime/scheduler.ts | 6 ++ tests/app/__snapshots__/app.test.ts.snap | 27 +++++++ tests/app/__snapshots__/sub_root.test.ts.snap | 79 +++++++++++++++++++ tests/app/app.test.ts | 19 ++++- tests/app/sub_root.test.ts | 65 ++++++++++++++- 7 files changed, 205 insertions(+), 5 deletions(-) diff --git a/src/runtime/app.ts b/src/runtime/app.ts index 189ed5b3..9431e6e7 100644 --- a/src/runtime/app.ts +++ b/src/runtime/app.ts @@ -1,6 +1,6 @@ import { version } from "../version"; import { Component, ComponentConstructor, Props } from "./component"; -import { ComponentNode } from "./component_node"; +import { ComponentNode, saveCurrent } from "./component_node"; import { nodeErrorHandlers, handleError } from "./error_handling"; import { OwlError } from "../common/owl_error"; import { Fiber, RootFiber, MountOptions } from "./fibers"; @@ -52,7 +52,7 @@ declare global { } } -interface Root { +interface Root

{ node: ComponentNode; mount(target: HTMLElement | ShadowRoot, options?: MountOptions): Promise>; destroy(): void; @@ -120,7 +120,10 @@ export class App< if (config.env) { this.env = config.env as any; } + + const restore = saveCurrent(); const node = this.makeNode(Root, props); + restore(); if (config.env) { this.env = env; } diff --git a/src/runtime/component_node.ts b/src/runtime/component_node.ts index bffc0fc5..31d4d933 100644 --- a/src/runtime/component_node.ts +++ b/src/runtime/component_node.ts @@ -10,6 +10,13 @@ import { batched, Callback } from "./utils"; let currentNode: ComponentNode | null = null; +export function saveCurrent() { + let n = currentNode; + return () => { + currentNode = n; + }; +} + export function getCurrent(): ComponentNode { if (!currentNode) { throw new OwlError("No active component (a hook function should only be called in 'setup')"); diff --git a/src/runtime/scheduler.ts b/src/runtime/scheduler.ts index d5fed0a4..72079dfe 100644 --- a/src/runtime/scheduler.ts +++ b/src/runtime/scheduler.ts @@ -16,6 +16,7 @@ export class Scheduler { frame: number = 0; delayedRenders: Fiber[] = []; cancelledNodes: Set = new Set(); + processing = false; constructor() { this.requestAnimationFrame = Scheduler.requestAnimationFrame; @@ -53,6 +54,10 @@ export class Scheduler { } processTasks() { + if (this.processing) { + return; + } + this.processing = true; this.frame = 0; for (let node of this.cancelledNodes) { node._destroy(); @@ -66,6 +71,7 @@ export class Scheduler { this.tasks.delete(task); } } + this.processing = false; } processFiber(fiber: RootFiber) { diff --git a/tests/app/__snapshots__/app.test.ts.snap b/tests/app/__snapshots__/app.test.ts.snap index dcee7a5e..21d48b9c 100644 --- a/tests/app/__snapshots__/app.test.ts.snap +++ b/tests/app/__snapshots__/app.test.ts.snap @@ -43,6 +43,33 @@ exports[`app app: clear scheduler tasks and destroy cancelled nodes immediately }" `; +exports[`app can call processTask twice in a row without crashing 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`Child\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`parent\`); + const b3 = comp1({}, key + \`__1\`, node, this, null); + return multi([b2, b3]); + } +}" +`; + +exports[`app can call processTask twice in a row without crashing 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block1 = createBlock(\`

\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + exports[`app can configure an app with props 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/app/__snapshots__/sub_root.test.ts.snap b/tests/app/__snapshots__/sub_root.test.ts.snap index 7af7806b..efc4d563 100644 --- a/tests/app/__snapshots__/sub_root.test.ts.snap +++ b/tests/app/__snapshots__/sub_root.test.ts.snap @@ -1,5 +1,62 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`destroy a subroot while another component is mounted in main app 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + const comp1 = app.createComponent(\`ChildB\`, true, false, false, []); + const comp2 = app.createComponent(\`ChildA\`, true, false, false, []); + + return function template(ctx, node, key = \\"\\") { + let b2, b3; + if (ctx['state'].flag) { + b2 = comp1({}, key + \`__1\`, node, this, null); + } else { + b3 = comp2({}, key + \`__2\`, node, this, null); + } + return multi([b2, b3]); + } +}" +`; + +exports[`destroy a subroot while another component is mounted in main app 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block3 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`a\`); + let ref1 = (el) => this.__owl__.setRef((\`elem\`), el); + const b3 = block3([ref1]); + return multi([b2, b3]); + } +}" +`; + +exports[`destroy a subroot while another component is mounted in main app 3`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`c\`); + } +}" +`; + +exports[`destroy a subroot while another component is mounted in main app 4`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`b\`); + } +}" +`; + exports[`subroot by default, env is the same in sub root 1`] = ` "function anonymous(app, bdom, helpers ) { @@ -26,6 +83,28 @@ exports[`subroot by default, env is the same in sub root 2`] = ` }" `; +exports[`subroot can create a root in a setup function, then use a hook 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`a\`); + } +}" +`; + +exports[`subroot can create a root in a setup function, then use a hook 2`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(\`c\`); + } +}" +`; + exports[`subroot can mount subroot 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/app/app.test.ts b/tests/app/app.test.ts index 6eab991b..0635eef9 100644 --- a/tests/app/app.test.ts +++ b/tests/app/app.test.ts @@ -1,4 +1,4 @@ -import { App, Component, mount, onWillStart, useState, xml } from "../../src"; +import { App, Component, mount, onWillPatch, onWillStart, useState, xml } from "../../src"; import { status } from "../../src/runtime/status"; import { makeTestFixture, @@ -184,4 +184,21 @@ describe("app", () => { expect(Object.keys(app.templates)).toEqual(["hello"]); expect(Object.keys(app.rawTemplates)).toEqual(["hello", "world"]); }); + + test("can call processTask twice in a row without crashing", async () => { + class Child extends Component { + static template = xml`
`; + setup() { + onWillPatch(() => app.scheduler.processTasks()); + } + } + class SomeComponent extends Component { + static template = xml`parent`; + static components = { Child }; + } + + const app = new App(SomeComponent); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("parent
"); + }); }); diff --git a/tests/app/sub_root.test.ts b/tests/app/sub_root.test.ts index 2f2146dc..0cb75810 100644 --- a/tests/app/sub_root.test.ts +++ b/tests/app/sub_root.test.ts @@ -1,6 +1,6 @@ -import { App, Component, xml } from "../../src"; +import { App, Component, onMounted, onWillDestroy, useRef, useState, xml } from "../../src"; import { status } from "../../src/runtime/status"; -import { makeTestFixture, snapshotEverything } from "../helpers"; +import { makeTestFixture, nextTick, snapshotEverything } from "../helpers"; let fixture: HTMLElement; @@ -112,4 +112,65 @@ describe("subroot", () => { expect(status(comp)).not.toBe("destroyed"); expect(status(subcomp)).toBe("destroyed"); }); + + test("can create a root in a setup function, then use a hook", async () => { + class C extends Component { + static template = xml`c`; + } + + class A extends Component { + static template = xml`a`; + state: any; + setup() { + app.createRoot(C); + this.state = useState({ value: 1 }); + } + } + + const app = new App(A); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("a"); + }); +}); + +test("destroy a subroot while another component is mounted in main app", async () => { + class C extends Component { + static template = xml`c`; + } + + class ChildA extends Component { + static template = xml`a
`; + ref: any; + setup() { + this.ref = useRef("elem"); + let root = app.createRoot(C); + onMounted(() => { + root.mount(this.ref.el); + }); + onWillDestroy(() => { + root.destroy(); + }); + } + } + class ChildB extends Component { + static template = xml`b`; + } + + class SomeComponent extends Component { + static template = xml` + + + `; + static components = { ChildA, ChildB }; + state = useState({ flag: false }); + } + + const app = new App(SomeComponent); + const comp = await app.mount(fixture); + expect(fixture.innerHTML).toBe("a
"); + await nextTick(); + expect(fixture.innerHTML).toBe("a
c
"); + comp.state.flag = true; + await nextTick(); + expect(fixture.innerHTML).toBe("b"); });