T09 · Insecure Skill Coding Practices
Warning
- Location
assets/hooks/useGameLoop.ts:26- Finding
Duplicate requestAnimationFrame Chains Can Survive Component Unmount
- Content
View full analysis
{ isRunningRef.current = isRunning; }, [isRunning]); const rafRef = useRef(null); const lastTimestampRef = useRef(null); const elapsedTimeRef = useRef(0); const tick = useCallback((timestamp: number) => { if (!isRunningRef.current) return; if (lastTimestampRef.current !== null) { let delta = timestamp - lastTimestampRef.current; if (delta > maxDeltaTime) delta = maxDeltaTime; elapsedTimeRef.current += delta; try { callbackRef.current(delta, elapsedTimeRef.current); } catch (err) { // eslint-disable-next-line no-console console.error('useGameLoop callback error:', err); } } lastTimestampRef.current = timestamp; rafRef.current = requestAnimationFrame(tick); }, [maxDeltaTime]); const start = useCallback(() => { if (isRunningRef.current) return; setIsRunning(true); lastTimestampRef.current = null; // skip stale timestamp rafRef.current = requestAnimationFrame(tick); }, [tick]); const stop = useCallback(() => { if (!isRunningRef.current) return; setIsRunning(false); if (rafRef.current !== null) { cancelAnimationFrame(rafRef.current); rafRef.current = null; } }, []); // Cleanup on unmount useEffect(() => { return () => { if (rafRef.current !== null) { cancelAnimationFrame(rafRef.current); rafRef.current = null; } }; }, []); ``` ### Technical Analysis The `start()` function checks `isRunningRef.current`, but it does not synchronously update that ref before scheduling a frame. Instead, the ref is updated later by a React effect after `setIsRunning(true)` causes a render. Consequently, two or more synchronous calls to `start()` before the effect executes can all ...[truncated 2234 chars]- Remediation
View remediation
{ if (isRunningRef.current) return; isRunningRef.current = true; setIsRunning(true); lastTimestampRef.current = null; rafRef.current = requestAnimationFrame(tick); }, [tick]); const stop = useCallback(() => { if (!isRunningRef.current) return; isRunningRef.current = false; setIsRunning(false); if (rafRef.current !== null) { cancelAnimationFrame(rafRef.current); rafRef.current = null; } }, []); useEffect(() => { return () => { isRunningRef.current = false; if (rafRef.current !== null) { cancelAnimationFrame(rafRef.current); rafRef.current = null; } }; }, []); ``` Additional hardening measures: 1. Treat `isRunningRef` as the immediate source of truth for asynchronous RAF callbacks rather than waiting for React effects to synchronize it. 2. Consider using a generation token. Increment it on every start and stop, capture the current generation in the scheduled chain, and refuse to reschedule callbacks belonging to an obsolete generation. 3. If the design permits multiple active callbacks, store every RAF identifier in a collection and cancel all identifiers during stop and unmount. 4. Add automated tests covering: - Multiple synchronous calls to `start()`. - `start()` immediately followed by `stop()`. - Component unmount immediately after `start()`. - Unmount after repeated synchronous starts. - Confirmation that no frame callback executes or reschedules after cleanup. ]]>