Skip to content

Commit

Permalink
fix(core): Fix memory leak of resolve data from ALL transitions ever
Browse files Browse the repository at this point in the history
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 ui-router#55
Fixes angular-ui/ui-router#3603
Fixes ui-router/angular#21
  • Loading branch information
christopherthielen authored and wawyed committed Mar 3, 2018
1 parent c086bcf commit 69962f1
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 20 deletions.
35 changes: 30 additions & 5 deletions src/hooks/coreResolvables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
3 changes: 2 additions & 1 deletion src/resolve/resolvable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <ResolvableLiteral> arg1;
return new Resolvable(literal.token, literal.resolveFn, literal.deps, literal.policy, literal.data);
}
Expand Down Expand Up @@ -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;
};
Expand Down
6 changes: 3 additions & 3 deletions src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 6 additions & 10 deletions src/transition/hookRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -108,7 +104,7 @@ export class RegisteredHook {
* }
*/
private _getDefaultMatchCriteria(): HookMatchCriteria {
return map(this.tranSvc._pluginapi._getPathTypes(), () => true);
return mapObj(this.tranSvc._pluginapi._getPathTypes(), () => true);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/transition/transitionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -163,6 +163,7 @@ export class TransitionService implements IHookRegistry, Disposable {
this._defineCorePaths();
this._defineCoreEvents();
this._registerCoreTransitionHooks();
_router.globals.successfulTransitions.onEvict(treeChangesCleanup);
}

/**
Expand Down
18 changes: 18 additions & 0 deletions test/transitionSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});

0 comments on commit 69962f1

Please sign in to comment.