Skip to content

Commit f8735d6

Browse files
authored
fix: ignore late ICE candidates after cleanup for RN speech detector (#2193)
### 💡 Overview Properly remove event listeners early in RN-speech-detector and avoid sending invalid candidates to native side <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced reliability of speech detection cleanup and shutdown behavior with more robust event listener management. * Improved error handling and logging for ICE candidate forwarding operations. * Fixed potential issues with repeated shutdown attempts through idempotent stop behavior. * **Tests** * Added comprehensive test coverage for speech detection listener lifecycle and cleanup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent b54f68f commit f8735d6

File tree

2 files changed

+92
-14
lines changed

2 files changed

+92
-14
lines changed

packages/client/src/helpers/RNSpeechDetector.ts

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@ import { SoundStateChangeHandler } from './sound-detector';
33
import { videoLoggerSystem } from '../logger';
44

55
export class RNSpeechDetector {
6-
private pc1 = new RTCPeerConnection({});
7-
private pc2 = new RTCPeerConnection({});
6+
private readonly pc1 = new RTCPeerConnection({});
7+
private readonly pc2 = new RTCPeerConnection({});
88
private audioStream: MediaStream | undefined;
99
private externalAudioStream: MediaStream | undefined;
10+
private isStopped = false;
1011

1112
constructor(externalAudioStream?: MediaStream) {
1213
this.externalAudioStream = externalAudioStream;
@@ -17,30 +18,31 @@ export class RNSpeechDetector {
1718
*/
1819
public async start(onSoundDetectedStateChanged: SoundStateChangeHandler) {
1920
try {
21+
this.isStopped = false;
2022
const audioStream =
2123
this.externalAudioStream != null
2224
? this.externalAudioStream
2325
: await navigator.mediaDevices.getUserMedia({ audio: true });
2426
this.audioStream = audioStream;
2527

26-
this.pc1.addEventListener('icecandidate', (e) => {
27-
this.pc2.addIceCandidate(e.candidate).catch(() => {
28-
// do nothing
29-
});
30-
});
31-
this.pc2.addEventListener('icecandidate', async (e) => {
32-
this.pc1.addIceCandidate(e.candidate).catch(() => {
33-
// do nothing
34-
});
35-
});
36-
this.pc2.addEventListener('track', (e) => {
28+
const onPc1IceCandidate = (e: RTCPeerConnectionIceEvent) => {
29+
this.forwardIceCandidate(this.pc2, e.candidate);
30+
};
31+
const onPc2IceCandidate = (e: RTCPeerConnectionIceEvent) => {
32+
this.forwardIceCandidate(this.pc1, e.candidate);
33+
};
34+
const onTrackPc2 = (e: RTCTrackEvent) => {
3735
e.streams[0].getTracks().forEach((track) => {
3836
// In RN, the remote track is automatically added to the audio output device
3937
// so we need to mute it to avoid hearing the audio back
4038
// @ts-expect-error _setVolume is a private method in react-native-webrtc
4139
track._setVolume(0);
4240
});
43-
});
41+
};
42+
43+
this.pc1.addEventListener('icecandidate', onPc1IceCandidate);
44+
this.pc2.addEventListener('icecandidate', onPc2IceCandidate);
45+
this.pc2.addEventListener('track', onTrackPc2);
4446

4547
audioStream
4648
.getTracks()
@@ -55,6 +57,9 @@ export class RNSpeechDetector {
5557
onSoundDetectedStateChanged,
5658
);
5759
return () => {
60+
this.pc1.removeEventListener('icecandidate', onPc1IceCandidate);
61+
this.pc2.removeEventListener('icecandidate', onPc2IceCandidate);
62+
this.pc2.removeEventListener('track', onTrackPc2);
5863
unsubscribe();
5964
this.stop();
6065
};
@@ -69,6 +74,9 @@ export class RNSpeechDetector {
6974
* Stops the speech detection and releases all allocated resources.
7075
*/
7176
private stop() {
77+
if (this.isStopped) return;
78+
this.isStopped = true;
79+
7280
this.pc1.close();
7381
this.pc2.close();
7482

@@ -185,4 +193,22 @@ export class RNSpeechDetector {
185193
this.audioStream.release();
186194
}
187195
}
196+
197+
private forwardIceCandidate(
198+
destination: RTCPeerConnection,
199+
candidate: RTCIceCandidate | null,
200+
) {
201+
if (
202+
this.isStopped ||
203+
!candidate ||
204+
destination.signalingState === 'closed'
205+
) {
206+
return;
207+
}
208+
destination.addIceCandidate(candidate).catch(() => {
209+
// silently ignore the error
210+
const logger = videoLoggerSystem.getLogger('RNSpeechDetector');
211+
logger.info('cannot add ice candidate - ignoring');
212+
});
213+
}
188214
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import '../../rtc/__tests__/mocks/webrtc.mocks';
2+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
3+
import { RNSpeechDetector } from '../RNSpeechDetector';
4+
5+
describe('RNSpeechDetector', () => {
6+
// Shared test setup stubs RTCPeerConnection with a vi.fn constructor.
7+
// We keep a typed handle to that constructor to inspect created instances.
8+
let rtcPeerConnectionMockCtor: ReturnType<typeof vi.fn>;
9+
10+
beforeEach(() => {
11+
rtcPeerConnectionMockCtor =
12+
globalThis.RTCPeerConnection as unknown as ReturnType<typeof vi.fn>;
13+
rtcPeerConnectionMockCtor.mockClear();
14+
});
15+
16+
afterEach(() => {
17+
vi.restoreAllMocks();
18+
});
19+
20+
it('ignores late ICE candidates after cleanup', async () => {
21+
const stream = {
22+
getTracks: () => [],
23+
} as unknown as MediaStream;
24+
const detector = new RNSpeechDetector(stream);
25+
26+
const cleanup = await detector.start(() => {});
27+
cleanup();
28+
29+
// start() creates two peer connections (pc1 and pc2). We pull them from
30+
// constructor call results to inspect listener wiring and ICE forwarding.
31+
const [pc1, pc2] = rtcPeerConnectionMockCtor.mock.results.map(
32+
(result) => result.value,
33+
);
34+
35+
// Find the registered ICE callback and invoke it manually after cleanup to
36+
// simulate a late ICE event arriving during teardown.
37+
const onIceCandidate = pc1.addEventListener.mock.calls.find(
38+
([eventName]: [string]) => eventName === 'icecandidate',
39+
)?.[1] as ((e: RTCPeerConnectionIceEvent) => void) | undefined;
40+
41+
expect(onIceCandidate).toBeDefined();
42+
onIceCandidate?.({
43+
candidate: { candidate: 'candidate:1 1 UDP 0 127.0.0.1 11111 typ host' },
44+
} as unknown as RTCPeerConnectionIceEvent);
45+
46+
expect(pc1.removeEventListener).toHaveBeenCalledWith(
47+
'icecandidate',
48+
onIceCandidate,
49+
);
50+
expect(pc2.addIceCandidate).not.toHaveBeenCalled();
51+
});
52+
});

0 commit comments

Comments
 (0)