Skip to content

Commit 33b001d

Browse files
AleksanderBodurriatscott
authored andcommitted
fix(devtools): clean up removed directive entries in IdentityTracker to prevent memory leak
The IdentityTracker singleton never deleted entries from its internal maps (_currentDirectiveId, _currentDirectivePosition, isComponent) for destroyed directives. This caused the maps to grow monotonically for the entire DevTools session, retaining references to destroyed component instances and preventing garbage collection. The cleanup was intentionally commented out because the profiler needs to resolve IDs and positions of removed components during recording. Introduce `setProfilingActive()` to gate cleanup: when profiling is inactive, removed entries are deleted immediately during `index()`. When profiling is active, removals are deferred into a `_pendingRemovals` set and flushed once profiling stops via `capture.ts` start/stop calls.
1 parent 2cb67c0 commit 33b001d

3 files changed

Lines changed: 205 additions & 4 deletions

File tree

devtools/projects/ng-devtools-backend/src/lib/hooks/capture.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {isCustomElement, runOutsideAngular} from '../utils';
2121

2222
import {initializeOrGetDirectiveForestHooks} from '.';
2323
import {DirectiveForestHooks} from './hooks';
24+
import {IdentityTracker} from './identity-tracker';
2425
import {Hooks} from './profiler';
2526

2627
let inProgress = false;
@@ -40,6 +41,7 @@ export const start = (onFrame: (frame: ProfilerFrame) => void): void => {
4041
}
4142
eventMap = new Map<any, DirectiveProfile>();
4243
inProgress = true;
44+
IdentityTracker.getInstance().setProfilingActive(true);
4345
hooks = getHooks(onFrame);
4446
initializeOrGetDirectiveForestHooks().profiler.subscribe(hooks);
4547
};
@@ -50,6 +52,7 @@ export const stop = (): ProfilerFrame => {
5052
initializeOrGetDirectiveForestHooks().profiler.unsubscribe(hooks);
5153
hooks = {};
5254
inProgress = false;
55+
IdentityTracker.getInstance().setProfilingActive(false);
5356
return result;
5457
};
5558

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.dev/license
7+
*/
8+
9+
import {IdentityTracker} from './identity-tracker';
10+
11+
describe('IdentityTracker', () => {
12+
let tracker: IdentityTracker;
13+
14+
beforeEach(() => {
15+
(IdentityTracker as any)._instance = undefined;
16+
tracker = IdentityTracker.getInstance();
17+
});
18+
19+
afterEach(() => {
20+
(IdentityTracker as any)._instance = undefined;
21+
document.querySelectorAll('[ng-version]').forEach((el) => el.remove());
22+
});
23+
24+
function seedDirective(opts: {dir: object; id: number; isComponent?: boolean}): void {
25+
const internal = tracker as any;
26+
internal._currentDirectiveId.set(opts.dir, opts.id);
27+
internal._currentDirectivePosition.set(opts.dir, [opts.id]);
28+
internal.isComponent.set(opts.dir, opts.isComponent ?? false);
29+
}
30+
31+
describe('setProfilingActive', () => {
32+
it('removes pending directives from all maps when profiling stops', () => {
33+
const dir = {};
34+
seedDirective({dir, id: 0, isComponent: true});
35+
(tracker as any)._pendingRemovals.add(dir);
36+
37+
tracker.setProfilingActive(false);
38+
39+
expect(tracker.hasDirective(dir)).toBeFalse();
40+
expect(tracker.getDirectiveId(dir)).toBeUndefined();
41+
expect(tracker.getDirectivePosition(dir)).toBeUndefined();
42+
});
43+
44+
it('does not flush pending removals when profiling starts', () => {
45+
const dir = {};
46+
seedDirective({dir, id: 0, isComponent: true});
47+
(tracker as any)._pendingRemovals.add(dir);
48+
49+
tracker.setProfilingActive(true);
50+
51+
expect(tracker.hasDirective(dir)).toBeTrue();
52+
expect(tracker.getDirectiveId(dir)).toBe(0);
53+
expect(tracker.getDirectivePosition(dir)).toEqual([0]);
54+
});
55+
56+
it('clears the pending set after flushing', () => {
57+
const dir = {};
58+
(tracker as any)._pendingRemovals.add(dir);
59+
60+
tracker.setProfilingActive(false);
61+
62+
expect((tracker as any)._pendingRemovals.size).toBe(0);
63+
});
64+
65+
it('handles an empty pending set gracefully', () => {
66+
expect(() => tracker.setProfilingActive(false)).not.toThrow();
67+
});
68+
69+
it('flushes multiple pending directives at once', () => {
70+
const dirs = [{}, {}, {}];
71+
dirs.forEach((dir, i) => {
72+
seedDirective({dir, id: i});
73+
(tracker as any)._pendingRemovals.add(dir);
74+
});
75+
76+
tracker.setProfilingActive(false);
77+
78+
dirs.forEach((dir) => {
79+
expect(tracker.hasDirective(dir)).toBeFalse();
80+
});
81+
expect((tracker as any)._pendingRemovals.size).toBe(0);
82+
});
83+
});
84+
85+
describe('index() cleanup behavior', () => {
86+
it('immediately removes a stale directive from all maps when not profiling', () => {
87+
const dir = {};
88+
seedDirective({dir, id: 0});
89+
90+
tracker.index();
91+
92+
expect(tracker.hasDirective(dir)).toBeFalse();
93+
expect(tracker.getDirectiveId(dir)).toBeUndefined();
94+
expect(tracker.getDirectivePosition(dir)).toBeUndefined();
95+
});
96+
97+
it('keeps a stale directive in maps during profiling, staging it for deferred cleanup', () => {
98+
const dir = {};
99+
seedDirective({dir, id: 0});
100+
101+
tracker.setProfilingActive(true);
102+
tracker.index();
103+
104+
expect(tracker.hasDirective(dir)).toBeTrue();
105+
expect(tracker.getDirectiveId(dir)).toBe(0);
106+
expect(tracker.getDirectivePosition(dir)).toEqual([0]);
107+
expect((tracker as any)._pendingRemovals.has(dir)).toBeTrue();
108+
});
109+
110+
it('removes deferred directives from maps once profiling stops', () => {
111+
const dir = {};
112+
seedDirective({dir, id: 0});
113+
114+
tracker.setProfilingActive(true);
115+
tracker.index();
116+
117+
expect(tracker.hasDirective(dir)).toBeTrue();
118+
119+
tracker.setProfilingActive(false);
120+
121+
expect(tracker.hasDirective(dir)).toBeFalse();
122+
});
123+
124+
it('includes removed directives in the returned removedNodes regardless of profiling state', () => {
125+
const dir = {};
126+
seedDirective({dir, id: 0, isComponent: true});
127+
128+
const {removedNodes} = tracker.index();
129+
130+
expect(removedNodes.length).toBe(1);
131+
expect(removedNodes[0].directive).toBe(dir);
132+
expect(removedNodes[0].isComponent).toBeTrue();
133+
});
134+
135+
it('does not add an already-pending directive to removedNodes twice on the next index call', () => {
136+
const dir = {};
137+
seedDirective({dir, id: 0});
138+
139+
tracker.setProfilingActive(true);
140+
tracker.index();
141+
142+
const {removedNodes} = tracker.index();
143+
144+
expect(removedNodes.some((n) => n.directive === dir)).toBeTrue();
145+
});
146+
147+
it('grows maps monotonically during profiling and fully clears on stop', () => {
148+
const dirs = [{}, {}, {}];
149+
dirs.forEach((dir, i) => seedDirective({dir, id: i}));
150+
151+
tracker.setProfilingActive(true);
152+
tracker.index();
153+
154+
dirs.forEach((dir) => expect(tracker.hasDirective(dir)).toBeTrue());
155+
156+
tracker.setProfilingActive(false);
157+
158+
dirs.forEach((dir) => expect(tracker.hasDirective(dir)).toBeFalse());
159+
});
160+
});
161+
});

devtools/projects/ng-devtools-backend/src/lib/hooks/identity-tracker.ts

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ export class IdentityTracker {
3434
private _currentDirectiveId = new Map<any, number>();
3535
isComponent = new Map<any, boolean>();
3636

37+
/**
38+
* Directives that were removed while profiling was active.
39+
* Cleanup is deferred until profiling stops so that the profiler
40+
* can still look up IDs / positions of destroyed components.
41+
*/
42+
private _pendingRemovals = new Set<any>();
43+
private _isProfiling = false;
44+
3745
// private constructor for Singleton Pattern
3846
private constructor() {}
3947

@@ -56,6 +64,18 @@ export class IdentityTracker {
5664
return this._currentDirectiveId.has(dir);
5765
}
5866

67+
/**
68+
* Toggle profiling state. While profiling is active, removed directive
69+
* entries are kept so the profiler can still resolve IDs and positions.
70+
* When profiling stops, deferred removals are flushed.
71+
*/
72+
setProfilingActive(active: boolean): void {
73+
this._isProfiling = active;
74+
if (!active) {
75+
this._flushPendingRemovals();
76+
}
77+
}
78+
5979
index(): {
6080
newNodes: NodeArray;
6181
removedNodes: NodeArray;
@@ -71,10 +91,11 @@ export class IdentityTracker {
7191
this._currentDirectiveId.forEach((_: number, dir: any) => {
7292
if (!allNodes.has(dir)) {
7393
removedNodes.push({directive: dir, isComponent: !!this.isComponent.get(dir)});
74-
// We can't clean these up because during profiling
75-
// they might be requested for removed components
76-
// this._currentDirectiveId.delete(dir);
77-
// this._currentDirectivePosition.delete(dir);
94+
if (this._isProfiling) {
95+
this._pendingRemovals.add(dir);
96+
} else {
97+
this._cleanupDirective(dir);
98+
}
7899
}
79100
});
80101
return {newNodes, removedNodes, indexedForest, directiveForest};
@@ -110,9 +131,25 @@ export class IdentityTracker {
110131
}
111132
}
112133

134+
private _cleanupDirective(dir: any): void {
135+
this._currentDirectiveId.delete(dir);
136+
this._currentDirectivePosition.delete(dir);
137+
this.isComponent.delete(dir);
138+
}
139+
140+
private _flushPendingRemovals(): void {
141+
for (const dir of this._pendingRemovals) {
142+
this._cleanupDirective(dir);
143+
}
144+
this._pendingRemovals.clear();
145+
}
146+
113147
destroy(): void {
114148
this._currentDirectivePosition = new Map<any, ElementPosition>();
115149
this._currentDirectiveId = new Map<any, number>();
150+
this.isComponent = new Map<any, boolean>();
151+
this._pendingRemovals.clear();
152+
this._isProfiling = false;
116153
}
117154
}
118155

0 commit comments

Comments
 (0)