Skip to content

Commit

Permalink
Fix helper/modifier memory leak
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bendemboski committed Dec 13, 2023
1 parent 5384934 commit faa2563
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 28 deletions.
37 changes: 21 additions & 16 deletions packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<ModifierDefinition>(
expect(handle, 'BUG: modifier handle expected')
);

let { manager } = definition;
let definition = {
resolvedName: null,
manager,
state: hostDefinition,
};

let state = manager.create(
owner,
Expand Down
29 changes: 17 additions & 12 deletions packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import type {
Helper,
HelperDefinitionState,
Owner,
ResolutionTimeConstants,
RuntimeConstants,
ScopeBlock,
VM as PublicVM,
} from '@glimmer/interfaces';
Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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)) {
Expand All @@ -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
Expand All @@ -141,7 +146,7 @@ function resolveHelper(
);
}

return constants.getValue(handle);
return helper!;
}

APPEND_OPCODES.add(Op.Helper, (vm, { op1: handle }) => {
Expand Down

0 comments on commit faa2563

Please sign in to comment.