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 #55
Fixes angular-ui/ui-router#3603
Fixes ui-router/angular#21
  • Loading branch information
christopherthielen committed Jan 21, 2018
1 parent eaff405 commit 7f2aed1
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();
});
});
});

4 comments on commit 7f2aed1

@LeonanCarvalho
Copy link

@LeonanCarvalho LeonanCarvalho commented on 7f2aed1 Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is any chance of it be implemented in legacy version? (angular-ui/ui-router#545)

@christopherthielen
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeonanCarvalho no, because legacy version doesn't use Transition. This was a new bug introduced somewhere around 1.0 release.

I'm not aware of any memory leaks in the legacy version

@LeonanCarvalho
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but at this issue angular-ui/ui-router#545 as closed, this is the reason that I went here to comment.
I reported the problem in 2015, to solve that I put a target="_self" on every state link to force a page to reload and do not generate detached dom leaks, but would be great to activate normal transition again.

PS: I didn't migrate to 1.0 because I didn't find any way to load controller files with the route in a lazy way.

@christopherthielen
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeonanCarvalho 545 is closed because nobody has been able to post an example of a memory leak caused by ui-router.

I ran your example plunker and I don't see any obvious memory leaks, nor dom node leaks.

screen shot 2018-01-24 at 10 27 01 am

Did you read my comment discussing how to post an example reproduction? angular-ui/ui-router#545 (comment)

Please sign in to comment.