Skip to content

Commit 355eb8d

Browse files
committed
fix(devtools): clean up profiler subscriptions on destroy
Cleans up `uploadedData` and message bus subscriptions when the profiler is destroyed to prevent duplicate error dialogs and stale profiler results from accumulating across profiler instances. Adds coverage for subscription cleanup on destroy.
1 parent f29243f commit 355eb8d

3 files changed

Lines changed: 118 additions & 7 deletions

File tree

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/profiler/BUILD.bazel‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
load("//devtools/tools:defaults.bzl", "ng_project", "sass_binary")
1+
load("//devtools/tools:defaults.bzl", "ng_project", "sass_binary", "ts_test_library", "zoneless_web_test_suite")
22

33
package(default_visibility = ["//devtools:__subpackages__"])
44

@@ -30,3 +30,26 @@ ng_project(
3030
"//devtools/projects/protocol",
3131
],
3232
)
33+
34+
ts_test_library(
35+
name = "profiler_test",
36+
srcs = [
37+
"profiler.component.spec.ts",
38+
],
39+
visibility = [
40+
"//visibility:private",
41+
],
42+
deps = [
43+
":profiler",
44+
"//:node_modules/@angular/core",
45+
"//:node_modules/@angular/material",
46+
"//devtools/projects/protocol",
47+
],
48+
)
49+
50+
zoneless_web_test_suite(
51+
name = "test",
52+
deps = [
53+
":profiler_test",
54+
],
55+
)
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
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 {ComponentFixture, TestBed} from '@angular/core/testing';
10+
import {MatDialog} from '@angular/material/dialog';
11+
12+
import {Events, MessageBus} from '../../../../../protocol';
13+
import {FileApiService} from './file-api-service';
14+
import {ProfilerComponent} from './profiler.component';
15+
16+
class MessageBusMock implements Pick<MessageBus<Events>, 'on' | 'emit' | 'once' | 'destroy'> {
17+
readonly emit = jasmine.createSpy('emit');
18+
readonly once = jasmine.createSpy('once');
19+
readonly destroy = jasmine.createSpy('destroy');
20+
private listeners = new Map<keyof Events, Set<Function>>();
21+
22+
on = jasmine.createSpy('on').and.callFake((topic: keyof Events, cb: Function) => {
23+
if (!this.listeners.has(topic)) this.listeners.set(topic, new Set());
24+
this.listeners.get(topic)!.add(cb);
25+
return () => this.listeners.get(topic)!.delete(cb);
26+
});
27+
28+
hasListener(topic: keyof Events): boolean {
29+
return (this.listeners.get(topic)?.size ?? 0) > 0;
30+
}
31+
}
32+
33+
describe('ProfilerComponent', () => {
34+
let messageBus: MessageBusMock;
35+
let dialog: jasmine.SpyObj<MatDialog>;
36+
let fixture: ComponentFixture<ProfilerComponent>;
37+
38+
beforeEach(async () => {
39+
messageBus = new MessageBusMock();
40+
dialog = jasmine.createSpyObj<MatDialog>('MatDialog', ['open']);
41+
// Importing an invalid file logs the error.
42+
spyOn(console, 'error');
43+
44+
TestBed.configureTestingModule({
45+
imports: [ProfilerComponent],
46+
providers: [
47+
{provide: MessageBus, useValue: messageBus},
48+
{provide: MatDialog, useValue: dialog},
49+
],
50+
});
51+
52+
fixture = TestBed.createComponent(ProfilerComponent);
53+
await fixture.whenStable();
54+
});
55+
56+
it('should unregister the profiler message bus listeners on destroy', () => {
57+
expect(messageBus.hasListener('profilerResults')).toBeTrue();
58+
expect(messageBus.hasListener('sendProfilerChunk')).toBeTrue();
59+
60+
fixture.destroy();
61+
62+
expect(messageBus.hasListener('profilerResults')).toBeFalse();
63+
expect(messageBus.hasListener('sendProfilerChunk')).toBeFalse();
64+
});
65+
66+
it('should stop handling uploaded files once destroyed', () => {
67+
const fileApiService = TestBed.inject(FileApiService);
68+
69+
fileApiService.uploadedData.next({error: new Error('Invalid file')});
70+
expect(dialog.open).toHaveBeenCalledTimes(1);
71+
72+
fixture.destroy();
73+
fileApiService.uploadedData.next({error: new Error('Invalid file')});
74+
75+
expect(dialog.open).toHaveBeenCalledTimes(1);
76+
});
77+
});

‎devtools/projects/ng-devtools/src/lib/devtools-tabs/profiler/profiler.component.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {Component, inject, signal} from '@angular/core';
9+
import {Component, DestroyRef, inject, signal} from '@angular/core';
10+
import {takeUntilDestroyed} from '@angular/core/rxjs-interop';
1011
import {MatDialog} from '@angular/material/dialog';
1112
import {MatIcon} from '@angular/material/icon';
1213
import {MatTooltip} from '@angular/material/tooltip';
@@ -42,7 +43,9 @@ export class ProfilerComponent {
4243
public dialog = inject(MatDialog);
4344

4445
constructor() {
45-
this._fileApiService.uploadedData.subscribe((importedFile) => {
46+
// `FileApiService` is provided in root and outlives this component, which is destroyed and
47+
// recreated when another frame is inspected.
48+
this._fileApiService.uploadedData.pipe(takeUntilDestroyed()).subscribe((importedFile) => {
4649
if (importedFile.error) {
4750
console.error('Could not process uploaded file');
4851
console.error(importedFile.error);
@@ -84,16 +87,24 @@ export class ProfilerComponent {
8487
}
8588
});
8689

87-
this._messageBus.on('profilerResults', (remainingRecords) => {
90+
const unlistenProfilerResults = this._messageBus.on('profilerResults', (remainingRecords) => {
8891
if (remainingRecords.duration > 0 && remainingRecords.source) {
8992
this.stream.next([remainingRecords]);
9093
this._buffer.push(remainingRecords);
9194
}
9295
});
9396

94-
this._messageBus.on('sendProfilerChunk', (chunkOfRecords: ProfilerFrame) => {
95-
this.stream.next([chunkOfRecords]);
96-
this._buffer.push(chunkOfRecords);
97+
const unlistenProfilerChunk = this._messageBus.on(
98+
'sendProfilerChunk',
99+
(chunkOfRecords: ProfilerFrame) => {
100+
this.stream.next([chunkOfRecords]);
101+
this._buffer.push(chunkOfRecords);
102+
},
103+
);
104+
105+
inject(DestroyRef).onDestroy(() => {
106+
unlistenProfilerResults();
107+
unlistenProfilerChunk();
97108
});
98109
}
99110

0 commit comments

Comments
 (0)