fix: memory leak due to retain cycle between view & controller

[kkafar]

Description

In our Paper code we rely on React Native to invalidate our views* -- that always happens when changes in view hierarchy are triggered by JS side (standard React Native application). However in brownfield applications (iOS) there are some flows in which React Native cleanup (see RCTInvalidate protocol) does not get triggered (see #1754 for context).

* The cleanup is required because RNSScreenView retains RNSScreen by holding a reference. In invalidate method of RNSScreenView we just release the controller by setting _controller = nil.

Closes #1754

Changes

Added check in viewDidDisappear method for situation when whole navigation controller gets dismissed and if so, we manually release view controller.

Checklist

• Included code example that can be used to test this change
• Updated TS types
• Updated documentation:
  • https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
  • https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
  • https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
  • https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
• Ensured that CI passes

[efstathiosntonas]

@kkafar I've patched 3.22.1 with this on rn 0.72.1 on iOS simulator:

diff --git a/node_modules/react-native-screens/ios/RNSScreen.mm b/node_modules/react-native-screens/ios/RNSScreen.mm
index d6975f7..0765cac 100644
--- a/node_modules/react-native-screens/ios/RNSScreen.mm
+++ b/node_modules/react-native-screens/ios/RNSScreen.mm
@@ -571,6 +571,11 @@
 }
 #endif // !TARGET_OS_TV
 
+- (void)releaseViewController
+{
+  _controller = nil;
+}
+
 #pragma mark - Fabric specific
 #ifdef RCT_NEW_ARCH_ENABLED
 
@@ -769,7 +774,7 @@
 
 - (void)invalidate
 {
-  _controller = nil;
+  [self releaseViewController];
 }
 #endif
 
@@ -921,6 +926,18 @@ Class<RCTComponentViewProtocol> RNSScreenCls(void)
 #else
   [self traverseForScrollView:self.screenView];
 #endif
+
+  // See: https://github.com/software-mansion/react-native-screens/pull/1797
+  // In brownfield applications there are some flows in which React Native mechanisms
+  // of invalidating the views are not called, resulting in memory leak on our side,
+  // as RNSScreenView holds strong reference to a RNSScreen, thus they both can not be
+  // released.
+  UINavigationController *navigationController = self.navigationController;
+  BOOL isGoingForward =
+      [navigationController isBeingDismissed] || [navigationController isMovingFromParentViewController];
+  if (!isGoingForward || !_goingForward) {
+    [self.screenView releaseViewController];
+  }
 }
 
 - (void)viewDidLayoutSubviews

but when I navigation.goBack() it won't go back, it won't crash, nothing to see in logs, I cannot get out of it or tap anything else on screen.

video demo with xcode breakpoints

Screen.Recording.2023-07-11.at.10.51.00.mov

[efstathiosntonas]

typo appliactions -> applications

[kkafar]

Hey, I've moved fix of this issue to react native's repository: facebook/react-native#38617

[kkafar]

Hey @efstathiosntonas,
I've moved fix of this issue to react native's repository: facebook/react-native#38617 you can track the progress there.

Most likely I won't merge this PR, but I'm leaving it open in case I have to come with some workarounds in this library as the changes this PR introduces are not sufficient (and they do break other things)

[efstathiosntonas]

Thanks for the update @kkafar! keep it up!

[kkafar]

Hey, as my PR to React Native is merged I'm closing the PR.

facebook/react-native#38617

[konradgap]

@kkafar I see the PR made it to production, however, I don't see your changes in RNSScreen.mm on RN 0.75.2 (latest).

We are experiencing a memory leak potentially related to RNS in some of our apps, I wanted to root that out and test your fix.

I am on:

"react-native-screens": "^3.34.0"

unfortunately when I tested your PR branch I experienced a crash:

Thread 1: "*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil"

coming from RNSScreenContainer attachScreen.

Are you aware what happened to your changes? We just updated from RN 0.73.6 in hopes that memory management would improve.