[FIX] runtime, compiler: fix refs getting set or unset incorrectly

Previously, refs could get set to null incorrectly, or could stay set
when they shouldn't. This was caused by the fact that singleRefSetter
and multiRefSetters are created on every render, meaning that any render
that did not affect the status of the ref (mounted or not), would
overwrite the previous ref setter, including its closure, causing the
captured `_el` or `count` to be lost. This means that on a subsequent
render that did affect the status of the ref, the singleRefSetter would
consider that it did not set the ref, and so shouldn't unset it, causing
it to incorrectly stay keep the element, while in multiRefSetter, the
opposite problem occured: the count would be 0 on removal, causing it to
believe that it was the first call in the corresponding patch that
affected the ref, when in fact, the closure is already stale, because it
was created from the previous render, and the fresh multiRefSetter from
the latest render may already have been called by an element with that
ref that came earlier in the template.

This commit changes the ref setting strategy to work around the issue of
stale closures: we define a setRef method on ComponentNode that is
always called, this method ignores calls with `null`, meaning that the
refs object on the ComponentNode never reverts to a null value for any
key. Instead, the useRef hook will check whether the element that the
ref points to is still mounted, and return null if not.

src/compiler/code_generator.ts

@@ -192,11 +192,9 @@ class CodeTarget {
   code: string[] = [];
   hasRoot = false;
   hasCache = false;
-  hasRef: boolean = false;
-  // maps ref name to [id, expr]
-  refInfo: { [name: string]: [string, string] } = {};
   shouldProtectScope: boolean = false;
   on: EventHandlers | null;
+  hasRefWrapper: boolean = false;
 
   constructor(name: string, on?: EventHandlers | null) {
     this.name = name;
@@ -215,17 +213,13 @@ class CodeTarget {
   generateCode(): string {
     let result: string[] = [];
     result.push(`function ${this.name}(ctx, node, key = "") {`);
-    if (this.hasRef) {
-      result.push(`  const refs = this.__owl__.refs;`);
-      for (let name in this.refInfo) {
-        const [id, expr] = this.refInfo[name];
-        result.push(`  const ${id} = ${expr};`);
-      }
-    }
     if (this.shouldProtectScope) {
       result.push(`  ctx = Object.create(ctx);`);
       result.push(`  ctx[isBoundary] = 1`);
     }
+    if (this.hasRefWrapper) {
+      result.push(`  let refWrapper = makeRefWrapper(this.__owl__);`);
+    }
     if (this.hasCache) {
       result.push(`  let cache = ctx.cache || {};`);
       result.push(`  let nextCache = ctx.cache = {};`);
@@ -704,30 +698,21 @@ export class CodeGenerator {
 
     // t-ref
     if (ast.ref) {
-      this.target.hasRef = true;
-      const isDynamic = INTERP_REGEXP.test(ast.ref);
-      if (isDynamic) {
-        this.helpers.add("singleRefSetter");
-        const str = replaceDynamicParts(ast.ref, (expr) => this.captureExpression(expr, true));
-        const idx = block!.insertData(`singleRefSetter(refs, ${str})`, "ref");
-        attrs["block-ref"] = String(idx);
-      } else {
-        let name = ast.ref;
-        if (name in this.target.refInfo) {
-          // ref has already been defined
-          this.helpers.add("multiRefSetter");
-          const info = this.target.refInfo[name];
-          const index = block!.data.push(info[0]) - 1;
-          attrs["block-ref"] = String(index);
-          info[1] = `multiRefSetter(refs, \`${name}\`)`;
-        } else {
-          let id = generateId("ref");
-          this.helpers.add("singleRefSetter");
-          this.target.refInfo[name] = [id, `singleRefSetter(refs, \`${name}\`)`];
-          const index = block!.data.push(id) - 1;
-          attrs["block-ref"] = String(index);
-        }
+      if (this.dev) {
+        this.helpers.add("makeRefWrapper");
+        this.target.hasRefWrapper = true;
       }
+      const isDynamic = INTERP_REGEXP.test(ast.ref);
+      let name = `\`${ast.ref}\``;
+      if (isDynamic) {
+        name = replaceDynamicParts(ast.ref, (expr) => this.captureExpression(expr, true));
+      }
+      let setRefStr = `(el) => this.__owl__.setRef((${name}), el)`;
+      if (this.dev) {
+        setRefStr = `refWrapper(${name}, ${setRefStr})`;
+      }
+      const idx = block!.insertData(setRefStr, "ref");
+      attrs["block-ref"] = String(idx);
     }
 
     const dom = xmlDoc.createElement(ast.tag);

src/runtime/component_node.ts

@@ -281,6 +281,19 @@ export class ComponentNode<P extends Props = any, E = any> implements VNode<Comp
     }
   }
 
+  /**
+   * Sets a ref to a given HTMLElement.
+   *
+   * @param name the name of the ref to set
+   * @param el the HTMLElement to set the ref to. The ref is not set if the el
+   *  is null, but useRef will not return elements that are not in the DOM
+   */
+  setRef(name: string, el: HTMLElement | null) {
+    if (el) {
+      this.refs[name] = el;
+    }
+  }
+
   // ---------------------------------------------------------------------------
   // Block DOM methods
   // ---------------------------------------------------------------------------

src/runtime/hooks.ts

@@ -15,7 +15,8 @@ export function useRef<T extends HTMLElement = HTMLElement>(name: string): { el:
   const refs = node.refs;
   return {
     get el(): T | null {
-      return refs[name] || null;
+      const el = refs[name];
+      return el?.ownerDocument.contains(el) ? el : null;
     },
   };
 }

src/runtime/template_helpers.ts

@@ -5,6 +5,7 @@ import { isOptional, validateSchema } from "./validation";
 import type { ComponentConstructor } from "./component";
 import { markRaw } from "./reactivity";
 import { OwlError } from "./error_handling";
+import type { ComponentNode } from "./component_node";
 
 const ObjectCreate = Object.create;
 /**
@@ -184,34 +185,6 @@ function bind(component: any, fn: Function): Function {
   return boundFn;
 }
 
-type RefMap = { [key: string]: HTMLElement | null };
-type RefSetter = (el: HTMLElement | null) => void;
-
-function multiRefSetter(refs: RefMap, name: string): RefSetter {
-  let count = 0;
-  return (el) => {
-    if (el) {
-      count++;
-      if (count > 1) {
-        throw new OwlError("Cannot have 2 elements with same ref name at the same time");
-      }
-    }
-    if (count === 0 || el) {
-      refs[name] = el;
-    }
-  };
-}
-
-function singleRefSetter(refs: RefMap, name: string): RefSetter {
-  let _el: HTMLElement | null = null;
-  return (el) => {
-    if (el || refs[name] === _el) {
-      refs[name] = el;
-      _el = el;
-    }
-  };
-}
-
 /**
  * Validate the component props (or next props) against the (static) props
  * description.  This is potentially an expensive operation: it may needs to
@@ -260,6 +233,19 @@ export function validateProps<P>(name: string | ComponentConstructor<P>, props:
   }
 }
 
+function makeRefWrapper(node: ComponentNode) {
+  let refNames: Set<String> = new Set();
+  return (name: string, fn: Function) => {
+    if (refNames.has(name)) {
+      throw new OwlError(
+        `Cannot set the same ref more than once in the same component, ref "${name}" was set multiple times in ${node.name}`
+      );
+    }
+    refNames.add(name);
+    return fn;
+  };
+}
+
 export const helpers = {
   withDefault,
   zero: Symbol("zero"),
@@ -269,8 +255,6 @@ export const helpers = {
   withKey,
   prepareList,
   setContextValue,
-  multiRefSetter,
-  singleRefSetter,
   shallowEqual,
   toNumber,
   validateProps,
@@ -280,4 +264,5 @@ export const helpers = {
   createCatcher,
   markRaw,
   OwlError,
+  makeRefWrapper,
 };