From fd13277e1d0a4eb5d75902945871337a55907127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 31 Mar 2022 13:48:45 +0200 Subject: [PATCH] [FIX] component: protect against user code executing in critical section Canceling a fiber may cause user code to be run, which means that some new renderings could be scheduled, but this could interfere with the current renderings! --- src/component/component_node.ts | 6 +- src/component/fibers.ts | 5 ++ .../__snapshots__/concurrency.test.ts.snap | 40 +++++++++ tests/components/concurrency.test.ts | 88 +++++++++++++++++++ 4 files changed, 135 insertions(+), 4 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index 2e1b4ad9..c82e6129 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -79,10 +79,8 @@ export function component

( let node: any = ctx.children[key]; let isDynamic = typeof name !== "string"; - if (node) { - if (node.status === STATUS.DESTROYED) { - node = undefined; - } + if (node && node.status === STATUS.DESTROYED) { + node = undefined; } if (isDynamic && node && node.component.constructor !== name) { node = undefined; diff --git a/src/component/fibers.ts b/src/component/fibers.ts index e65082d5..0ff9eb6f 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -16,7 +16,12 @@ export function makeRootFiber(node: ComponentNode): Fiber { let current = node.fiber; if (current) { let root = current.root!; + // lock root fiber because canceling children fibers may destroy components, + // which means any arbitrary code can be run in onWillDestroy, which may + // trigger new renderings + root.locked = true; root.setCounter(root.counter + 1 - cancelFibers(current.children)); + root.locked = false; current.children = []; current.childrenMap = {}; current.bdom = null; diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index 75c66357..3b1bee88 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -1289,6 +1289,46 @@ exports[`delayed rendering, then component is destroyed and stuff 3`] = ` }" `; +exports[`destroyed component causes other soon to be destroyed component to rerender, weird stuff happens 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(\` A \`); + if (ctx['state'].flag) { + const b4 = component(\`B\`, {value: ctx['state'].valueB}, key + \`__1\`, node, ctx); + const b5 = component(\`C\`, {value: ctx['state'].valueC}, key + \`__2\`, node, ctx); + b3 = multi([b4, b5]); + } + return multi([b2, b3]); + } +}" +`; + +exports[`destroyed component causes other soon to be destroyed component to rerender, weird stuff happens 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(ctx['props'].value); + } +}" +`; + +exports[`destroyed component causes other soon to be destroyed component to rerender, weird stuff happens 3`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return text(ctx['state'].val+ctx['props'].value); + } +}" +`; + exports[`destroying/recreating a subcomponent, other scenario 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index 48349d1b..b4526933 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -4,6 +4,7 @@ import { mount, onMounted, onRendered, + onWillDestroy, onWillStart, onWillUnmount, onWillUpdateProps, @@ -3695,6 +3696,93 @@ test("another scenario with delayed rendering", async () => { ]).toBeLogged(); }); +test("destroyed component causes other soon to be destroyed component to rerender, weird stuff happens", async () => { + let def = makeDeferred(); + let c: any = null; + + class B extends Component { + static template = xml``; + setup() { + useLogLifecycle(); + onRendered(() => { + def.resolve(); + }); + onWillDestroy(() => { + c.state.val++; + c.render(); + }); + } + } + class C extends Component { + static template = xml``; + state = useState({ val: 0 }); + setup() { + c = this; + useLogLifecycle(); + } + } + + class A extends Component { + static template = xml` + A + + + + `; + static components = { B, C }; + state = useState({ flag: false, valueB: 1, valueC: 2 }); + setup() { + useLogLifecycle(); + } + } + + const parent = await mount(A, fixture); + expect(fixture.innerHTML).toBe(" A "); + expect(["A:setup", "A:willStart", "A:willRender", "A:rendered", "A:mounted"]).toBeLogged(); + + // initiate a render in A, but is blocked in B + parent.state.flag = true; + + await def; + await nextMicroTick(); + expect([ + "A:willRender", + "B:setup", + "B:willStart", + "C:setup", + "C:willStart", + "A:rendered", + "B:willRender", + "B:rendered", + "C:willRender", + "C:rendered", + ]).toBeLogged(); + + // initiate render in A => will cancel renders in B/C and restarts + parent.state.valueB = 2; + await nextTick(); + expect([ + "B:willDestroy", + "C:willDestroy", + "A:willRender", + "B:setup", + "B:willStart", + "C:setup", + "C:willStart", + "A:rendered", + "B:willRender", + "B:rendered", + "C:willRender", + "C:rendered", + "A:willPatch", + "C:mounted", + "B:mounted", + "A:patched", + ]).toBeLogged(); + + expect(fixture.innerHTML).toBe(" A 22"); +}); + // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 };