Skip to content

Commit 6b28f61

Browse files
rubennorteAndyPengc12
authored andcommitted
Guard against unmounted components when accessing public instances on Fabric (facebook#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">
1 parent 0bc3948 commit 6b28f61

File tree

3 files changed

+33
-4
lines changed

3 files changed

+33
-4
lines changed

packages/react-native-renderer/src/ReactFiberConfigFabric.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -278,13 +278,21 @@ function getPublicTextInstance(
278278
export function getPublicInstanceFromInternalInstanceHandle(
279279
internalInstanceHandle: InternalInstanceHandle,
280280
): 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+
281289
if (internalInstanceHandle.tag === HostText) {
282-
const textInstance: TextInstance = internalInstanceHandle.stateNode;
290+
const textInstance: TextInstance = instance;
283291
return getPublicTextInstance(textInstance, internalInstanceHandle);
284292
}
285293

286-
const instance: Instance = internalInstanceHandle.stateNode;
287-
return getPublicInstance(instance);
294+
const elementInstance: Instance = internalInstanceHandle.stateNode;
295+
return getPublicInstance(elementInstance);
288296
}
289297

290298
export function prepareForCommit(containerInfo: Container): null | Object {

packages/react-native-renderer/src/ReactNativeTypes.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ export type ReactFabricType = {
245245
): ?Node,
246246
getPublicInstanceFromInternalInstanceHandle(
247247
internalInstanceHandle: InternalInstanceHandle,
248-
): PublicInstance | PublicTextInstance,
248+
): PublicInstance | PublicTextInstance | null,
249249
...
250250
};
251251

packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1112,6 +1112,16 @@ describe('ReactFabric', () => {
11121112
internalInstanceHandle,
11131113
);
11141114
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);
11151125
});
11161126

11171127
it('getPublicInstanceFromInternalInstanceHandle should provide public instances for HostText', async () => {
@@ -1153,5 +1163,16 @@ describe('ReactFabric', () => {
11531163
ReactNativePrivateInterface.createPublicTextInstance.mock.results[0]
11541164
.value;
11551165
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);
11561177
});
11571178
});

0 commit comments

Comments
 (0)