From a116e7d2e558ccd6d3e3c674f71f0f03a342db68 Mon Sep 17 00:00:00 2001 From: Kacper Kafara Date: Mon, 14 Oct 2024 14:32:33 +0200 Subject: [PATCH] fix(iOS,Fabric): prevent memory leak by calling `invalidate` on deleted screens (#2402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 image ## After image > [!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:
See bigFatMemoryChunk ```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 *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 ```
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 --- apps/src/tests/TestMemoryLeak.tsx | 64 +++++++++++++++++++++++++++++++ apps/src/tests/index.ts | 1 + ios/RNSScreen.h | 8 ++++ ios/RNSScreen.mm | 11 +++--- ios/RNSScreenStack.mm | 45 ++++++++++++++++++++++ 5 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 apps/src/tests/TestMemoryLeak.tsx diff --git a/apps/src/tests/TestMemoryLeak.tsx b/apps/src/tests/TestMemoryLeak.tsx new file mode 100644 index 0000000000..507f75304e --- /dev/null +++ b/apps/src/tests/TestMemoryLeak.tsx @@ -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; + route: RouteProp; +} + +const Stack = createNativeStackNavigator(); + +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 ( + + + {[...Array(100).keys()].map((index) => { + return ( + {index} + ); + })} + + + ); +} + + +function App(): React.JSX.Element { + return ( + + + + + + + + ); +} +export default App; diff --git a/apps/src/tests/index.ts b/apps/src/tests/index.ts index 20c3a880ee..b35f4fb546 100644 --- a/apps/src/tests/index.ts +++ b/apps/src/tests/index.ts @@ -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'; diff --git a/ios/RNSScreen.h b/ios/RNSScreen.h index 28cbedbf7e..87a80d059d 100644 --- a/ios/RNSScreen.h +++ b/ios/RNSScreen.h @@ -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; diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 7f2fd3b8d9..6d069499f0 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -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 @@ -1265,11 +1271,6 @@ - (void)reactSetFrame:(CGRect)frame // subviews } -- (void)invalidate -{ - _controller = nil; - [_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil]; -} #endif @end diff --git a/ios/RNSScreenStack.mm b/ios/RNSScreenStack.mm index c2a983c0a0..dacdc5f355 100644 --- a/ios/RNSScreenStack.mm +++ b/ios/RNSScreenStack.mm @@ -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 @@ -1151,6 +1158,16 @@ - (void)mountChildComponentView:(UIView *)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 *)childComponentView index:(NSInteger)index { RNSScreenView *screenChildComponent = (RNSScreenView *)childComponentView; @@ -1184,6 +1201,19 @@ - (void)unmountChildComponentView:(UIView *)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 { @@ -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