Skip to content

Commit

Permalink
Guard against unmounted components when accessing public instances on…
Browse files Browse the repository at this point in the history
… 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">
  • Loading branch information
rubennorte authored Nov 10, 2023
1 parent 6a7f3aa commit 6b3834a
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 4 deletions.
14 changes: 11 additions & 3 deletions packages/react-native-renderer/src/ReactFiberConfigFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native-renderer/src/ReactNativeTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ export type ReactFabricType = {
): ?Node,
getPublicInstanceFromInternalInstanceHandle(
internalInstanceHandle: InternalInstanceHandle,
): PublicInstance | PublicTextInstance,
): PublicInstance | PublicTextInstance | null,
...
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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);
});
});

0 comments on commit 6b3834a

Please sign in to comment.