diff --git a/example/src/reproducers/Issue407ListUnmount.tsx b/example/src/reproducers/Issue407ListUnmount.tsx new file mode 100644 index 00000000..c35f7d7f --- /dev/null +++ b/example/src/reproducers/Issue407ListUnmount.tsx @@ -0,0 +1,124 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; +import { View, Text, StyleSheet, Pressable } from 'react-native'; +import { + useRiveFile, + useRiveList, + useViewModelInstance, + type ViewModelInstance, +} from '@rive-app/react-native'; +import { type Metadata } from '../shared/metadata'; + +/** + * Issue #407: useRiveList's cleanup called a method on a list property that + * useDisposableMemo had already disposed. Debug builds throw (and the hook + * swallowed it); release builds segfault on the first unmount. + * + * Start mounts and unmounts a component using useRiveList every CYCLE_MS, for + * CYCLES rounds. Run it in a release build: expected is reaching + * "done" without the app crashing. + */ + +const CYCLES = 20; +const CYCLE_MS = 300; + +function TeamList({ instance }: { instance: ViewModelInstance }) { + const { length, error } = useRiveList('team', instance); + return ( + + team length: {error ? `error: ${error.message}` : String(length)} + + ); +} + +export default function Issue407ListUnmount() { + const { riveFile } = useRiveFile( + require('../../assets/rive/databinding_lists.riv') + ); + const { instance } = useViewModelInstance(riveFile, { + viewModelName: 'DevRel', + async: true, + }); + const [mounted, setMounted] = useState(false); + const [cycle, setCycle] = useState(0); + const [status, setStatus] = useState('idle'); + const timer = useRef | undefined>(undefined); + + const stop = useCallback(() => { + if (timer.current) clearInterval(timer.current); + timer.current = undefined; + }, []); + + const start = useCallback(() => { + stop(); + setCycle(0); + setStatus('running'); + let n = 0; + timer.current = setInterval(() => { + n += 1; + setMounted((m) => !m); + setCycle(Math.ceil(n / 2)); + if (n >= CYCLES * 2) { + stop(); + setStatus('done'); + } + }, CYCLE_MS); + }, [stop]); + + useEffect(() => { + if (instance) start(); + return stop; + }, [instance, start, stop]); + + return ( + + + {status} ยท cycle {cycle}/{CYCLES} + + + {mounted && instance ? : null} + + + + Start + + { + stop(); + setStatus('stopped'); + }} + > + Stop + + + + ); +} + +Issue407ListUnmount.metadata = { + name: 'Issue #407: useRiveList unmount', + description: + 'Mount/unmount a useRiveList consumer; crashed in release builds', +} satisfies Metadata; + +const styles = StyleSheet.create({ + container: { flex: 1, backgroundColor: '#fff', padding: 16 }, + status: { fontSize: 16, fontFamily: 'monospace', marginBottom: 12 }, + box: { + height: 60, + justifyContent: 'center', + borderWidth: 1, + borderColor: '#ddd', + borderRadius: 8, + paddingHorizontal: 12, + }, + list: { fontSize: 15 }, + buttons: { flexDirection: 'row', gap: 12, marginTop: 16 }, + button: { + backgroundColor: '#007AFF', + paddingHorizontal: 20, + paddingVertical: 10, + borderRadius: 8, + }, + buttonText: { color: '#fff', fontWeight: '600' }, +}); diff --git a/src/hooks/__tests__/useRiveList.test.ts b/src/hooks/__tests__/useRiveList.test.ts new file mode 100644 index 00000000..d6666731 --- /dev/null +++ b/src/hooks/__tests__/useRiveList.test.ts @@ -0,0 +1,121 @@ +import { renderHook, act } from '@testing-library/react-native'; +import { useRiveList } from '../useRiveList'; +import type { ViewModelInstance } from '../../specs/ViewModel.nitro'; + +// Calling a method on a disposed Nitro object crashes release builds (#407), +// so the mock records every such call. +function createMockListProperty() { + let disposed = false; + const callsAfterDispose: string[] = []; + const method = + (name: string, result: T) => + () => { + if (disposed) callsAfterDispose.push(name); + return result; + }; + return { + callsAfterDispose, + addListener: jest.fn(method('addListener', () => {})), + removeListeners: jest.fn(method('removeListeners', undefined)), + getLengthAsync: jest.fn(method('getLengthAsync', Promise.resolve(3))), + getInstanceAtAsync: jest.fn( + method('getInstanceAtAsync', Promise.resolve(undefined)) + ), + addInstanceAsync: jest.fn(method('addInstanceAsync', Promise.resolve())), + swapAsync: jest.fn(method('swapAsync', Promise.resolve())), + dispose: jest.fn(() => { + disposed = true; + }), + }; +} + +type MockListProperty = ReturnType; + +function createMockViewModelInstance(property: MockListProperty) { + return { + listProperty: jest.fn(() => property), + } as unknown as ViewModelInstance; +} + +describe('useRiveList', () => { + const globals = globalThis as { __DEV__?: boolean }; + let previousDev: boolean | undefined; + + beforeEach(() => { + // Release builds dispose synchronously on unmount, before effect cleanups. + previousDev = globals.__DEV__; + globals.__DEV__ = false; + }); + + afterEach(() => { + globals.__DEV__ = previousDev; + }); + + it('does not call into the list property after it is disposed on unmount', async () => { + const property = createMockListProperty(); + const instance = createMockViewModelInstance(property); + const { unmount } = renderHook(() => useRiveList('team', instance)); + await act(async () => {}); + expect(property.dispose).not.toHaveBeenCalled(); + unmount(); + + expect(property.dispose).toHaveBeenCalled(); + expect(property.callsAfterDispose).toEqual([]); + }); + + it('does not call into a list property disposed by a dependency change', async () => { + const first = createMockListProperty(); + const { rerender, unmount } = renderHook( + ({ instance }: { instance: ViewModelInstance }) => + useRiveList('team', instance), + { initialProps: { instance: createMockViewModelInstance(first) } } + ); + await act(async () => {}); + rerender({ + instance: createMockViewModelInstance(createMockListProperty()), + }); + await act(async () => {}); + + expect(first.dispose).toHaveBeenCalled(); + expect(first.callsAfterDispose).toEqual([]); + unmount(); + }); + + it('turns operations called after unmount into no-ops', async () => { + const property = createMockListProperty(); + const instance = createMockViewModelInstance(property); + const { result, unmount } = renderHook(() => useRiveList('team', instance)); + await act(async () => {}); + const { getInstanceAt, addInstance, swap } = result.current; + expect(property.dispose).not.toHaveBeenCalled(); + unmount(); + + await expect(getInstanceAt(0)).resolves.toBeUndefined(); + await addInstance({} as ViewModelInstance); + await swap(0, 1); + + expect(property.callsAfterDispose).toEqual([]); + }); + + it('changes operation identity when the list property changes', async () => { + const { result, rerender, unmount } = renderHook( + ({ instance }: { instance: ViewModelInstance }) => + useRiveList('team', instance), + { + initialProps: { + instance: createMockViewModelInstance(createMockListProperty()), + }, + } + ); + await act(async () => {}); + const { getInstanceAt, swap } = result.current; + rerender({ + instance: createMockViewModelInstance(createMockListProperty()), + }); + await act(async () => {}); + + expect(result.current.getInstanceAt).not.toBe(getInstanceAt); + expect(result.current.swap).not.toBe(swap); + unmount(); + }); +}); diff --git a/src/hooks/useRiveList.ts b/src/hooks/useRiveList.ts index 5acdae3d..e1f41e90 100644 --- a/src/hooks/useRiveList.ts +++ b/src/hooks/useRiveList.ts @@ -1,5 +1,8 @@ -import { useCallback, useEffect, useState } from 'react'; -import type { ViewModelInstance } from '../specs/ViewModel.nitro'; +import { useCallback, useEffect, useRef, useState } from 'react'; +import type { + ViewModelInstance, + ViewModelListProperty, +} from '../specs/ViewModel.nitro'; import type { UseRiveListResult } from '../types'; import { useDisposableMemo } from './useDisposableMemo'; @@ -25,13 +28,17 @@ export function useRiveList( setError(null); }, [path, viewModelInstance]); + // Cleared when the property is disposed; calling into a disposed list + // crashes release builds (#407), so the operations read it from here. + const liveRef = useRef(undefined); const property = useDisposableMemo( () => { if (!viewModelInstance) return undefined; return viewModelInstance.listProperty(path); }, (p) => p?.dispose(), - [viewModelInstance, path] + [viewModelInstance, path], + liveRef ); useEffect(() => { @@ -50,13 +57,9 @@ export function useRiveList( }); return () => { - try { - removeListener(); - property.removeListeners(); - } catch { - // Property may already be disposed by useDisposableMemo (deps change). - // Native dispose() handles listener cleanup, so this is safe to ignore. - } + // Only the unsubscribe function is safe here: the property may already + // be disposed. + removeListener(); }; }, [property]); @@ -96,57 +99,69 @@ export function useRiveList( }); }, []); + // The operations read liveRef rather than `property`, but keep `property` in + // deps so their identity changes with the list and consumers' effects re-run. const getInstanceAt = useCallback( (index: number) => { - return property - ? property.getInstanceAtAsync(index) - : Promise.resolve(undefined); + const list = liveRef.current; + return list ? list.getInstanceAtAsync(index) : Promise.resolve(undefined); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property] ); const addInstance = useCallback( (instance: ViewModelInstance) => { - return property - ? afterMutation(property.addInstanceAsync(instance)) + const list = liveRef.current; + return list + ? afterMutation(list.addInstanceAsync(instance)) : Promise.resolve(); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property, afterMutation] ); const addInstanceAt = useCallback( (instance: ViewModelInstance, index: number) => { - return property - ? afterMutation(property.addInstanceAtAsync(instance, index)) + const list = liveRef.current; + return list + ? afterMutation(list.addInstanceAtAsync(instance, index)) : Promise.resolve(); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property, afterMutation] ); const removeInstance = useCallback( (instance: ViewModelInstance) => { - return property - ? afterMutation(property.removeInstanceAsync(instance)) + const list = liveRef.current; + return list + ? afterMutation(list.removeInstanceAsync(instance)) : Promise.resolve(); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property, afterMutation] ); const removeInstanceAt = useCallback( (index: number) => { - return property - ? afterMutation(property.removeInstanceAtAsync(index)) + const list = liveRef.current; + return list + ? afterMutation(list.removeInstanceAtAsync(index)) : Promise.resolve(); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property, afterMutation] ); const swap = useCallback( (index1: number, index2: number) => { - return property - ? afterMutation(property.swapAsync(index1, index2)) + const list = liveRef.current; + return list + ? afterMutation(list.swapAsync(index1, index2)) : Promise.resolve(); }, + // eslint-disable-next-line react-hooks/exhaustive-deps [property, afterMutation] );