From cb867e388d0de3547b3f4ebc10e2e92c95b75d7e Mon Sep 17 00:00:00 2001 From: Ben Demboski Date: Fri, 15 Dec 2023 08:35:10 -0800 Subject: [PATCH] Fix helper/modifier memory leak (#1440) 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. --- .../runtime/lib/compiled/opcodes/dom.ts | 37 +++++++++++-------- .../lib/compiled/opcodes/expressions.ts | 29 +++++++++------ 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts index 74f94016f0..5d7d757972 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts @@ -20,6 +20,7 @@ import { CheckString, } from '@glimmer/debug'; import { associateDestroyableChild, destroy } from '@glimmer/destroyable'; +import { getInternalModifierManager } from '@glimmer/manager'; import { createComputeRef, isConstRef, valueForRef } from '@glimmer/reference'; import { assign, debugToString, expect, isObject } from '@glimmer/util'; import { consumeTag, CURRENT_TAG, validateTag, valueForTag } from '@glimmer/validator'; @@ -149,7 +150,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(); @@ -188,23 +189,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( - 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 index d11eb8c608..2fa7af7475 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts @@ -4,8 +4,6 @@ import type { Helper, HelperDefinitionState, Owner, - ResolutionTimeConstants, - RuntimeConstants, ScopeBlock, VM as PublicVM, } from '@glimmer/interfaces'; @@ -20,6 +18,7 @@ import { } from '@glimmer/debug'; import { _hasDestroyableChildren, associateDestroyableChild, destroy } from '@glimmer/destroyable'; import { toBool } from '@glimmer/global-context'; +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)) { @@ -124,14 +123,20 @@ APPEND_OPCODES.add(Op.DynamicHelper, (vm) => { vm.loadValue($v0, helperValueRef); }); -function resolveHelper( - constants: RuntimeConstants & ResolutionTimeConstants, - definition: HelperDefinitionState, - ref: Reference -): Helper { - let handle = constants.helper(definition, null, true)!; +function resolveHelper(definition: HelperDefinitionState, ref: Reference): Helper { + 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 +146,7 @@ function resolveHelper( ); } - return constants.getValue(handle); + return helper!; } APPEND_OPCODES.add(Op.Helper, (vm, { op1: handle }) => {