From 3883cec07985f6ea15964f4661aef1c4bcae18d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Fri, 2 Sep 2022 09:50:31 +0200
Subject: [PATCH] [FIX] slots: prevent crash when using same slot in different
 locations

Before this commit, a crash could occur when a component with no props
is defined in a slot, and that slot is conditionally displayed in
multiple locations.

The reason for that is that the key provided to the callSlot function
was identical, so from the perspective of the component function, it was
not possible to make the difference between a component located in
either places.  With this commit, we make sure that a unique key is used
when a slot is reused in a template (or if it is dynamic, because in
that case, we have no idea at compile time if it will be unique or not)

closes #1246
---
 src/compiler/code_generator.ts                |  16 +-
 .../__snapshots__/slots.test.ts.snap          | 180 +++++++++++++++++-
 tests/components/slots.test.ts                |  98 ++++++++++
 3 files changed, 286 insertions(+), 8 deletions(-)

diff --git a/src/compiler/code_generator.ts b/src/compiler/code_generator.ts
index 1013d13f..e4bb897f 100644
--- a/src/compiler/code_generator.ts
+++ b/src/compiler/code_generator.ts
@@ -232,6 +232,7 @@ export class CodeGenerator {
   translatableAttributes: string[] = TRANSLATABLE_ATTRS;
   ast: AST;
   staticDefs: { id: string; expr: string }[] = [];
+  slotNames: Set<String> = new Set();
   helpers: Set<string> = new Set();
 
   constructor(ast: AST, options: CodeGenOptions) {
@@ -1245,28 +1246,37 @@ export class CodeGenerator {
     let blockString: string;
     let slotName;
     let dynamic = false;
+    let isMultiple = false;
     if (ast.name.match(INTERP_REGEXP)) {
       dynamic = true;
+      isMultiple = true;
       slotName = interpolate(ast.name);
     } else {
       slotName = "'" + ast.name + "'";
+      isMultiple = isMultiple || this.slotNames.has(ast.name);
+      this.slotNames.add(ast.name);
     }
     const dynProps = ast.attrs ? ast.attrs["t-props"] : null;
     if (ast.attrs) {
       delete ast.attrs["t-props"];
     }
+    let key = this.target.loopLevel ? `key${this.target.loopLevel}` : "key";
+    if (isMultiple) {
+      key = `${key} + \`${this.generateComponentKey()}\``;
+    }
+
     const props = ast.attrs ? this.formatPropObject(ast.attrs) : [];
     const scope = this.getPropString(props, dynProps);
     if (ast.defaultContent) {
       const name = this.compileInNewTarget("defaultContent", ast.defaultContent, ctx);
-      blockString = `callSlot(ctx, node, key, ${slotName}, ${dynamic}, ${scope}, ${name})`;
+      blockString = `callSlot(ctx, node, ${key}, ${slotName}, ${dynamic}, ${scope}, ${name})`;
     } else {
       if (dynamic) {
         let name = generateId("slot");
         this.define(name, slotName);
-        blockString = `toggler(${name}, callSlot(ctx, node, key, ${name}, ${dynamic}, ${scope}))`;
+        blockString = `toggler(${name}, callSlot(ctx, node, ${key}, ${name}, ${dynamic}, ${scope}))`;
       } else {
-        blockString = `callSlot(ctx, node, key, ${slotName}, ${dynamic}, ${scope})`;
+        blockString = `callSlot(ctx, node, ${key}, ${slotName}, ${dynamic}, ${scope})`;
       }
     }
     // event handling
diff --git a/tests/components/__snapshots__/slots.test.ts.snap b/tests/components/__snapshots__/slots.test.ts.snap
index a9f0c944..4930b7d1 100644
--- a/tests/components/__snapshots__/slots.test.ts.snap
+++ b/tests/components/__snapshots__/slots.test.ts.snap
@@ -604,6 +604,64 @@ exports[`slots default slot work with text nodes 2`] = `
 }"
 `;
 
+exports[`slots dynamic slot in multiple locations 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { capture, markRaw } = helpers;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, true);
+  const comp2 = app.createComponent(\`Slotter\`, true, true, false, false);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    const b2 = text(\`hello \`);
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    return multi([b2, b3]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    const ctx1 = capture(ctx);
+    return comp2({location: ctx['state'].location,slots: markRaw({'coffee': {__render: slot1, __ctx: ctx1}})}, key + \`__2\`, node, this, null);
+  }
+}"
+`;
+
+exports[`slots dynamic slot in multiple locations 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { callSlot } = helpers;
+  
+  let block2 = createBlock(\`<p><block-child-0/></p>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b4;
+    if (ctx['props'].location===1) {
+      const slot1 = ('coffee');
+      const b3 = toggler(slot1, callSlot(ctx, node, key + \`__1\`, slot1, true, {}));
+      b2 = block2([], [b3]);
+    }
+    if (ctx['props'].location===2) {
+      const slot2 = ('coffee');
+      b4 = toggler(slot2, callSlot(ctx, node, key + \`__2\`, slot2, true, {}));
+    }
+    return multi([b2, b4]);
+  }
+}"
+`;
+
+exports[`slots dynamic slot in multiple locations 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>child</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`slots dynamic t-slot call 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
@@ -645,7 +703,7 @@ exports[`slots dynamic t-slot call 2`] = `
   return function template(ctx, node, key = \\"\\") {
     let hdlr1 = [ctx['toggle'], ctx];
     const slot1 = (ctx['current'].slot);
-    const b2 = toggler(slot1, callSlot(ctx, node, key, slot1, true, {}));
+    const b2 = toggler(slot1, callSlot(ctx, node, key + \`__1\`, slot1, true, {}));
     return block1([hdlr1], [b2]);
   }
 }"
@@ -695,7 +753,7 @@ exports[`slots dynamic t-slot call with default 2`] = `
   
   return function template(ctx, node, key = \\"\\") {
     let hdlr1 = [ctx['toggle'], ctx];
-    const b3 = callSlot(ctx, node, key, (ctx['current'].slot), true, {}, defaultContent1);
+    const b3 = callSlot(ctx, node, key + \`__1\`, (ctx['current'].slot), true, {}, defaultContent1);
     return block1([hdlr1], [b3]);
   }
 }"
@@ -726,7 +784,7 @@ exports[`slots fun: two calls to the same slot 2`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const b2 = callSlot(ctx, node, key, 'default', false, {});
-    const b3 = callSlot(ctx, node, key, 'default', false, {});
+    const b3 = callSlot(ctx, node, key + \`__1\`, 'default', false, {});
     return multi([b2, b3]);
   }
 }"
@@ -1527,7 +1585,7 @@ exports[`slots simple dynamic slot with slot scope 2`] = `
   
   return function template(ctx, node, key = \\"\\") {
     const slot1 = ('slotName');
-    const b2 = toggler(slot1, callSlot(ctx, node, key, slot1, true, {bool: ctx['state'].bool}));
+    const b2 = toggler(slot1, callSlot(ctx, node, key + \`__1\`, slot1, true, {bool: ctx['state'].bool}));
     return block1([], [b2]);
   }
 }"
@@ -1854,7 +1912,7 @@ exports[`slots slot content has different key from other content -- dynamic slot
   return function template(ctx, node, key = \\"\\") {
     const b2 = comp1({parent: 'SlotDisplay'}, key + \`__1\`, node, this, null);
     const slot1 = (ctx['slotName']);
-    const b3 = toggler(slot1, callSlot(ctx, node, key, slot1, true, {}));
+    const b3 = toggler(slot1, callSlot(ctx, node, key + \`__2\`, slot1, true, {}));
     return multi([b2, b3]);
   }
 }"
@@ -1995,6 +2053,118 @@ exports[`slots slot content is bound to caller 2`] = `
 }"
 `;
 
+exports[`slots slot in multiple locations 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { markRaw } = helpers;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, true);
+  const comp2 = app.createComponent(\`Slotter\`, true, true, false, false);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    const b2 = text(\` hello \`);
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    return multi([b2, b3]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    return comp2({location: ctx['state'].location,slots: markRaw({'default': {__render: slot1, __ctx: ctx}})}, key + \`__2\`, node, this, null);
+  }
+}"
+`;
+
+exports[`slots slot in multiple locations 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { callSlot } = helpers;
+  
+  let block2 = createBlock(\`<p><block-child-0/></p>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    let b2,b4;
+    if (ctx['props'].location===1) {
+      const b3 = callSlot(ctx, node, key, 'default', false, {});
+      b2 = block2([], [b3]);
+    }
+    if (ctx['props'].location===2) {
+      b4 = callSlot(ctx, node, key + \`__1\`, 'default', false, {});
+    }
+    return multi([b2, b4]);
+  }
+}"
+`;
+
+exports[`slots slot in multiple locations 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>child</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
+exports[`slots slot in t-foreach locations 1`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { markRaw } = helpers;
+  const comp1 = app.createComponent(\`Child\`, true, false, false, true);
+  const comp2 = app.createComponent(\`Slotter\`, true, true, false, false);
+  
+  function slot1(ctx, node, key = \\"\\") {
+    const b2 = text(\` hello \`);
+    const b3 = comp1({}, key + \`__1\`, node, this, null);
+    return multi([b2, b3]);
+  }
+  
+  return function template(ctx, node, key = \\"\\") {
+    return comp2({list: ctx['state'].list,slots: markRaw({'default': {__render: slot1, __ctx: ctx}})}, key + \`__2\`, node, this, null);
+  }
+}"
+`;
+
+exports[`slots slot in t-foreach locations 2`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  let { prepareList, callSlot, withKey } = helpers;
+  
+  let block2 = createBlock(\`<p><block-text-0/><block-child-0/></p>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    ctx = Object.create(ctx);
+    const [k_block1, v_block1, l_block1, c_block1] = prepareList(ctx['props'].list);;
+    for (let i1 = 0; i1 < l_block1; i1++) {
+      ctx[\`elem\`] = v_block1[i1];
+      ctx[\`elem_index\`] = i1;
+      const key1 = ctx['elem_index'];
+      let txt1 = ctx['elem'];
+      const b3 = callSlot(ctx, node, key1, 'default', false, {});
+      c_block1[i1] = withKey(block2([txt1], [b3]), key1);
+    }
+    return list(c_block1);
+  }
+}"
+`;
+
+exports[`slots slot in t-foreach locations 3`] = `
+"function anonymous(app, bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
+  
+  let block1 = createBlock(\`<div>child</div>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`slots slot preserves properly parented relationship 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/components/slots.test.ts b/tests/components/slots.test.ts
index 370be644..be1e574d 100644
--- a/tests/components/slots.test.ts
+++ b/tests/components/slots.test.ts
@@ -1819,4 +1819,102 @@ describe("slots", () => {
     await nextTick();
     expect(fixture.innerHTML).toBe("<button>inc</button>[B][C][A][sub1] [sub2334]");
   });
+
+  test("slot in multiple locations", async () => {
+    class Child extends Component {
+      static template = xml`<div>child</div>`;
+    }
+
+    class Slotter extends Component {
+      static components = { Child };
+      static template = xml`
+          <t t-if="props.location === 1">
+            <p><t t-slot="default"/></p>
+          </t>
+          <t t-if="props.location === 2">
+            <t t-slot="default"/>
+          </t>
+      `;
+    }
+
+    class Parent extends Component {
+      static components = { Child, Slotter };
+      static template = xml`
+         <Slotter location="state.location">
+          hello <Child/>
+         </Slotter>`;
+      state = useState({ location: 1 });
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<p> hello <div>child</div></p>");
+    parent.state.location = 2;
+    await nextTick();
+    expect(fixture.innerHTML).toBe(" hello <div>child</div>");
+  });
+
+  test("dynamic slot in multiple locations", async () => {
+    class Child extends Component {
+      static template = xml`<div>child</div>`;
+    }
+
+    class Slotter extends Component {
+      static components = { Child };
+      static template = xml`
+          <t t-if="props.location === 1">
+            <p><t t-slot="{{'coffee'}}"/></p>
+          </t>
+          <t t-if="props.location === 2">
+            <t t-slot="{{'coffee'}}"/>
+          </t>
+      `;
+    }
+
+    class Parent extends Component {
+      static components = { Child, Slotter };
+      static template = xml`
+         <Slotter location="state.location">
+          <t t-set-slot="coffee">hello <Child/></t>
+         </Slotter>`;
+      state = useState({ location: 1 });
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<p>hello <div>child</div></p>");
+    parent.state.location = 2;
+    await nextTick();
+    expect(fixture.innerHTML).toBe("hello <div>child</div>");
+  });
+
+  test("slot in t-foreach locations", async () => {
+    class Child extends Component {
+      static template = xml`<div>child</div>`;
+    }
+
+    class Slotter extends Component {
+      static components = { Child };
+      static template = xml`
+          <t t-foreach="props.list" t-as="elem" t-key="elem_index">
+            <p><t t-esc="elem"/><t t-slot="default"/></p>
+          </t>
+      `;
+    }
+
+    class Parent extends Component {
+      static components = { Child, Slotter };
+      static template = xml`
+         <Slotter list="state.list">
+          hello <Child/>
+         </Slotter>`;
+      state = useState({ list: [1] });
+    }
+
+    const parent = await mount(Parent, fixture);
+    expect(fixture.innerHTML).toBe("<p>1 hello <div>child</div></p>");
+    parent.state.list.push(2);
+    await nextTick();
+    expect(fixture.innerHTML).toBe(
+      "<p>1 hello <div>child</div></p><p>2 hello <div>child</div></p>"
+    );
+  });
 });