software-mansion / react-native-screens

Native navigation primitives for your React Native app.
https://docs.swmansion.com/react-native-screens/
MIT License
3.1k stars 521 forks source link

Memory leak while moving from one screen to another in the same stack #843

Closed abhaynpai closed 3 years ago

abhaynpai commented 3 years ago

Description

One can observe retained memory while moving across different screens in the application. Even if you have one single stack with 3 different pages, you can observer a memory leak there. I have created a sample project to reproduce this issue. The link is given below

https://github.com/abhaynpai/rn-screens-leak

Screenshots

This is a YouTube video showcasing the leak in the project.

<img src="https://img.youtube.com/vi/_xtXRnA-vEE/0.jpg" alt="IMAGE ALT TEXT HERE" width="240" height="180" />

Steps To Reproduce

The steps are given in this GitHub link - https://github.com/abhaynpai/rn-screens-leak

Expected behavior

No memory leak should be displayed while moving across page.

Actual behavior

While moving between pages you can observe multiple memory leaks.

Snack or minimal code example

// App.js

import React from 'react';
import {NavigationContainer} from '@react-navigation/native';
import {StatusBar} from 'react-native';

import {enableScreens} from 'react-native-screens';

import TestStack from './src/TestStack';

enableScreens();

const App: () => React$Node = () => {
  return (
    <NavigationContainer>
      <StatusBar barStyle="dark-content" />
      {TestStack()}
    </NavigationContainer>
  );
};

export default App;

// ________________________

// TestStack.js

import React from 'react';
import {createNativeStackNavigator} from 'react-native-screens/native-stack';

import TestPage1 from './TestPage1';
import TestPage2 from './TestPage2';
import TestPage3 from './TestPage3';

const Stack = createNativeStackNavigator();

const TestStack = () => {
  return (
    <Stack.Navigator>
      <Stack.Screen name="TestPage1" component={TestPage1} />
      <Stack.Screen name="TestPage2" component={TestPage2} />
      <Stack.Screen name="TestPage3" component={TestPage3} />
    </Stack.Navigator>
  );
};

export default TestStack;

Package versions

jp928 commented 3 years ago

@abhaynpai this is very scary. plz fix it.

jp928 commented 3 years ago

Just notice you didn't do super.onCreate(null) in MainApplication.java. Not sure if this is the reason.

abhaynpai commented 3 years ago

Hey @jp928

Even after adding super.onCreate(null) in MainApplication.java you can observe the memory leak.

I am unsure on how to fix it. Do you have any idea on the potential root cause for this memory leak?

omaryoussef commented 3 years ago

I feel like it might be related to this issue perhaps: https://github.com/react-navigation/react-navigation/issues/7078

abhaynpai commented 3 years ago

@omaryoussef - I think the it is somewhat related but not exactly. This issue highlights the actual issue with native navigation + screens.

If you disable enableScreens and run the exact same test then you will not be able to see the memory leak.

Thunderbird7 commented 3 years ago

It's the same for me! on my situation, I was using too many scenes in the same stack then get a memory leak issue. Anyway, I did try to disable enableScreen() and try it again. The memory leak issue is gone but I get the very slow animation...

I think it's have a problem who can be to fixing or explain?

abhaynpai commented 3 years ago

I know how we can recreate the issue but I am unsure on what exactly is causing this memory leak.

Alaa-Ben commented 3 years ago

Same issue, not sure if it's a good idea to upgrade to 3.0.0 with this still happening (as the latest release enables screens by default)

TNBoiBoi commented 3 years ago

I'm having this bug too. It's a severe problem please check it.

WoLewicki commented 3 years ago

The behavior described by the tools as a leak is the consequence of keeping the ScreenFragments in the memory. It is done like this because, in react-native, we cannot destroy and then make new views by restoring the state of the Fragment, since each view has its reactTag etc. The behavior is shown as a leak due to heuristics of the leak detector tools, which say that if onDestroy was called on a Fragment, then the reference to it should not be kept anywhere, but, as mentioned above, it is not applicable to react-native apps, since we do not recreate the views of the Fragment, but rather call remove on the them when they become invisible and then add them back on the Screen becoming visible with the same Screen attached to it.

I hope this resolves the issue, so I will close it since I don't think we can do much more about it. If you have any questions or can propose other solutions, please write here and I can reopen it.

grahammendick commented 3 years ago

I've profiled the Navigation router and it doesn’t show any memory leaks. The Navigation router uses Android's native back stack to manage the scenes. Android doesn't call onDestroy on a Fragment in the back stack until it's popped and the pop animation has completed.

WoLewicki commented 3 years ago

@grahammendick using back stack can be a way to go, it is even implemented in the RNScreens, but there were problems with integrating them with React Navigation iirc. Does this approach have benefits other than just not calling this lifecycle method on the Fragment, which we are not using at the moment?

grahammendick commented 3 years ago

@WoLewicki A couple of benefits that spring to mind

  1. Native custom animations - when you add a fragment to the back stack you specify the 4 different animations (enter, exit, popEnter and popExit). Android takes care of running them depending on how the scene is moving through or off the stack. The medley Navigation router example has 4 scenes called north, south, east and west and Android animates them from and to these compass points as they’re popped and pushed. The animations are specified in React and point at a java resource file
  2. Native shared elements - the fragment back stack supports shared element animations. Android plays these animations when the scene is pushed and plays them in reverse when the scene is popped. The zoom Navigation router example shows a list of colors and a color details scene. Android animates the selected color as the details scene is pushed and popped. Notice that you can change the color on the details scene and Android will correctly run the shared element animation on the new color when its popped. Again the animations are fully customisable in React but must point at a java resource file
zoyopo commented 3 years ago

The behavior described by the tools as a leak is the consequence of keeping the ScreenFragments in the memory. It is done like this because, in react-native, we cannot destroy and then make new views by restoring the state of the Fragment, since each view has its reactTag etc. The behavior is shown as a leak due to heuristics of the leak detector tools, which say that if onDestroy was called on a Fragment, then the reference to it should not be kept anywhere, but, as mentioned above, it is not applicable to react-native apps, since we do not recreate the views of the Fragment, but rather call remove on the them when they become invisible and then add them back on the Screen becoming visible with the same Screen attached to it.

I hope this resolves the issue, so I will close it since I don't think we can do much more about it. If you have any questions or can propose other solutions, please write here and I can reopen it.

alt code I found that Screen instance keep the reference of ScreenFragment and also ScreenFragment instance keep the reference of Screen in java source code.Could it cause memory leak?

WoLewicki commented 3 years ago

@zoyopo the references on both sides are nulled when the Screen is removed from React view hierarchy, you can see it when going back, the memory is freed then.

Neha0595 commented 1 year ago

I made two changes -

  1. I have added the line enableScreen(false). This change was made to address the issue of memory leaks caused by the default value enableScreen being set to true. This issue arises because we are using navigation in JavaScript while utilizing native screens, which can result in memory leaks during tab changes and transitions.
  2. I added super.onCreate(null) in MainApplication/MainActivity where onCreate method is created. It reduces memory leaks. But the super.onCreate(savedInstanceState) method call is used to call the superclass's implementation of onCreate and pass it the saved state information. It is essential for the proper initialisation of your activity and its lifecycle management. By passing null instead of a valid Bundle (which is used to save and restore the state of your activity), you might cause unintended issues in your application.