From 7f2aed1377b8ba9e1cbdc8cc267c9386e3f0360c Mon Sep 17 00:00:00 2001 From: Chris Thielen Date: Sat, 20 Jan 2018 09:36:39 -0800 Subject: [PATCH] fix(core): Fix memory leak of resolve data from ALL transitions ever The treeChanges object has references to the PathNodes from the previous transition (`.treeChanges("from")`). The PathNode object has resolve data inside it. The previous transition is reachable via a resolve (via tokens: `"$transition$"` or `Transition`). Through this chain, all other transitions are reachable and not eligible for GC. This change cleans out the previous Transition object from the resolves, but only after a transition has been evicted from the `successfulTransitions` queue. The `successfulTransitions` queue currently has a max size of 1, so the transition is cleaned up once it is no longer the current nor previous transition (it is cleaned when it's the previous previous transition); Fixes https://github.com/ui-router/core/issues/55 Fixes https://github.com/angular-ui/ui-router/issues/3603 Fixes https://github.com/ui-router/angular/issues/21 --- src/hooks/coreResolvables.ts | 35 ++++++++++++++++++++++++----- src/resolve/resolvable.ts | 3 ++- src/router.ts | 6 ++--- src/transition/hookRegistry.ts | 16 +++++-------- src/transition/transitionService.ts | 3 ++- test/transitionSpec.ts | 18 +++++++++++++++ 6 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/hooks/coreResolvables.ts b/src/hooks/coreResolvables.ts index 14210d1c..77cbf77e 100644 --- a/src/hooks/coreResolvables.ts +++ b/src/hooks/coreResolvables.ts @@ -2,17 +2,42 @@ import { Transition } from '../transition/transition'; import { UIRouter } from '../router'; import { TransitionService } from '../transition/transitionService'; +import { Resolvable } from '../resolve'; +import { extend, inArray, map, mapObj, unnestR, values } from '../common'; +import { PathNode } from '../path'; +import { TreeChanges } from "../transition"; function addCoreResolvables(trans: Transition) { - trans.addResolvable({ token: UIRouter, deps: [], resolveFn: () => trans.router, data: trans.router }, ''); - trans.addResolvable({ token: Transition, deps: [], resolveFn: () => trans, data: trans }, ''); - trans.addResolvable({ token: '$transition$', deps: [], resolveFn: () => trans, data: trans }, ''); - trans.addResolvable({ token: '$stateParams', deps: [], resolveFn: () => trans.params(), data: trans.params() }, ''); + trans.addResolvable(Resolvable.fromData(UIRouter, trans.router), ''); + trans.addResolvable(Resolvable.fromData(Transition, trans), ''); + trans.addResolvable(Resolvable.fromData('$transition$', trans), ''); + trans.addResolvable(Resolvable.fromData('$stateParams', trans.params()), ''); trans.entering().forEach(state => { - trans.addResolvable({ token: '$state$', deps: [], resolveFn: () => state, data: state }, state); + trans.addResolvable(Resolvable.fromData('$state$', state), state); }); } export const registerAddCoreResolvables = (transitionService: TransitionService) => transitionService.onCreate({}, addCoreResolvables); + +const TRANSITION_TOKENS = ['$transition$', Transition]; +const isTransition = inArray(TRANSITION_TOKENS); + +// References to Transition in the treeChanges pathnodes makes all +// previous Transitions reachable in memory, causing a memory leak +// This function removes resolves for '$transition$' and `Transition` from the treeChanges. +// Do not use this on current transitions, only on old ones. +export const treeChangesCleanup = (trans: Transition) => { + // If the resolvable is a Transition, return a new resolvable with null data + const replaceTransitionWithNull = (r: Resolvable): Resolvable => + isTransition(r.token) ? Resolvable.fromData(r.token, null) : r; + + const cleanPath = (path: PathNode[]) => path.map((node: PathNode) => { + const resolvables = node.resolvables.map(replaceTransitionWithNull); + return extend(node.clone(), { resolvables }); + }); + + const treeChanges: TreeChanges = trans.treeChanges(); + mapObj(treeChanges, cleanPath, treeChanges); +}; diff --git a/src/resolve/resolvable.ts b/src/resolve/resolvable.ts index a48cf708..1edad807 100644 --- a/src/resolve/resolvable.ts +++ b/src/resolve/resolvable.ts @@ -88,7 +88,7 @@ export class Resolvable implements ResolvableLiteral { this.data = data; this.resolved = data !== undefined; this.promise = this.resolved ? services.$q.when(this.data) : undefined; - } else if (isObject(arg1) && arg1.token && isFunction(arg1.resolveFn)) { + } else if (isObject(arg1) && arg1.token && (arg1.hasOwnProperty('resolveFn') || arg1.hasOwnProperty('data'))) { const literal = arg1; return new Resolvable(literal.token, literal.resolveFn, literal.deps, literal.policy, literal.data); } @@ -144,6 +144,7 @@ export class Resolvable implements ResolvableLiteral { const applyResolvedValue = (resolvedValue: any) => { this.data = resolvedValue; this.resolved = true; + this.resolveFn = null; trace.traceResolvableResolved(this, trans); return this.data; }; diff --git a/src/router.ts b/src/router.ts index 011392a8..5d3c6f3b 100644 --- a/src/router.ts +++ b/src/router.ts @@ -45,12 +45,12 @@ export class UIRouter { /** Provides services related to ui-view synchronization */ viewService = new ViewService(); - /** Provides services related to Transitions */ - transitionService: TransitionService = new TransitionService(this); - /** Global router state */ globals: UIRouterGlobals = new UIRouterGlobals(); + /** Provides services related to Transitions */ + transitionService: TransitionService = new TransitionService(this); + /** * Deprecated for public use. Use [[urlService]] instead. * @deprecated Use [[urlService]] instead diff --git a/src/transition/hookRegistry.ts b/src/transition/hookRegistry.ts index 0f86ec93..07b66855 100644 --- a/src/transition/hookRegistry.ts +++ b/src/transition/hookRegistry.ts @@ -2,18 +2,14 @@ * @coreapi * @module transition */ /** for typedoc */ -import { extend, removeFrom, tail, values, identity, map } from '../common/common'; -import { isString, isFunction } from '../common/predicates'; +import { isString, isFunction, Glob, extend, removeFrom, tail, values, identity, mapObj } from '../common'; import { PathNode } from '../path/pathNode'; import { - TransitionStateHookFn, TransitionHookFn, TransitionHookPhase, TransitionHookScope, IHookRegistry, PathType, -} from './interface'; // has or is using - -import { - HookRegOptions, HookMatchCriteria, TreeChanges, - HookMatchCriterion, IMatchingNodes, HookFn, + TransitionStateHookFn, TransitionHookFn, TransitionHookPhase, // has or is using + TransitionHookScope, IHookRegistry, PathType, } from './interface'; -import { Glob } from '../common/glob'; + +import { HookRegOptions, HookMatchCriteria, TreeChanges, HookMatchCriterion, IMatchingNodes, HookFn } from './interface'; import { StateObject } from '../state/stateObject'; import { TransitionEventType } from './transitionEventType'; import { TransitionService } from './transitionService'; @@ -108,7 +104,7 @@ export class RegisteredHook { * } */ private _getDefaultMatchCriteria(): HookMatchCriteria { - return map(this.tranSvc._pluginapi._getPathTypes(), () => true); + return mapObj(this.tranSvc._pluginapi._getPathTypes(), () => true); } /** diff --git a/src/transition/transitionService.ts b/src/transition/transitionService.ts index 32ba1c38..529f53f3 100644 --- a/src/transition/transitionService.ts +++ b/src/transition/transitionService.ts @@ -13,7 +13,7 @@ import { TargetState } from '../state/targetState'; import { PathNode } from '../path/pathNode'; import { ViewService } from '../view/view'; import { UIRouter } from '../router'; -import { registerAddCoreResolvables } from '../hooks/coreResolvables'; +import { registerAddCoreResolvables, treeChangesCleanup } from '../hooks/coreResolvables'; import { registerRedirectToHook } from '../hooks/redirectTo'; import { registerOnExitHook, registerOnRetainHook, registerOnEnterHook } from '../hooks/onEnterExitRetain'; import { registerEagerResolvePath, registerLazyResolveState, registerResolveRemaining } from '../hooks/resolve'; @@ -163,6 +163,7 @@ export class TransitionService implements IHookRegistry, Disposable { this._defineCorePaths(); this._defineCoreEvents(); this._registerCoreTransitionHooks(); + _router.globals.successfulTransitions.onEvict(treeChangesCleanup); } /** diff --git a/test/transitionSpec.ts b/test/transitionSpec.ts index a3098c84..35b8ee7f 100644 --- a/test/transitionSpec.ts +++ b/test/transitionSpec.ts @@ -1002,4 +1002,22 @@ describe('transition', function () { done(); }); }); + + describe('from previous transitions', () => { + it('should get their Transition resolves cleaned up', async(done) => { + router.stateRegistry.register({ name: 'resolve', resolve: { foo: () => 'Some data' } }); + const trans = router.stateService.go('resolve').transition; + await trans.promise; + + expect(trans.injector().get(Transition)).toBe(trans); + expect(trans.injector().get('foo')).toBe('Some data'); + + await router.stateService.go('A'); + + expect(trans.injector().get(Transition)).toBe(null); + expect(trans.injector().get('foo')).toBe('Some data'); + + done(); + }); + }); });