From faa256384a4fead369cff3fa2fee030eceaf2611 Mon Sep 17 00:00:00 2001 From: Ben Demboski Date: Thu, 31 Aug 2023 22:02:54 -0700 Subject: [PATCH] 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. --- .../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 74f94016f..5d7d75797 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 d11eb8c60..2fa7af747 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 }) => {