iOS 15 - function deinit not called on custom native components on goback

[delphinebugner]

Description

Thanks for working on this project! This bug is reaaally weird and it could be a memory leak on iOS 15. I only patched a workaround on my project, I do want your advice on it!

Bug

TL;DR: on iOS 15, after we upgraded RNScreens from 3.5 to 3.25, our custom bridged player continued to play after the destruction of its screen.

Here is a demo (turn the sound on!):

Bug_player-not-deinit_small.mov

Not iOS 14 neither iOS 16 ou 17. We still have around ~15% of our users on iOS 15 so it was important to fix.

Investigation

Our custom bridged player has its own deinit function in Swift, equivalent to dealloc in objective C++:

// Bridged component (old archi)
@objc(PlayerReactNativeViewManager)
class PlayerReactNativeViewManager: RCTViewManager {
  override func view() -> (UIView) {
    return PlayerReactNativeView()
  }
}

class PlayerReactNativeView : UIView, PlayerDelegate, PlayerUiEventDelegate {
  deinit { // ⬅️ here, lots of clean-up function responsible to turn the sound off of the player, for example
    player?.reset()
    playerController = nil
    player = nil
    // (...)
  }

(...)
}

On iOS 15, since version 3.19 of RNScreens, we saw that we did not enter the deinit function of the player when its screen was destroyed (= after a go-back in our native stack). Therefore its sound continued to play.

We do enter the deinit function when we destroy the player alone, keeping the screen around it.

After bisecting the version, we saw that this PR introduces the regression: #1649.

More precisely, it's this line of RNSScreen.mm that introduces the leak :

react-native-screens/ios/RNSScreen.mm

Line 618 in e0bbf77

UISheetPresentationController *sheet = _controller.sheetPresentationController;

Just creating the *sheet pointer is enough to introduce the bug, even without the sheet being actually used after. I'm not a native expert but i think we can see it in this memory graph on XCode that the "UISnoopController" (=> its the one managing our player sound) is still attached to a _PageSheetPresentationController (our *sheet!); it could explain why we still hear the video sound:

Fix

It's a workaround: because we do not use the PresentationFormSheet in my app, I simply moved the line where we create the *sheet pointer inside of the if:

diff --git a/node_modules/react-native-screens/ios/RNSScreen.mm b/node_modules/react-native-screens/ios/RNSScreen.mm
index 4b24cff..7e0718d 100644
--- a/node_modules/react-native-screens/ios/RNSScreen.mm
+++ b/node_modules/react-native-screens/ios/RNSScreen.mm
@@ -609,8 +609,9 @@ - (void)updatePresentationStyle
 #if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && defined(__IPHONE_15_0) && \
     __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_15_0
   if (@available(iOS 15.0, *)) {
-    UISheetPresentationController *sheet = _controller.sheetPresentationController;
-    if (_stackPresentation == RNSScreenStackPresentationFormSheet && sheet != nil) {
+    if (_stackPresentation == RNSScreenStackPresentationFormSheet) {
+      UISheetPresentationController *sheet = _controller.sheetPresentationController;
+      if (sheet != nil) {
       sheet.prefersScrollingExpandsWhenScrolledToEdge = _sheetExpandsWhenScrolledToEdge;
       sheet.prefersGrabberVisible = _sheetGrabberVisible;
       sheet.preferredCornerRadius =
@@ -646,6 +647,7 @@ - (void)updatePresentationStyle
       } else {
         RCTLogError(@"Unhandled value of sheetAllowedDetents passed");
       }
+      }
     }
   }
 #endif // Check for max allowed iOS version

Here is an example of a go-back on iOS 15 with video sound properly disappearing:

Fixed_player-deinit.mov

Next steps

• Do you think there is a leak here, and the *sheet should be dealloc anyway? It's the main reason why I'm opening this issue, if you find something useful here!
• And I do not know why is it only on iOS 15 - I get why it's not iOS 14 obviously, but what is different in the iOS 16 memory?
• Is it related to this issue ? Memory Leak. iOS. Strong reference cycle between RNSScreen and RNSScreenView #1754
• Other accepted answer is, "you simply do not use a custom deinit on your native components" and I can hear that too 😅

Steps to reproduce

The bug is currently in production on my app : https://apps.apple.com/fr/app/tf1-info-lci-actualit%C3%A9s/id426125722

Download it on an iPhone 15 and enjoy multiple sound superposing when doing go-backs!! 🥳

More seriously, I do not have a small reproduction yet, and can't share the full code as it belongs to a private company ; but if you judge it necessary I can try to work on it! (the URL provided s a sandbox but without repro in it)

Snack or a link to a repository

https://github.com/delphinebugner/expo-bac-a-sable

Screens version

3.29.0

React Native version

0.73.2

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native (without Expo)

Architecture

Paper (Old Architecture)

Build type

None

Device

iOS simulator

Device model

iOS 15 only (neither 14 nor 16) ; real device & simulator

Acknowledgements

Yes

[kkafar]

Hey @delphinebugner,

Is it related to this issue ? #1754

What is your setup? Do you use RN only in part of your application, or is it standard RN app?

Honestly I'm baffled, as the strong reference should be dropped once it goes out of the scope (after the function end), this is an automatic variable 😅

Thanks for reporting, we will investigate it.

[delphinebugner]

It is a standard RN app, created in July 2021!

We don't use expo (yet) ; in custom native, we just have the player mentioned above, along 2 other in-house native modules.
But that's it!

And we use the Native stack navigator of RNNavigation

[Romick2005]

Honestly I'm baffled, as the strong reference should be dropped once it goes out of the scope (after the function end), this is an automatic variable 😅

The sheet variable is not cleared after function end as it is used inside animateChanges callback as this variable should be accessible here from parent scope.

[tboba]

Hi @delphinebugner! I'm trying to reproduce this issue and I've stumbled upon some questions, while trying to do my own repro:

• Could you tell us more about the hierarchy of your screens? I'm especially concerned about its presentation - is the screen with the video player a formsheet?
• Do you know if this issue is reproducible with other video players, like Expo AV?

Also, even if I'm trying to do something with this PR, could you try to create a minimal repro with the two screens that show such behaviour? Of course it's not necessary to put your own video controller there, if it's possible to reproduce that leak in other components 😁
I'm trying to reproduce the behaviour where screens are calling for updatePresentationStyle, but even with the formsheet I can't make that happen 😕

[delphinebugner]

Thanks @tboba for working on this!

• No I don't use a formsheet (this is why my patch is "working", I don't enter the "if") ; it's a Native Stack, with orientation : portrait_up and a custom header, and it's the only cutom parameters used :

const RootStack = createNativeStackNavigator<RootStackParamList>();

 <RootStack.Navigator screenOptions={{ header: CustomRootStackHeader,  orientation: isTablet() ? 'all' : 'portrait_up'}}>
   <RootStack.Screen
      name={Routes.BASEPAGE} 
      component={BasePage} // 🐞  Unmounting this screen, video player inside is not deinit
      getId={({ params }) => params.pageLink}
   />
...

type RootStackParamList = {
  [Routes.BASEPAGE]: { pageLink: string };
  ...
};

• I don't know about other players - but I guess they have a different native implementation, and don't require a custom deinit like mine, so maybe not having the problem
• I will definitely try to set up a minimal reproduction to help, I'll let you know!

[tboba]

@delphinebugner great, thanks! Yeah, it's quite weird that didSetProps is being called when there's a custom deinit 🤔 Have you checked if the same problem exists on Fabric? I see this method is being also called in finalizeUpdates, but maybe this is not the case there?

[delphinebugner]

Okay I finally did it, the repro is here: https://github.com/delphinebugner/react-native-repro-deinit-15-old-arch

Follow the instruction in the readme to see it happen!

It's in old arch only, I will check now how it works with new arch! It's longer to reproduce as there is no Swift template for Fabric component, but it may be the same with the ObjectiveC dealloc function, I will try it!

[tboba]

@delphinebugner Great, thanks for that! I really also appreciate the explanation in README! 🙏
This has helped us to investigate that issue further and I believe we've (me and @kkafar) found a better solution for the leakage.

I'll let you know if we would have any other questions.