Fix helper/modifier memory leak

By treating passing dynamic helpers and modifiers through the `ConstantsImpl`, the runtime was ensuring that they would be retained for the lifetime of the runtime. However, dynamic helpers and modifiers are often mean to have much shorter lifetimes. For example, in the case of local helpers/modifiers inside Component classes, they are often expected to be released when the component is destroyed. If the local helper/modifier is implemented by an arrow function that closes over the Component, then retaining the arrow function will also retain the component, causing the component to leak. So, skip `ConstantsImpl` and resolve the dynamic helper/modifier directly. Note that this means these dynamic helper/modifier definitions will no longer be cache, and the dynamic helper/modifier manager lookup and invocation will happen each time the helper/modifier is resolved.
glimmerjs · Sep 1, 2023 · bc6263d · bc6263d
1 parent 68d371b
commit bc6263d
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 24 deletions.
diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
@@ -18,6 +18,7 @@ import type {
   UpdatingOpcode,
   UpdatingVM,
 } from "@glimmer/interfaces";
+import { getInternalModifierManager } from '@glimmer/manager';
 import { createComputeRef, isConstRef, type Reference, valueForRef } from '@glimmer/reference';
 import { assign, debugToString, expect, isObject } from '@glimmer/util';
 import {
@@ -153,7 +154,7 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => {
     return;
   }
 
-  let { stack, [CONSTANTS]: constants } = vm;
+  let { stack } = vm;
   let ref = check(stack.pop(), CheckReference);
   let args = check(stack.pop(), CheckArguments).capture();
   let { constructing } = vm.elements();
@@ -192,23 +193,27 @@ APPEND_OPCODES.add(Op.DynamicModifier, (vm) => {
       owner = initialOwner;
     }
 
-    let handle = constants.modifier(hostDefinition, null, true);
-
-    if (import.meta.env.DEV && handle === null) {
-      throw new Error(
-        `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${
-          ref.debugLabel
-        }}}\`, and the incorrect definition is the value at the path \`${
-          ref.debugLabel
-        }\`, which was: ${debugToString!(hostDefinition)}`
-      );
+    let manager = getInternalModifierManager(hostDefinition, true);
+
+    if (manager === null) {
+      if (import.meta.env.DEV) {
+        throw new Error(
+          `Expected a dynamic modifier definition, but received an object or function that did not have a modifier manager associated with it. The dynamic invocation was \`{{${
+            ref.debugLabel
+          }}}\`, and the incorrect definition is the value at the path \`${
+            ref.debugLabel
+          }\`, which was: ${debugToString!(hostDefinition)}`
+        );
+      } else {
+        throw new Error('BUG: modifier manager expected');
+      }
     }
 
-    let definition = constants.getValue<ModifierDefinition>(
-      expect(handle, 'BUG: modifier handle expected')
-    );
-
-    let { manager } = definition;
+    let definition = {
+      resolvedName: null,
+      manager,
+      state: hostDefinition,
+    }
 
     let state = manager.create(
       owner,

diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts
@@ -14,11 +14,10 @@ import type {
   Helper,
   HelperDefinitionState,
   Owner,
-  ResolutionTimeConstants,
-  RuntimeConstants,
   ScopeBlock,
   VM as PublicVM,
 } from "@glimmer/interfaces";
+import { getInternalHelperManager } from "@glimmer/manager";
 import {
   childRefFor,
   createComputeRef,
@@ -90,7 +89,7 @@ APPEND_OPCODES.add(Op.DynamicHelper, (vm) => {
     if (isCurriedType(definition, CurriedTypes.Helper)) {
       let { definition: resolvedDef, owner, positional, named } = resolveCurriedValue(definition);
 
-      let helper = resolveHelper(vm[CONSTANTS], resolvedDef, ref);
+      let helper = resolveHelper(resolvedDef, ref);
 
       if (named !== undefined) {
         args.named = assign({}, ...named, args.named);
@@ -104,7 +103,7 @@ APPEND_OPCODES.add(Op.DynamicHelper, (vm) => {
 
       associateDestroyableChild(helperInstanceRef, helperRef);
     } else if (isObject(definition)) {
-      let helper = resolveHelper(vm[CONSTANTS], definition, ref);
+      let helper = resolveHelper(definition, ref);
       helperRef = helper(args, initialOwner);
 
       if (_hasDestroyableChildren(helperRef)) {
@@ -125,13 +124,22 @@ APPEND_OPCODES.add(Op.DynamicHelper, (vm) => {
 });
 
 function resolveHelper(
-  constants: RuntimeConstants & ResolutionTimeConstants,
   definition: HelperDefinitionState,
   ref: Reference
 ): Helper {
-  let handle = constants.helper(definition, null, true)!;
+  let managerOrHelper = getInternalHelperManager(definition, true);
+  let helper;
+  if (managerOrHelper === null) {
+    helper = null;
+  } else {
+    helper =
+      typeof managerOrHelper === 'function'
+        ? managerOrHelper
+        : managerOrHelper.getHelper(definition);
+    assert(managerOrHelper, 'BUG: expected manager or helper');
+  }
 
-  if (import.meta.env.DEV && handle === null) {
+  if (import.meta.env.DEV && helper === null) {
     throw new Error(
       `Expected a dynamic helper definition, but received an object or function that did not have a helper manager associated with it. The dynamic invocation was \`{{${
         ref.debugLabel
@@ -141,7 +149,7 @@ function resolveHelper(
     );
   }
 
-  return constants.getValue(handle);
+  return helper!;
 }
 
 APPEND_OPCODES.add(Op.Helper, (vm, { op1: handle }) => {