From 6b3834a45b585e4340734139841ae81dc1b1a75d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Fri, 10 Nov 2023 15:49:07 +0000 Subject: [PATCH] Guard against unmounted components when accessing public instances on Fabric (#27687) ## Summary This fixes an error in `getPublicInstanceFromInstanceHandle` where we throw an error when trying to access the public instance from the fiber of an unmounted component. This shouldn't throw but return `null` instead. ## How did you test this change? Updated unit tests. Before: Screenshot 2023-11-10 at 15 26 14 After: Screenshot 2023-11-10 at 15 28 37 --- .../src/ReactFiberConfigFabric.js | 14 ++++++++++--- .../src/ReactNativeTypes.js | 2 +- .../__tests__/ReactFabric-test.internal.js | 21 +++++++++++++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFiberConfigFabric.js b/packages/react-native-renderer/src/ReactFiberConfigFabric.js index 285c1a0f107a0..784e00a9da5b7 100644 --- a/packages/react-native-renderer/src/ReactFiberConfigFabric.js +++ b/packages/react-native-renderer/src/ReactFiberConfigFabric.js @@ -278,13 +278,21 @@ function getPublicTextInstance( export function getPublicInstanceFromInternalInstanceHandle( internalInstanceHandle: InternalInstanceHandle, ): null | PublicInstance | PublicTextInstance { + const instance = internalInstanceHandle.stateNode; + + // React resets all the fields in the fiber when the component is unmounted + // to prevent memory leaks. + if (instance == null) { + return null; + } + if (internalInstanceHandle.tag === HostText) { - const textInstance: TextInstance = internalInstanceHandle.stateNode; + const textInstance: TextInstance = instance; return getPublicTextInstance(textInstance, internalInstanceHandle); } - const instance: Instance = internalInstanceHandle.stateNode; - return getPublicInstance(instance); + const elementInstance: Instance = internalInstanceHandle.stateNode; + return getPublicInstance(elementInstance); } export function prepareForCommit(containerInfo: Container): null | Object { diff --git a/packages/react-native-renderer/src/ReactNativeTypes.js b/packages/react-native-renderer/src/ReactNativeTypes.js index a6063cb2c10a0..f2ae65a82c380 100644 --- a/packages/react-native-renderer/src/ReactNativeTypes.js +++ b/packages/react-native-renderer/src/ReactNativeTypes.js @@ -245,7 +245,7 @@ export type ReactFabricType = { ): ?Node, getPublicInstanceFromInternalInstanceHandle( internalInstanceHandle: InternalInstanceHandle, - ): PublicInstance | PublicTextInstance, + ): PublicInstance | PublicTextInstance | null, ... }; diff --git a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js index e0c3e0e2257aa..a187595ad8a73 100644 --- a/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js @@ -1112,6 +1112,16 @@ describe('ReactFabric', () => { internalInstanceHandle, ); expect(publicInstance).toBe(viewRef); + + await act(() => { + ReactFabric.render(null, 1); + }); + + const publicInstanceAfterUnmount = + ReactFabric.getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle, + ); + expect(publicInstanceAfterUnmount).toBe(null); }); it('getPublicInstanceFromInternalInstanceHandle should provide public instances for HostText', async () => { @@ -1153,5 +1163,16 @@ describe('ReactFabric', () => { ReactNativePrivateInterface.createPublicTextInstance.mock.results[0] .value; expect(publicInstance).toBe(expectedPublicInstance); + + await act(() => { + ReactFabric.render(null, 1); + }); + + const publicInstanceAfterUnmount = + ReactFabric.getPublicInstanceFromInternalInstanceHandle( + internalInstanceHandle, + ); + + expect(publicInstanceAfterUnmount).toBe(null); }); });