Skip to content

Commit 7f2aed1

Browse files
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 #55 Fixes angular-ui/ui-router#3603 Fixes ui-router/angular#21
1 parent eaff405 commit 7f2aed1

File tree

6 files changed

+61
-20
lines changed

6 files changed

+61
-20
lines changed

src/hooks/coreResolvables.ts

+30-5
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,42 @@
22
import { Transition } from '../transition/transition';
33
import { UIRouter } from '../router';
44
import { TransitionService } from '../transition/transitionService';
5+
import { Resolvable } from '../resolve';
6+
import { extend, inArray, map, mapObj, unnestR, values } from '../common';
7+
import { PathNode } from '../path';
8+
import { TreeChanges } from "../transition";
59

610
function addCoreResolvables(trans: Transition) {
7-
trans.addResolvable({ token: UIRouter, deps: [], resolveFn: () => trans.router, data: trans.router }, '');
8-
trans.addResolvable({ token: Transition, deps: [], resolveFn: () => trans, data: trans }, '');
9-
trans.addResolvable({ token: '$transition$', deps: [], resolveFn: () => trans, data: trans }, '');
10-
trans.addResolvable({ token: '$stateParams', deps: [], resolveFn: () => trans.params(), data: trans.params() }, '');
11+
trans.addResolvable(Resolvable.fromData(UIRouter, trans.router), '');
12+
trans.addResolvable(Resolvable.fromData(Transition, trans), '');
13+
trans.addResolvable(Resolvable.fromData('$transition$', trans), '');
14+
trans.addResolvable(Resolvable.fromData('$stateParams', trans.params()), '');
1115

1216
trans.entering().forEach(state => {
13-
trans.addResolvable({ token: '$state$', deps: [], resolveFn: () => state, data: state }, state);
17+
trans.addResolvable(Resolvable.fromData('$state$', state), state);
1418
});
1519
}
1620

1721
export const registerAddCoreResolvables = (transitionService: TransitionService) =>
1822
transitionService.onCreate({}, addCoreResolvables);
23+
24+
const TRANSITION_TOKENS = ['$transition$', Transition];
25+
const isTransition = inArray(TRANSITION_TOKENS);
26+
27+
// References to Transition in the treeChanges pathnodes makes all
28+
// previous Transitions reachable in memory, causing a memory leak
29+
// This function removes resolves for '$transition$' and `Transition` from the treeChanges.
30+
// Do not use this on current transitions, only on old ones.
31+
export const treeChangesCleanup = (trans: Transition) => {
32+
// If the resolvable is a Transition, return a new resolvable with null data
33+
const replaceTransitionWithNull = (r: Resolvable): Resolvable =>
34+
isTransition(r.token) ? Resolvable.fromData(r.token, null) : r;
35+
36+
const cleanPath = (path: PathNode[]) => path.map((node: PathNode) => {
37+
const resolvables = node.resolvables.map(replaceTransitionWithNull);
38+
return extend(node.clone(), { resolvables });
39+
});
40+
41+
const treeChanges: TreeChanges = trans.treeChanges();
42+
mapObj(treeChanges, cleanPath, treeChanges);
43+
};

src/resolve/resolvable.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export class Resolvable implements ResolvableLiteral {
8888
this.data = data;
8989
this.resolved = data !== undefined;
9090
this.promise = this.resolved ? services.$q.when(this.data) : undefined;
91-
} else if (isObject(arg1) && arg1.token && isFunction(arg1.resolveFn)) {
91+
} else if (isObject(arg1) && arg1.token && (arg1.hasOwnProperty('resolveFn') || arg1.hasOwnProperty('data'))) {
9292
const literal = <ResolvableLiteral> arg1;
9393
return new Resolvable(literal.token, literal.resolveFn, literal.deps, literal.policy, literal.data);
9494
}
@@ -144,6 +144,7 @@ export class Resolvable implements ResolvableLiteral {
144144
const applyResolvedValue = (resolvedValue: any) => {
145145
this.data = resolvedValue;
146146
this.resolved = true;
147+
this.resolveFn = null;
147148
trace.traceResolvableResolved(this, trans);
148149
return this.data;
149150
};

src/router.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ export class UIRouter {
4545
/** Provides services related to ui-view synchronization */
4646
viewService = new ViewService();
4747

48-
/** Provides services related to Transitions */
49-
transitionService: TransitionService = new TransitionService(this);
50-
5148
/** Global router state */
5249
globals: UIRouterGlobals = new UIRouterGlobals();
5350

51+
/** Provides services related to Transitions */
52+
transitionService: TransitionService = new TransitionService(this);
53+
5454
/**
5555
* Deprecated for public use. Use [[urlService]] instead.
5656
* @deprecated Use [[urlService]] instead

src/transition/hookRegistry.ts

+6-10
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,14 @@
22
* @coreapi
33
* @module transition
44
*/ /** for typedoc */
5-
import { extend, removeFrom, tail, values, identity, map } from '../common/common';
6-
import { isString, isFunction } from '../common/predicates';
5+
import { isString, isFunction, Glob, extend, removeFrom, tail, values, identity, mapObj } from '../common';
76
import { PathNode } from '../path/pathNode';
87
import {
9-
TransitionStateHookFn, TransitionHookFn, TransitionHookPhase, TransitionHookScope, IHookRegistry, PathType,
10-
} from './interface'; // has or is using
11-
12-
import {
13-
HookRegOptions, HookMatchCriteria, TreeChanges,
14-
HookMatchCriterion, IMatchingNodes, HookFn,
8+
TransitionStateHookFn, TransitionHookFn, TransitionHookPhase, // has or is using
9+
TransitionHookScope, IHookRegistry, PathType,
1510
} from './interface';
16-
import { Glob } from '../common/glob';
11+
12+
import { HookRegOptions, HookMatchCriteria, TreeChanges, HookMatchCriterion, IMatchingNodes, HookFn } from './interface';
1713
import { StateObject } from '../state/stateObject';
1814
import { TransitionEventType } from './transitionEventType';
1915
import { TransitionService } from './transitionService';
@@ -108,7 +104,7 @@ export class RegisteredHook {
108104
* }
109105
*/
110106
private _getDefaultMatchCriteria(): HookMatchCriteria {
111-
return map(this.tranSvc._pluginapi._getPathTypes(), () => true);
107+
return mapObj(this.tranSvc._pluginapi._getPathTypes(), () => true);
112108
}
113109

114110
/**

src/transition/transitionService.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { TargetState } from '../state/targetState';
1313
import { PathNode } from '../path/pathNode';
1414
import { ViewService } from '../view/view';
1515
import { UIRouter } from '../router';
16-
import { registerAddCoreResolvables } from '../hooks/coreResolvables';
16+
import { registerAddCoreResolvables, treeChangesCleanup } from '../hooks/coreResolvables';
1717
import { registerRedirectToHook } from '../hooks/redirectTo';
1818
import { registerOnExitHook, registerOnRetainHook, registerOnEnterHook } from '../hooks/onEnterExitRetain';
1919
import { registerEagerResolvePath, registerLazyResolveState, registerResolveRemaining } from '../hooks/resolve';
@@ -163,6 +163,7 @@ export class TransitionService implements IHookRegistry, Disposable {
163163
this._defineCorePaths();
164164
this._defineCoreEvents();
165165
this._registerCoreTransitionHooks();
166+
_router.globals.successfulTransitions.onEvict(treeChangesCleanup);
166167
}
167168

168169
/**

test/transitionSpec.ts

+18
Original file line numberDiff line numberDiff line change
@@ -1002,4 +1002,22 @@ describe('transition', function () {
10021002
done();
10031003
});
10041004
});
1005+
1006+
describe('from previous transitions', () => {
1007+
it('should get their Transition resolves cleaned up', async(done) => {
1008+
router.stateRegistry.register({ name: 'resolve', resolve: { foo: () => 'Some data' } });
1009+
const trans = router.stateService.go('resolve').transition;
1010+
await trans.promise;
1011+
1012+
expect(trans.injector().get(Transition)).toBe(trans);
1013+
expect(trans.injector().get('foo')).toBe('Some data');
1014+
1015+
await router.stateService.go('A');
1016+
1017+
expect(trans.injector().get(Transition)).toBe(null);
1018+
expect(trans.injector().get('foo')).toBe('Some data');
1019+
1020+
done();
1021+
});
1022+
});
10051023
});

0 commit comments

Comments
 (0)