Skip to content

Commit 71db286

Browse files
committed
fix(iOS,Fabric): prevent memory leak by calling invalidate on deleted screens (#2402)
On new architecture there is no callback that notifies components that they're deleted and won't be used anymore. `RCTComponentViewProtocol#prepareForRecycle` was supposed to fulfill this role, however after it became possible to disable view recycling for given class of components, it is not always called. In our case, we need such callback in `Screen` to break the retain (strong reference) cycle between `ScreenView` and its `Screen` (controller), otherwise we leak the `RNSScreenView` and `RNSScreen` instances. Overrode the `mountingTransactionWillMount:withSurfaceTelemetry:` in `RNSScreenStack`, where screens that are meant to receive `Delete` mutation are retained, and later in `mountingTransactionDidMount:withSurfaceTelemetry:` we dispatch a series of `invalidate` calls & release the components. <img width="557" alt="image" src="https://github.com/user-attachments/assets/546050e2-5eeb-4c2f-b0f9-5d1d4212889d"> <img width="539" alt="image" src="https://github.com/user-attachments/assets/e92a2778-b7c0-41e6-9ebb-68a8270aa786"> > [!note] > Please note, that these screenshots are done with a patch presented below 👇🏻, w/o it, the memory leak is not that big. Added `TestMemoryLeak` test screen to our example app. The easiest way to test this is to apply following patch: <details> <summary>See bigFatMemoryChunk</summary> ```c diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 6d06949..0e3a572b5 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -61,6 +61,7 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; @implementation RNSScreenView { __weak ReactScrollViewBase *_sheetsScrollView; BOOL _didSetSheetAllowedDetentsOnController; + NSMutableArray<UIView *> *bigFatMemoryChunk; #ifdef RCT_NEW_ARCH_ENABLED RCTSurfaceTouchHandler *_touchHandler; react::RNSScreenShadowNode::ConcreteState::Shared _state; @@ -89,7 +90,12 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; _props = defaultProps; _reactSubviews = [NSMutableArray new]; _contentWrapper = nil; + bigFatMemoryChunk = [[NSMutableArray alloc] init]; + for (int i = 0; i < 1024 * 5; ++i) { + [bigFatMemoryChunk addObject:[[UIView alloc] initWithFrame:frame]]; + } [self initCommonProps]; +// NSLog(@"[ALLOC][%ld] %p, memory chunk at %p, %@", self.tag, self, bigFatMemoryChunk, bigFatMemoryChunk[1023]); } return self; } @@ -753,6 +759,7 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; { _controller = nil; [_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil]; + [bigFatMemoryChunk removeAllObjects]; } #if !TARGET_OS_TV && !TARGET_OS_VISION ``` </details> Try to remove call to `[strongSelf invalidate]` in `mountingTransactionDidMount:withSurfaceTelemetry:` and see that the screens are indeed retained indefinitely. - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes (cherry picked from commit a116e7d)
1 parent 652dbcf commit 71db286

File tree

5 files changed

+124
-4
lines changed

5 files changed

+124
-4
lines changed

apps/src/tests/TestMemoryLeak.tsx

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import React from "react";
2+
import { NavigationContainer, RouteProp } from "@react-navigation/native";
3+
import { NativeStackNavigationProp, createNativeStackNavigator } from "@react-navigation/native-stack";
4+
import { Button, ColorValue, ScrollView, Text, View } from "react-native";
5+
6+
type HomeParams = {
7+
backgroundColor: ColorValue;
8+
forward: boolean;
9+
}
10+
11+
type StackParamList = {
12+
Home: HomeParams;
13+
SecondHome: HomeParams;
14+
ThirdHome: HomeParams;
15+
};
16+
17+
type BaseRouteProps = {
18+
navigation: NativeStackNavigationProp<StackParamList>;
19+
route: RouteProp<StackParamList>;
20+
}
21+
22+
const Stack = createNativeStackNavigator<StackParamList>();
23+
24+
function Home({ navigation, route }: BaseRouteProps): React.JSX.Element {
25+
const params = route.params;
26+
27+
React.useEffect(() => {
28+
setTimeout(() => {
29+
if (!params.forward) {
30+
navigation.popTo('Home', { backgroundColor: 'lightsalmon', forward: true });
31+
} else if (route.name === 'Home') {
32+
navigation.push('SecondHome', { backgroundColor: 'seagreen', forward: true });
33+
} else if (route.name === 'SecondHome') {
34+
navigation.push('ThirdHome', { backgroundColor: 'lightblue', forward: false });
35+
}
36+
}, 1000)
37+
})
38+
39+
return (
40+
<View style={{ flex: 1, backgroundColor: params.backgroundColor ?? 'red' }}>
41+
<ScrollView>
42+
{[...Array(100).keys()].map((index) => {
43+
return (
44+
<Text key={index.toString()}>{index}</Text>
45+
);
46+
})}
47+
</ScrollView>
48+
</View>
49+
);
50+
}
51+
52+
53+
function App(): React.JSX.Element {
54+
return (
55+
<NavigationContainer>
56+
<Stack.Navigator>
57+
<Stack.Screen name="Home" component={Home} initialParams={{ backgroundColor: 'lightsalmon', forward: true }} />
58+
<Stack.Screen name="SecondHome" component={Home} initialParams={{ backgroundColor: 'goldenrod', forward: true }} />
59+
<Stack.Screen name="ThirdHome" component={Home} initialParams={{ backgroundColor: 'lightblue', forward: false }} />
60+
</Stack.Navigator>
61+
</NavigationContainer>
62+
);
63+
}
64+
export default App;

apps/src/tests/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,3 +115,4 @@ export { default as Test2332 } from './Test2332';
115115
export { default as TestScreenAnimation } from './TestScreenAnimation';
116116
export { default as TestHeader } from './TestHeader';
117117
export { default as TestModalNavigation } from './TestModalNavigation';
118+
export { default as TestMemoryLeak } from './TestMemoryLeak';

ios/RNSScreen.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ namespace react = facebook::react;
131131
- (BOOL)isModal;
132132
- (BOOL)isPresentedAsNativeModal;
133133

134+
/**
135+
* Tell `Screen` component that it has been removed from react state and can safely cleanup
136+
* any retained resources.
137+
*
138+
* Note, that on old architecture this method might be called by RN via `RCTInvalidating` protocol.
139+
*/
140+
- (void)invalidate;
141+
134142
/// Looks for header configuration in instance's `reactSubviews` and returns it. If not present returns `nil`.
135143
- (RNSScreenStackHeaderConfig *_Nullable)findHeaderConfig;
136144

ios/RNSScreen.mm

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,12 @@ - (BOOL)isTransparentModal
614614
self.controller.modalPresentationStyle == UIModalPresentationOverCurrentContext;
615615
}
616616

617+
- (void)invalidate
618+
{
619+
_controller = nil;
620+
[_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil];
621+
}
622+
617623
#if !TARGET_OS_TV && !TARGET_OS_VISION
618624
/**
619625
* Updates settings for sheet presentation controller.
@@ -859,10 +865,6 @@ - (void)reactSetFrame:(CGRect)frame
859865
// subviews
860866
}
861867

862-
- (void)invalidate
863-
{
864-
_controller = nil;
865-
}
866868
#endif
867869

868870
@end

ios/RNSScreenStack.mm

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,13 @@ @implementation RNSScreenStackView {
120120
UIPercentDrivenInteractiveTransition *_interactionController;
121121
__weak RNSScreenStackManager *_manager;
122122
BOOL _updateScheduled;
123+
#ifdef RCT_NEW_ARCH_ENABLED
124+
/// Screens that are subject of `ShadowViewMutation::Type::Delete` mutation
125+
/// in current transaction. This vector should be populated when we receive notification via
126+
/// `RCTMountingObserving` protocol, that a transaction will be performed, and should
127+
/// be cleaned up when we're notified that the transaction has been completed.
128+
std::vector<__strong RNSScreenView *> _toBeDeletedScreens;
129+
#endif // RCT_NEW_ARCH_ENABLED
123130
}
124131

125132
#ifdef RCT_NEW_ARCH_ENABLED
@@ -1118,6 +1125,16 @@ - (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompone
11181125
// `- [RNSScreenStackView mountingTransactionDidMount: withSurfaceTelemetry:]`
11191126
}
11201127

1128+
- (nullable RNSScreenView *)childScreenForTag:(react::Tag)tag
1129+
{
1130+
for (RNSScreenView *child in _reactSubviews) {
1131+
if (child.tag == tag) {
1132+
return child;
1133+
}
1134+
}
1135+
return nil;
1136+
}
1137+
11211138
- (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
11221139
{
11231140
RNSScreenView *screenChildComponent = (RNSScreenView *)childComponentView;
@@ -1151,6 +1168,19 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo
11511168
[screenChildComponent removeFromSuperview];
11521169
}
11531170

1171+
- (void)mountingTransactionWillMount:(const facebook::react::MountingTransaction &)transaction
1172+
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
1173+
{
1174+
for (const auto &mutation : transaction.getMutations()) {
1175+
if (mutation.type == react::ShadowViewMutation::Delete) {
1176+
RNSScreenView *_Nullable toBeRemovedChild = [self childScreenForTag:mutation.oldChildShadowView.tag];
1177+
if (toBeRemovedChild != nil) {
1178+
_toBeDeletedScreens.push_back(toBeRemovedChild);
1179+
}
1180+
}
1181+
}
1182+
}
1183+
11541184
- (void)mountingTransactionDidMount:(const facebook::react::MountingTransaction &)transaction
11551185
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
11561186
{
@@ -1167,6 +1197,21 @@ - (void)mountingTransactionDidMount:(const facebook::react::MountingTransaction
11671197
break;
11681198
}
11691199
}
1200+
1201+
if (!self->_toBeDeletedScreens.empty()) {
1202+
__weak RNSScreenStackView *weakSelf = self;
1203+
// We want to run after container updates are performed (transitions etc.)
1204+
dispatch_async(dispatch_get_main_queue(), ^{
1205+
RNSScreenStackView *_Nullable strongSelf = weakSelf;
1206+
if (strongSelf == nil) {
1207+
return;
1208+
}
1209+
for (RNSScreenView *screenRef : strongSelf->_toBeDeletedScreens) {
1210+
[screenRef invalidate];
1211+
}
1212+
strongSelf->_toBeDeletedScreens.clear();
1213+
});
1214+
}
11701215
}
11711216

11721217
- (void)prepareForRecycle

0 commit comments

Comments
 (0)