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: <img width="969" alt="Screenshot 2023-11-10 at 15 26 14" src="https://github.com/facebook/react/assets/117921/ea161616-2775-4fab-8d74-da4bef48d09a"> After: <img width="1148" alt="Screenshot 2023-11-10 at 15 28 37" src="https://github.com/facebook/react/assets/117921/db18b918-b6b6-4925-9cfc-3b4b2f3ab92d">
Rubén Norte committed
Nov 10, 2023 at 15:49 UTC
6b3834a45b585e4340734139841ae81dc1b1a75d
3 files changed
+33
-4
packages/react-native-renderer/src/ReactFiberConfigFabric.js
+11
-3
@@ -278,13 +278,21 @@ function getPublicTextInstance(
278
export function getPublicInstanceFromInternalInstanceHandle(
279
internalInstanceHandle: InternalInstanceHandle,
280
): null | PublicInstance | PublicTextInstance {
281
+ const instance = internalInstanceHandle.stateNode;
282
+
283
+ // React resets all the fields in the fiber when the component is unmounted
284
+ // to prevent memory leaks.
285
+ if (instance == null) {
286
+ return null;
287
+ }
288
+
289
if (internalInstanceHandle.tag === HostText) {
282
- const textInstance: TextInstance = internalInstanceHandle.stateNode;
290
+ const textInstance: TextInstance = instance;
291
return getPublicTextInstance(textInstance, internalInstanceHandle);
292
}
293
286
- const instance: Instance = internalInstanceHandle.stateNode;
287
- return getPublicInstance(instance);
294
+ const elementInstance: Instance = internalInstanceHandle.stateNode;
295
+ return getPublicInstance(elementInstance);
296
}
297
298
export function prepareForCommit(containerInfo: Container): null | Object {
packages/react-native-renderer/src/ReactNativeTypes.js
+1
-1
@@ -245,7 +245,7 @@ export type ReactFabricType = {
245
): ?Node,
246
getPublicInstanceFromInternalInstanceHandle(
247
internalInstanceHandle: InternalInstanceHandle,
248
- ): PublicInstance | PublicTextInstance,
248
+ ): PublicInstance | PublicTextInstance | null,
249
...
250
};
251
packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
+21
@@ -1112,6 +1112,16 @@ describe('ReactFabric', () => {
1112
internalInstanceHandle,
1113
);
1114
expect(publicInstance).toBe(viewRef);
1115
+
1116
+ await act(() => {
1117
+ ReactFabric.render(null, 1);
1118
+ });
1119
+
1120
+ const publicInstanceAfterUnmount =
1121
+ ReactFabric.getPublicInstanceFromInternalInstanceHandle(
1122
+ internalInstanceHandle,
1123
+ );
1124
+ expect(publicInstanceAfterUnmount).toBe(null);
1125
});
1126
1127
it('getPublicInstanceFromInternalInstanceHandle should provide public instances for HostText', async () => {
@@ -1153,5 +1163,16 @@ describe('ReactFabric', () => {
1163
ReactNativePrivateInterface.createPublicTextInstance.mock.results[0]
1164
.value;
1165
expect(publicInstance).toBe(expectedPublicInstance);
1166
+
1167
+ await act(() => {
1168
+ ReactFabric.render(null, 1);
1169
+ });
1170
+
1171
+ const publicInstanceAfterUnmount =
1172
+ ReactFabric.getPublicInstanceFromInternalInstanceHandle(
1173
+ internalInstanceHandle,
1174
+ );
1175
+
1176
+ expect(publicInstanceAfterUnmount).toBe(null);
1177
});
1178
});