Skip to content

Commit

Permalink
[FIX] app: make subroots more robust
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ged-odoo committed Oct 21, 2024
1 parent 04c2808 commit 25d0f58
Show file tree
Hide file tree
Showing 7 changed files with 205 additions and 5 deletions.
7 changes: 5 additions & 2 deletions src/runtime/app.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -52,7 +52,7 @@ declare global {
}
}

interface Root<P, E> {
interface Root<P extends Props, E> {
node: ComponentNode<P, E>;
mount(target: HTMLElement | ShadowRoot, options?: MountOptions): Promise<Component<P, E>>;
destroy(): void;
Expand Down Expand Up @@ -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;
}
Expand Down
7 changes: 7 additions & 0 deletions src/runtime/component_node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')");
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/scheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export class Scheduler {
frame: number = 0;
delayedRenders: Fiber[] = [];
cancelledNodes: Set<ComponentNode> = new Set();
processing = false;

constructor() {
this.requestAnimationFrame = Scheduler.requestAnimationFrame;
Expand Down Expand Up @@ -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();
Expand All @@ -66,6 +71,7 @@ export class Scheduler {
this.tasks.delete(task);
}
}
this.processing = false;
}

processFiber(fiber: RootFiber) {
Expand Down
27 changes: 27 additions & 0 deletions tests/app/__snapshots__/app.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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(\`<div/>\`);
return function template(ctx, node, key = \\"\\") {
return block1();
}
}"
`;
exports[`app can configure an app with props 1`] = `
"function anonymous(app, bdom, helpers
) {
Expand Down
79 changes: 79 additions & 0 deletions tests/app/__snapshots__/sub_root.test.ts.snap
Original file line number Diff line number Diff line change
@@ -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(\`<div block-ref=\\"0\\"/>\`);
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
) {
Expand All @@ -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
) {
Expand Down
19 changes: 18 additions & 1 deletion tests/app/app.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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`<div/>`;
setup() {
onWillPatch(() => app.scheduler.processTasks());
}
}
class SomeComponent extends Component {
static template = xml`parent<Child/>`;
static components = { Child };
}

const app = new App(SomeComponent);
await app.mount(fixture);
expect(fixture.innerHTML).toBe("parent<div></div>");
});
});
65 changes: 63 additions & 2 deletions tests/app/sub_root.test.ts
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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<div t-ref="elem"></div>`;
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`
<t t-if="state.flag"><ChildB/></t>
<t t-else=""><ChildA/></t>
`;
static components = { ChildA, ChildB };
state = useState({ flag: false });
}

const app = new App(SomeComponent);
const comp = await app.mount(fixture);
expect(fixture.innerHTML).toBe("a<div></div>");
await nextTick();
expect(fixture.innerHTML).toBe("a<div>c</div>");
comp.state.flag = true;
await nextTick();
expect(fixture.innerHTML).toBe("b");
});

0 comments on commit 25d0f58

Please sign in to comment.