Skip to content

Commit

Permalink
fix(iOS,Fabric): prevent memory leak by calling invalidate on delet…
Browse files Browse the repository at this point in the history
…ed screens (#2402)

## Description

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.

## Changes

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.

## Before

<img width="557" alt="image"
src="https://github.com/user-attachments/assets/546050e2-5eeb-4c2f-b0f9-5d1d4212889d">


## After

<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.

## Test code and steps to reproduce

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 6d069499f..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.

## Checklist

- [x] Included code example that can be used to test this change
- [ ] Ensured that CI passes
  • Loading branch information
kkafar authored Oct 14, 2024
1 parent c1ca095 commit a116e7d
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 5 deletions.
64 changes: 64 additions & 0 deletions apps/src/tests/TestMemoryLeak.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React from "react";
import { NavigationContainer, RouteProp } from "@react-navigation/native";
import { NativeStackNavigationProp, createNativeStackNavigator } from "@react-navigation/native-stack";
import { Button, ColorValue, ScrollView, Text, View } from "react-native";

type HomeParams = {
backgroundColor: ColorValue;
forward: boolean;
}

type StackParamList = {
Home: HomeParams;
SecondHome: HomeParams;
ThirdHome: HomeParams;
};

type BaseRouteProps = {
navigation: NativeStackNavigationProp<StackParamList>;
route: RouteProp<StackParamList>;
}

const Stack = createNativeStackNavigator<StackParamList>();

function Home({ navigation, route }: BaseRouteProps): React.JSX.Element {
const params = route.params;

React.useEffect(() => {
setTimeout(() => {
if (!params.forward) {
navigation.popTo('Home', { backgroundColor: 'lightsalmon', forward: true });
} else if (route.name === 'Home') {
navigation.push('SecondHome', { backgroundColor: 'seagreen', forward: true });
} else if (route.name === 'SecondHome') {
navigation.push('ThirdHome', { backgroundColor: 'lightblue', forward: false });
}
}, 1000)
})

return (
<View style={{ flex: 1, backgroundColor: params.backgroundColor ?? 'red' }}>
<ScrollView>
{[...Array(100).keys()].map((index) => {
return (
<Text key={index.toString()}>{index}</Text>
);
})}
</ScrollView>
</View>
);
}


function App(): React.JSX.Element {
return (
<NavigationContainer>
<Stack.Navigator>
<Stack.Screen name="Home" component={Home} initialParams={{ backgroundColor: 'lightsalmon', forward: true }} />
<Stack.Screen name="SecondHome" component={Home} initialParams={{ backgroundColor: 'goldenrod', forward: true }} />
<Stack.Screen name="ThirdHome" component={Home} initialParams={{ backgroundColor: 'lightblue', forward: false }} />
</Stack.Navigator>
</NavigationContainer>
);
}
export default App;
1 change: 1 addition & 0 deletions apps/src/tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,4 @@ export { default as TestPreload } from './TestPreload';
export { default as TestActivityStateProgression } from './TestActivityStateProgression';
export { default as TestHeaderTitle } from './TestHeaderTitle';
export { default as TestModalNavigation } from './TestModalNavigation';
export { default as TestMemoryLeak } from './TestMemoryLeak';
8 changes: 8 additions & 0 deletions ios/RNSScreen.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,14 @@ namespace react = facebook::react;
- (BOOL)isModal;
- (BOOL)isPresentedAsNativeModal;

/**
* Tell `Screen` component that it has been removed from react state and can safely cleanup
* any retained resources.
*
* Note, that on old architecture this method might be called by RN via `RCTInvalidating` protocol.
*/
- (void)invalidate;

/// Looks for header configuration in instance's `reactSubviews` and returns it. If not present returns `nil`.
- (RNSScreenStackHeaderConfig *_Nullable)findHeaderConfig;

Expand Down
11 changes: 6 additions & 5 deletions ios/RNSScreen.mm
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,12 @@ - (BOOL)isTransparentModal
self.controller.modalPresentationStyle == UIModalPresentationOverCurrentContext;
}

- (void)invalidate
{
_controller = nil;
[_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil];
}

#if !TARGET_OS_TV && !TARGET_OS_VISION

- (void)setPropertyForSheet:(UISheetPresentationController *)sheet
Expand Down Expand Up @@ -1265,11 +1271,6 @@ - (void)reactSetFrame:(CGRect)frame
// subviews
}

- (void)invalidate
{
_controller = nil;
[_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil];
}
#endif

@end
Expand Down
45 changes: 45 additions & 0 deletions ios/RNSScreenStack.mm
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,13 @@ @implementation RNSScreenStackView {
UIPercentDrivenInteractiveTransition *_interactionController;
__weak RNSScreenStackManager *_manager;
BOOL _updateScheduled;
#ifdef RCT_NEW_ARCH_ENABLED
/// Screens that are subject of `ShadowViewMutation::Type::Delete` mutation
/// in current transaction. This vector should be populated when we receive notification via
/// `RCTMountingObserving` protocol, that a transaction will be performed, and should
/// be cleaned up when we're notified that the transaction has been completed.
std::vector<__strong RNSScreenView *> _toBeDeletedScreens;
#endif // RCT_NEW_ARCH_ENABLED
}

#ifdef RCT_NEW_ARCH_ENABLED
Expand Down Expand Up @@ -1151,6 +1158,16 @@ - (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompone
// `- [RNSScreenStackView mountingTransactionDidMount: withSurfaceTelemetry:]`
}

- (nullable RNSScreenView *)childScreenForTag:(react::Tag)tag
{
for (RNSScreenView *child in _reactSubviews) {
if (child.tag == tag) {
return child;
}
}
return nil;
}

- (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
{
RNSScreenView *screenChildComponent = (RNSScreenView *)childComponentView;
Expand Down Expand Up @@ -1184,6 +1201,19 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo
[screenChildComponent removeFromSuperview];
}

- (void)mountingTransactionWillMount:(const facebook::react::MountingTransaction &)transaction
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
{
for (const auto &mutation : transaction.getMutations()) {
if (mutation.type == react::ShadowViewMutation::Delete) {
RNSScreenView *_Nullable toBeRemovedChild = [self childScreenForTag:mutation.oldChildShadowView.tag];
if (toBeRemovedChild != nil) {
_toBeDeletedScreens.push_back(toBeRemovedChild);
}
}
}
}

- (void)mountingTransactionDidMount:(const facebook::react::MountingTransaction &)transaction
withSurfaceTelemetry:(const facebook::react::SurfaceTelemetry &)surfaceTelemetry
{
Expand All @@ -1200,6 +1230,21 @@ - (void)mountingTransactionDidMount:(const facebook::react::MountingTransaction
break;
}
}

if (!self->_toBeDeletedScreens.empty()) {
__weak RNSScreenStackView *weakSelf = self;
// We want to run after container updates are performed (transitions etc.)
dispatch_async(dispatch_get_main_queue(), ^{
RNSScreenStackView *_Nullable strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}
for (RNSScreenView *screenRef : strongSelf->_toBeDeletedScreens) {
[screenRef invalidate];
}
strongSelf->_toBeDeletedScreens.clear();
});
}
}

- (void)prepareForRecycle
Expand Down

0 comments on commit a116e7d

Please sign in to comment.