diff --git a/.changeset/fix-recorder-interval-memory-leak.md b/.changeset/fix-recorder-interval-memory-leak.md new file mode 100644 index 0000000000000..78e324af2b9da --- /dev/null +++ b/.changeset/fix-recorder-interval-memory-leak.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': patch +--- + +Fixes memory leak in `VideoMessageRecorder` and `AudioMessageRecorder` caused by `setInterval` not being cleared on component unmount. diff --git a/apps/meteor/client/lib/openCASLoginPopup.spec.ts b/apps/meteor/client/lib/openCASLoginPopup.spec.ts new file mode 100644 index 0000000000000..ef27ba66ca4d6 --- /dev/null +++ b/apps/meteor/client/lib/openCASLoginPopup.spec.ts @@ -0,0 +1,78 @@ +import { Meteor } from 'meteor/meteor'; + +import { openCASLoginPopup } from './openCASLoginPopup'; + +const mockPeek = jest.fn(); + +jest.mock('./settings', () => ({ + settings: { + peek: (...args: unknown[]) => mockPeek(...args), + }, +})); + +jest.mock('meteor/meteor', () => ({ + Meteor: { + absoluteUrl: jest.fn(), + }, +})); + +describe('openCASLoginPopup', () => { + const originalRuntimeConfig = (global as any).__meteor_runtime_config__; + let openSpy: jest.SpyInstance; + + beforeEach(() => { + jest.useFakeTimers(); + jest.clearAllMocks(); + mockPeek.mockImplementation((key: string) => { + switch (key) { + case 'CAS_login_url': + return 'https://cas.example.com/login'; + case 'CAS_popup_width': + return 800; + case 'CAS_popup_height': + return 600; + default: + return undefined; + } + }); + + (Meteor.absoluteUrl as jest.Mock).mockReturnValue('https://chat.example.com/'); + (global as any).__meteor_runtime_config__ = { ROOT_URL_PATH_PREFIX: '' }; + openSpy = jest.spyOn(window, 'open').mockReturnValue({ focus: jest.fn(), closed: false } as unknown as Window); + }); + + afterEach(() => { + jest.useRealTimers(); + openSpy.mockRestore(); + (global as any).__meteor_runtime_config__ = originalRuntimeConfig; + }); + + it('resolves when the popup closes', async () => { + const popup = { focus: jest.fn(), closed: false } as unknown as Window; + (window.open as jest.Mock).mockReturnValue(popup); + + const promise = openCASLoginPopup('credential-token'); + + await jest.advanceTimersByTimeAsync(100); + (popup as { closed: boolean }).closed = true; + await jest.advanceTimersByTimeAsync(100); + + await expect(promise).resolves.toBeUndefined(); + expect(window.open).toHaveBeenCalledWith( + 'https://cas.example.com/login?service=https%3A%2F%2Fchat.example.com%2F_cas%2Fcredential-token', + 'Login', + expect.stringContaining('width=800'), + ); + }); + + it('rejects if the popup stays open too long', async () => { + const popup = { focus: jest.fn(), closed: false } as unknown as Window; + (window.open as jest.Mock).mockReturnValue(popup); + + const promise = openCASLoginPopup('credential-token'); + + await jest.advanceTimersByTimeAsync(5 * 60 * 1000); + + await expect(promise).rejects.toThrow('CAS login popup did not close in time'); + }); +}); diff --git a/apps/meteor/client/lib/openCASLoginPopup.ts b/apps/meteor/client/lib/openCASLoginPopup.ts index 836ec66b91202..434db05169142 100644 --- a/apps/meteor/client/lib/openCASLoginPopup.ts +++ b/apps/meteor/client/lib/openCASLoginPopup.ts @@ -2,6 +2,8 @@ import { Meteor } from 'meteor/meteor'; import { settings } from './settings'; +const POPUP_CLOSE_TIMEOUT_MS = 5 * 60 * 1000; + const openCenteredPopup = (url: string, width: number, height: number) => { const screenX = window.screenX ?? window.screenLeft; const screenY = window.screenY ?? window.screenTop; @@ -41,13 +43,19 @@ const getPopupUrl = (credentialToken: string): string => { }; const waitForPopupClose = (popup: Window) => { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const checkPopupOpen = setInterval(() => { if (popup.closed || popup.closed === undefined) { clearInterval(checkPopupOpen); + clearTimeout(timeoutId); resolve(); } }, 100); + + const timeoutId = setTimeout(() => { + clearInterval(checkPopupOpen); + reject(new Error('CAS login popup did not close in time')); + }, POPUP_CLOSE_TIMEOUT_MS); }); }; diff --git a/apps/meteor/client/views/composer/AudioMessageRecorder/AudioMessageRecorder.tsx b/apps/meteor/client/views/composer/AudioMessageRecorder/AudioMessageRecorder.tsx index 543c975eda01a..61d386b673891 100644 --- a/apps/meteor/client/views/composer/AudioMessageRecorder/AudioMessageRecorder.tsx +++ b/apps/meteor/client/views/composer/AudioMessageRecorder/AudioMessageRecorder.tsx @@ -3,7 +3,7 @@ import { Box, Icon, Throbber } from '@rocket.chat/fuselage'; import { useEffectEvent } from '@rocket.chat/fuselage-hooks'; import { MessageComposerAction } from '@rocket.chat/ui-composer'; import type { ReactElement } from 'react'; -import { useEffect, useState } from 'react'; +import { useEffect, useState, useRef } from 'react'; import { useTranslation } from 'react-i18next'; import { AudioRecorder } from '../../../../app/ui/client/lib/recorderjs/AudioRecorder'; @@ -21,14 +21,18 @@ const AudioMessageRecorder = ({ rid, isMicrophoneDenied }: AudioMessageRecorderP const [state, setState] = useState<'loading' | 'recording'>('recording'); const [time, setTime] = useState('00:00'); - const [recordingInterval, setRecordingInterval] = useState | null>(null); + const recordingIntervalRef = useRef | null>(null); const [recordingRoomId, setRecordingRoomId] = useState(null); - const stopRecording = useEffectEvent(async () => { - if (recordingInterval) { - clearInterval(recordingInterval); + const clearRecordingInterval = () => { + if (recordingIntervalRef.current) { + clearInterval(recordingIntervalRef.current); + recordingIntervalRef.current = null; } - setRecordingInterval(null); + }; + + const stopRecording = useEffectEvent(async () => { + clearRecordingInterval(); setRecordingRoomId(null); setTime('00:00'); @@ -42,9 +46,12 @@ const AudioMessageRecorder = ({ rid, isMicrophoneDenied }: AudioMessageRecorderP return blob; }); - const handleUnmount = useEffectEvent(async () => { + const handleUnmount = useEffectEvent(() => { + clearRecordingInterval(); if (state === 'recording') { - await stopRecording(); + audioRecorder.stop(() => undefined); + chat?.action.stop('recording'); + chat?.composer?.setRecordingMode(false); } }); @@ -59,18 +66,16 @@ const AudioMessageRecorder = ({ rid, isMicrophoneDenied }: AudioMessageRecorderP await audioRecorder.start(); chat?.action.performContinuously('recording'); const startTime = new Date(); - setRecordingInterval( - setInterval(() => { - const now = new Date(); - const distance = (now.getTime() - startTime.getTime()) / 1000; - const minutes = Math.floor(distance / 60); - const seconds = Math.floor(distance % 60); - setTime(`${String(minutes).padStart(2, '0')}:${String(seconds).padStart(2, '0')}`); - }, 1000), - ); + recordingIntervalRef.current = setInterval(() => { + const now = new Date(); + const distance = (now.getTime() - startTime.getTime()) / 1000; + const minutes = Math.floor(distance / 60); + const seconds = Math.floor(distance % 60); + setTime(`${String(minutes).padStart(2, '0')}:${String(seconds).padStart(2, '0')}`); + }, 1000); setRecordingRoomId(rid); } catch (error) { - console.log(error); + console.error(error); chat?.composer?.setRecordingMode(false); } }); @@ -100,6 +105,14 @@ const AudioMessageRecorder = ({ rid, isMicrophoneDenied }: AudioMessageRecorderP }; }, [handleUnmount, handleRecord]); + // Ensure interval is cleared on unmount as a safety net + useEffect(() => { + return () => { + clearRecordingInterval(); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + if (isMicrophoneDenied) { return null; } diff --git a/apps/meteor/client/views/composer/VideoMessageRecorder/VideoMessageRecorder.tsx b/apps/meteor/client/views/composer/VideoMessageRecorder/VideoMessageRecorder.tsx index 5952c35da15e3..b0f2ec2c2cd24 100644 --- a/apps/meteor/client/views/composer/VideoMessageRecorder/VideoMessageRecorder.tsx +++ b/apps/meteor/client/views/composer/VideoMessageRecorder/VideoMessageRecorder.tsx @@ -4,7 +4,7 @@ import { Box, ButtonGroup, Button, Icon, PositionAnimated } from '@rocket.chat/f import { useEffectEvent } from '@rocket.chat/fuselage-hooks'; import { useTranslation, useToastMessageDispatch } from '@rocket.chat/ui-contexts'; import type { AllHTMLAttributes, RefObject } from 'react'; -import { useRef, useEffect, useState } from 'react'; +import { useRef, useEffect, useState, useCallback } from 'react'; import { UserAction, USER_ACTIVITIES } from '../../../../app/ui/client/lib/UserAction'; import { VideoRecorder } from '../../../../app/ui/client/lib/recorderjs/videoRecorder'; @@ -43,23 +43,30 @@ const VideoMessageRecorder = ({ rid, tmid, reference }: VideoMessageRecorderProp const [time, setTime] = useState('00:00'); const [recordingState, setRecordingState] = useState<'idle' | 'loading' | 'recording'>('idle'); - const [recordingInterval, setRecordingInterval] = useState | null>(null); + const recordingIntervalRef = useRef | null>(null); const isRecording = recordingState === 'recording'; const sendButtonDisabled = !(VideoRecorder.cameraStarted.get() && !(recordingState === 'recording')); const chat = useChat(); - const stopVideoRecording = async (rid: IRoom['_id'], tmid?: IMessage['_id']) => { - if (recordingInterval) { - clearInterval(recordingInterval); + const clearRecordingInterval = useCallback(() => { + if (recordingIntervalRef.current) { + clearInterval(recordingIntervalRef.current); + recordingIntervalRef.current = null; } - setRecordingInterval(null); - VideoRecorder.stopRecording(); - UserAction.stop(rid, USER_ACTIVITIES.USER_RECORDING, { tmid }); - setRecordingState('idle'); - }; + }, []); + + const stopVideoRecording = useCallback( + (rid: IRoom['_id'], tmid?: IMessage['_id']) => { + clearRecordingInterval(); + VideoRecorder.stopRecording(); + UserAction.stop(rid, USER_ACTIVITIES.USER_RECORDING, { tmid }); + setRecordingState('idle'); + }, + [clearRecordingInterval], + ); - const handleRecord = async () => { + const handleRecord = useCallback(async () => { if (recordingState === 'recording') { stopVideoRecording(rid, tmid); } else { @@ -68,17 +75,15 @@ const VideoMessageRecorder = ({ rid, tmid, reference }: VideoMessageRecorderProp UserAction.performContinuously(rid, USER_ACTIVITIES.USER_RECORDING, { tmid }); setTime('00:00'); const startTime = new Date(); - setRecordingInterval( - setInterval(() => { - const now = new Date(); - const distance = (now.getTime() - startTime.getTime()) / 1000; - const minutes = Math.floor(distance / 60); - const seconds = Math.floor(distance % 60); - setTime(`${String(minutes).padStart(2, '0')}:${String(seconds).padStart(2, '0')}`); - }, 1000), - ); + recordingIntervalRef.current = setInterval(() => { + const now = new Date(); + const distance = (now.getTime() - startTime.getTime()) / 1000; + const minutes = Math.floor(distance / 60); + const seconds = Math.floor(distance % 60); + setTime(`${String(minutes).padStart(2, '0')}:${String(seconds).padStart(2, '0')}`); + }, 1000); } - }; + }, [recordingState, rid, tmid, stopVideoRecording]); const handleSendRecord = async () => { const cb = async (blob: Blob) => { @@ -108,9 +113,11 @@ const VideoMessageRecorder = ({ rid, tmid, reference }: VideoMessageRecorderProp VideoRecorder.start(videoRef.current ?? undefined, (success) => (!success ? handleCancel() : undefined)); return () => { + clearRecordingInterval(); VideoRecorder.stop(); + UserAction.stop(rid, USER_ACTIVITIES.USER_RECORDING, { tmid }); }; - }, [dispatchToastMessage, handleCancel, t]); + }, [dispatchToastMessage, handleCancel, clearRecordingInterval, rid, tmid, t]); return (