diff --git a/src/livekit/useECConnectionState.test.tsx b/src/livekit/useECConnectionState.test.tsx index 5f2f6064..72324884 100644 --- a/src/livekit/useECConnectionState.test.tsx +++ b/src/livekit/useECConnectionState.test.tsx @@ -6,7 +6,7 @@ Please see LICENSE in the repository root for full details. */ import { type FC, useCallback, useState } from "react"; -import { test, vi } from "vitest"; +import { describe, expect, test, vi, vitest } from "vitest"; import { ConnectionError, ConnectionErrorReason, @@ -15,6 +15,7 @@ import { import userEvent from "@testing-library/user-event"; import { render, screen } from "@testing-library/react"; import { MemoryRouter } from "react-router-dom"; +import { defer, sleep } from "matrix-js-sdk/lib/utils"; import { useECConnectionState } from "./useECConnectionState"; import { type SFUConfig } from "./openIDSFU"; @@ -57,7 +58,7 @@ test.each<[string, ConnectionError]>([ () => setSfuConfig({ url: "URL", jwt: "JWT token" }), [], ); - useECConnectionState({}, false, mockRoom, sfuConfig); + useECConnectionState("default", false, mockRoom, sfuConfig); return ; }; @@ -73,3 +74,111 @@ test.each<[string, ConnectionError]>([ screen.getByText("Insufficient capacity"); }, ); + +describe("Leaking connection prevention", () => { + function createTestComponent(mockRoom: Room): FC { + const TestComponent: FC = () => { + const [sfuConfig, setSfuConfig] = useState( + undefined, + ); + const connect = useCallback( + () => setSfuConfig({ url: "URL", jwt: "JWT token" }), + [], + ); + useECConnectionState("default", false, mockRoom, sfuConfig); + return ; + }; + return TestComponent; + } + + test("Should cancel pending connections when the component is unmounted", async () => { + const connectCall = vi.fn(); + const pendingConnection = defer(); + // let pendingDisconnection = defer() + const disconnectMock = vi.fn(); + + const mockRoom = { + on: () => {}, + off: () => {}, + once: () => {}, + connect: async () => { + connectCall.call(undefined); + return await pendingConnection.promise; + }, + disconnect: disconnectMock, + localParticipant: { + getTrackPublication: () => {}, + createTracks: () => [], + }, + } as unknown as Room; + + const TestComponent = createTestComponent(mockRoom); + + const { unmount } = render(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: "Connect" })); + + expect(connectCall).toHaveBeenCalled(); + // unmount while the connection is pending + unmount(); + + // resolve the pending connection + pendingConnection.resolve(); + + await vitest.waitUntil( + () => { + return disconnectMock.mock.calls.length > 0; + }, + { + timeout: 1000, + interval: 100, + }, + ); + + // There should be some cleaning up to avoid leaking an open connection + expect(disconnectMock).toHaveBeenCalledTimes(1); + }); + + test("Should cancel about to open but not yet opened connection", async () => { + const createTracksCall = vi.fn(); + const pendingCreateTrack = defer(); + // let pendingDisconnection = defer() + const disconnectMock = vi.fn(); + const connectMock = vi.fn(); + + const mockRoom = { + on: () => {}, + off: () => {}, + once: () => {}, + connect: connectMock, + disconnect: disconnectMock, + localParticipant: { + getTrackPublication: () => {}, + createTracks: async () => { + createTracksCall.call(undefined); + await pendingCreateTrack.promise; + return []; + }, + }, + } as unknown as Room; + + const TestComponent = createTestComponent(mockRoom); + + const { unmount } = render(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: "Connect" })); + + expect(createTracksCall).toHaveBeenCalled(); + // unmount while createTracks is pending + unmount(); + + // resolve createTracks + pendingCreateTrack.resolve(); + + // Yield to the event loop to let the connection attempt finish + await sleep(100); + + // The operation should have been aborted before even calling connect. + expect(connectMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/livekit/useECConnectionState.ts b/src/livekit/useECConnectionState.ts index fa9a3038..3c7b91f8 100644 --- a/src/livekit/useECConnectionState.ts +++ b/src/livekit/useECConnectionState.ts @@ -6,7 +6,6 @@ Please see LICENSE in the repository root for full details. */ import { - type AudioCaptureOptions, ConnectionError, ConnectionState, type LocalTrack, @@ -25,6 +24,7 @@ import { InsufficientCapacityError, UnknownCallError, } from "../utils/errors.ts"; +import { AbortHandle } from "../utils/abortHandle.ts"; declare global { interface Window { @@ -59,7 +59,8 @@ async function doConnect( livekitRoom: Room, sfuConfig: SFUConfig, audioEnabled: boolean, - audioOptions: AudioCaptureOptions, + initialDeviceId: string | undefined, + abortHandle: AbortHandle, ): Promise { // Always create an audio track manually. // livekit (by default) keeps the mic track open when you mute, but if you start muted, @@ -82,19 +83,40 @@ async function doConnect( let preCreatedAudioTrack: LocalTrack | undefined; try { const audioTracks = await livekitRoom!.localParticipant.createTracks({ - audio: audioOptions, + audio: { deviceId: initialDeviceId }, }); + if (audioTracks.length < 1) { logger.info("Tried to pre-create local audio track but got no tracks"); } else { preCreatedAudioTrack = audioTracks[0]; } + // There was a yield point previously (awaiting for the track to be created) so we need to check + // if the operation was cancelled and stop connecting if needed. + if (abortHandle.isAborted()) { + logger.info( + "[Lifecycle] Signal Aborted: Pre-created audio track but connection aborted", + ); + preCreatedAudioTrack?.stop(); + return; + } + logger.info("Pre-created microphone track"); } catch (e) { logger.error("Failed to pre-create microphone track", e); } - if (!audioEnabled) await preCreatedAudioTrack?.mute(); + if (!audioEnabled) { + await preCreatedAudioTrack?.mute(); + // There was a yield point. Check if the operation was cancelled and stop connecting. + if (abortHandle.isAborted()) { + logger.info( + "[Lifecycle] Signal Aborted: Pre-created audio track but connection aborted", + ); + preCreatedAudioTrack?.stop(); + return; + } + } // check again having awaited for the track to create if ( @@ -107,9 +129,18 @@ async function doConnect( return; } - logger.info("Connecting & publishing"); + logger.info("[Lifecycle] Connecting & publishing"); try { await connectAndPublish(livekitRoom, sfuConfig, preCreatedAudioTrack, []); + if (abortHandle.isAborted()) { + logger.info( + "[Lifecycle] Signal Aborted: Connected but operation was cancelled. Force disconnect", + ); + livekitRoom?.disconnect().catch((err) => { + logger.error("Failed to disconnect from SFU", err); + }); + return; + } } catch (e) { preCreatedAudioTrack?.stop(); logger.debug("Stopped precreated audio tracks."); @@ -137,13 +168,16 @@ async function connectAndPublish( livekitRoom.once(RoomEvent.SignalConnected, tracker.cacheWsConnect); try { + logger.info(`[Lifecycle] Connecting to livekit room ${sfuConfig!.url} ...`); await livekitRoom!.connect(sfuConfig!.url, sfuConfig!.jwt, { // Due to stability issues on Firefox we are testing the effect of different // timeouts, and allow these values to be set through the console peerConnectionTimeout: window.peerConnectionTimeout ?? 45000, websocketTimeout: window.websocketTimeout ?? 45000, }); + logger.info(`[Lifecycle] ... connected to livekit room`); } catch (e) { + logger.error("[Lifecycle] Failed to connect", e); // LiveKit uses 503 to indicate that the server has hit its track limits. // https://github.com/livekit/livekit/blob/fcb05e97c5a31812ecf0ca6f7efa57c485cea9fb/pkg/service/rtcservice.go#L171 // It also errors with a status code of 200 (yes, really) for room @@ -184,7 +218,7 @@ async function connectAndPublish( } export function useECConnectionState( - initialAudioOptions: AudioCaptureOptions, + initialDeviceId: string | undefined, initialAudioEnabled: boolean, livekitRoom?: Room, sfuConfig?: SFUConfig, @@ -247,6 +281,22 @@ export function useECConnectionState( const currentSFUConfig = useRef(Object.assign({}, sfuConfig)); + // Protection against potential leaks, where the component to be unmounted and there is + // still a pending doConnect promise. This would lead the user to still be in the call even + // if the component is unmounted. + const abortHandlesBag = useRef(new Set()); + + // This is a cleanup function that will be called when the component is about to be unmounted. + // It will cancel all abortHandles in the bag + useEffect(() => { + const bag = abortHandlesBag.current; + return (): void => { + bag.forEach((handle) => { + handle.abort(); + }); + }; + }, []); + // Id we are transitioning from a valid config to another valid one, we need // to explicitly switch focus useEffect(() => { @@ -273,11 +323,14 @@ export function useECConnectionState( // always capturing audio: it helps keep bluetooth headsets in the right mode and // mobile browsers to know we're doing a call. setIsInDoConnect(true); + const abortHandle = new AbortHandle(); + abortHandlesBag.current.add(abortHandle); doConnect( livekitRoom!, sfuConfig!, initialAudioEnabled, - initialAudioOptions, + initialDeviceId, + abortHandle, ) .catch((e) => { if (e instanceof ElementCallError) { @@ -286,14 +339,17 @@ export function useECConnectionState( setError(new UnknownCallError(e)); } else logger.error("Failed to connect to SFU", e); }) - .finally(() => setIsInDoConnect(false)); + .finally(() => { + abortHandlesBag.current.delete(abortHandle); + setIsInDoConnect(false); + }); } currentSFUConfig.current = Object.assign({}, sfuConfig); }, [ sfuConfig, livekitRoom, - initialAudioOptions, + initialDeviceId, initialAudioEnabled, doFocusSwitch, ]); diff --git a/src/livekit/useLiveKit.ts b/src/livekit/useLiveKit.ts index 99eda021..972f7756 100644 --- a/src/livekit/useLiveKit.ts +++ b/src/livekit/useLiveKit.ts @@ -155,9 +155,7 @@ export function useLiveKit( ); const connectionState = useECConnectionState( - { - deviceId: initialDevices.current.audioInput.selectedId, - }, + initialDevices.current.audioInput.selectedId, initialMuteStates.current.audio.enabled, room, sfuConfig, diff --git a/src/room/GroupCallView.tsx b/src/room/GroupCallView.tsx index 0d727485..4ea356bf 100644 --- a/src/room/GroupCallView.tsx +++ b/src/room/GroupCallView.tsx @@ -118,6 +118,13 @@ export const GroupCallView: FC = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, []); + useEffect(() => { + logger.info("[Lifecycle] GroupCallView Component mounted"); + return (): void => { + logger.info("[Lifecycle] GroupCallView Component unmounted"); + }; + }, []); + useEffect(() => { window.rtcSession = rtcSession; return (): void => { diff --git a/src/room/InCallView.tsx b/src/room/InCallView.tsx index b434a1da..c337bc3c 100644 --- a/src/room/InCallView.tsx +++ b/src/room/InCallView.tsx @@ -127,10 +127,23 @@ export const ActiveCall: FC = (props) => { const [vm, setVm] = useState(null); useEffect(() => { + logger.info( + `[Lifecycle] InCallView Component mounted, livekitroom state ${livekitRoom?.state}`, + ); return (): void => { - livekitRoom?.disconnect().catch((e) => { - logger.error("Failed to disconnect from livekit room", e); - }); + logger.info( + `[Lifecycle] InCallView Component unmounted, livekitroom state ${livekitRoom?.state}`, + ); + livekitRoom + ?.disconnect() + .then(() => { + logger.info( + `[Lifecycle] Disconnected from livekite room, state:${livekitRoom?.state}`, + ); + }) + .catch((e) => { + logger.error("[Lifecycle] Failed to disconnect from livekit room", e); + }); }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); diff --git a/src/room/LobbyView.tsx b/src/room/LobbyView.tsx index 0cabc645..72079783 100644 --- a/src/room/LobbyView.tsx +++ b/src/room/LobbyView.tsx @@ -5,7 +5,14 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial Please see LICENSE in the repository root for full details. */ -import { type FC, useCallback, useMemo, useState, type JSX } from "react"; +import { + type FC, + useCallback, + useMemo, + useState, + type JSX, + useEffect, +} from "react"; import { useTranslation } from "react-i18next"; import { type MatrixClient } from "matrix-js-sdk"; import { Button } from "@vector-im/compound-web"; @@ -72,6 +79,13 @@ export const LobbyView: FC = ({ onShareClick, waitingForInvite, }) => { + useEffect(() => { + logger.info("[Lifecycle] GroupCallView Component mounted"); + return (): void => { + logger.info("[Lifecycle] GroupCallView Component unmounted"); + }; + }, []); + const { t } = useTranslation(); usePageTitle(matrixInfo.roomName); diff --git a/src/utils/abortHandle.ts b/src/utils/abortHandle.ts new file mode 100644 index 00000000..f4bb2ef5 --- /dev/null +++ b/src/utils/abortHandle.ts @@ -0,0 +1,18 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE in the repository root for full details. +*/ + +export class AbortHandle { + public constructor(private aborted = false) {} + + public abort(): void { + this.aborted = true; + } + + public isAborted(): boolean { + return this.aborted; + } +}