Skip to content

Commit 01375a5

Browse files
authored
fix(Android, Stack v4): Clear supportActionBar on fragment removal to prevent ScreenStackFragment leak (#3867)
## Description Detected retention chain: - AppCompatDelegateImpl.mActionBar - ToolbarActionBar.mDecorToolbar - ToolbarWidgetWrapper.mToolbar - DebugMenuToolbar.config - ScreenStackHeaderConfig.mParent - Screen.fragment `ScreenStackHeaderConfig.onUpdate` calls `activity.setSupportActionBar(toolbar)` on top screen updates. `AppCompatDelegateImpl` stores the resulting `ToolbarActionBar` in `mActionBar` for the lifetime of the activity. When a screen is popped (and the new top screen does not install a replacement ActionBar) the stale `ToolbarActionBar` is never released. In `onDestroyView` of `ScreenStackFragment`, call reset SupportActionBar to release it and the fragment. I'm storing a reference to `actionBar` that ensures the call is skipped when the incoming screen has already installed its own toolbar in `onUpdate`. Closes: software-mansion/react-native-screens-labs#1129 ## Changes - added a reference to the actionBar in ScreenStackHeaderConfig to clear actionBar only if it's associated with that fragment and wasn't overridden by other screen - added onDestroyView override in ScreenStackFragment that will clear the ActionBar - changed `setToolbar` signature to pass a CustomToolbar, which allows for storing the header config - HeaderConfig is nulled by React transaction before FragmentManager sends onDestroy to ScreenStackFragment ## Before & after - visual documentation | Before | After | | --- | --- | | <video src="https://github.com/user-attachments/assets/6d095d46-b868-4036-b8ed-120e65114711" /> | <video src="https://github.com/user-attachments/assets/09a29685-5b81-433d-b408-61fcd54944a0" /> | ## Test plan Test3867 ## Checklist - [ ] Included code example that can be used to test this change. - [ ] For visual changes, included screenshots / GIFs / recordings documenting the change. - [ ] For API changes, updated relevant public types. - [ ] Ensured that CI passes
1 parent 9bb2d74 commit 01375a5

6 files changed

Lines changed: 142 additions & 10 deletions

File tree

android/src/main/java/com/swmansion/rnscreens/ScreenModalFragment.kt

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import android.view.View
1010
import android.view.ViewGroup
1111
import android.view.ViewParent
1212
import android.view.WindowManager
13-
import androidx.appcompat.widget.Toolbar
1413
import androidx.fragment.app.Fragment
1514
import com.facebook.react.bridge.ReactContext
1615
import com.facebook.react.uimanager.UIManagerHelper
@@ -187,7 +186,7 @@ class ScreenModalFragment :
187186

188187
override fun removeToolbar(): Unit = throw IllegalStateException("[RNScreens] Modal screens on Android do not support header right now")
189188

190-
override fun setToolbar(toolbar: Toolbar): Unit =
189+
override fun setToolbar(toolbar: CustomToolbar): Unit =
191190
throw IllegalStateException("[RNScreens] Modal screens on Android do not support header right now")
192191

193192
override fun setToolbarShadowHidden(hidden: Boolean): Unit =

android/src/main/java/com/swmansion/rnscreens/ScreenStackFragment.kt

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import android.view.View
1414
import android.view.ViewGroup
1515
import android.view.animation.Animation
1616
import android.widget.LinearLayout
17+
import androidx.appcompat.app.AppCompatActivity
1718
import androidx.appcompat.widget.Toolbar
1819
import androidx.coordinatorlayout.widget.CoordinatorLayout
1920
import androidx.core.view.ViewCompat
@@ -57,6 +58,8 @@ class ScreenStackFragment :
5758
private var isToolbarShadowHidden = false
5859
private var isToolbarTranslucent = false
5960

61+
private var lastActiveHeaderConfig: ScreenStackHeaderConfig? = null
62+
6063
private lateinit var sheetTransitionCoordinator: BottomSheetTransitionCoordinator
6164

6265
private var lastFocusedChild: View? = null
@@ -103,7 +106,8 @@ class ScreenStackFragment :
103106
toolbar = null
104107
}
105108

106-
override fun setToolbar(toolbar: Toolbar) {
109+
override fun setToolbar(toolbar: CustomToolbar) {
110+
lastActiveHeaderConfig = toolbar.config
107111
appBarLayout?.addView(toolbar)
108112
toolbar.layoutParams =
109113
AppBarLayout
@@ -301,6 +305,25 @@ class ScreenStackFragment :
301305
super.onViewCreated(view, savedInstanceState)
302306
}
303307

308+
override fun onDestroyView() {
309+
// ScreenStackHeaderConfig.onUpdate() calls activity.setSupportActionBar(toolbar) each time
310+
// the top screen updates. AppCompatDelegateImpl stores the resulting ToolbarActionBar in
311+
// its mActionBar field for the lifetime of the activity. When a screen is popped and the new
312+
// top screen does not install a replacement action bar (e.g. headerShown: false),
313+
// the stale ToolbarActionBar — and the entire object graph hanging off the toolbar is never released.
314+
// When this fragment is being removed, we're clearing the activity's support action bar if it
315+
// still belongs to us. This will break the retention chain:
316+
// - AppCompatDelegateImpl.mActionBar
317+
// - ToolbarActionBar.mDecorToolbar
318+
// - ToolbarWidgetWrapper.mToolbar
319+
// - DebugMenuToolbar.config
320+
// - ScreenStackHeaderConfig.mParent
321+
// - Screen.fragment
322+
lastActiveHeaderConfig?.clearActionBarIfOwned(activity as? AppCompatActivity)
323+
lastActiveHeaderConfig = null
324+
super.onDestroyView()
325+
}
326+
304327
override fun onCreateAnimation(
305328
transit: Int,
306329
enter: Boolean,

android/src/main/java/com/swmansion/rnscreens/ScreenStackFragmentWrapper.kt

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
package com.swmansion.rnscreens
22

3-
import androidx.appcompat.widget.Toolbar
4-
53
interface ScreenStackFragmentWrapper : ScreenFragmentWrapper {
64
// Toolbar management
75
fun removeToolbar()
86

9-
fun setToolbar(toolbar: Toolbar)
7+
fun setToolbar(toolbar: CustomToolbar)
108

119
fun setToolbarShadowHidden(hidden: Boolean)
1210

android/src/main/java/com/swmansion/rnscreens/ScreenStackHeaderConfig.kt

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import android.view.Gravity
99
import android.view.View.OnClickListener
1010
import android.widget.ImageView
1111
import android.widget.TextView
12+
import androidx.appcompat.app.ActionBar
1213
import androidx.appcompat.app.AppCompatActivity
1314
import androidx.appcompat.widget.Toolbar
1415
import androidx.fragment.app.Fragment
@@ -58,6 +59,7 @@ class ScreenStackHeaderConfig(
5859
private var isBackButtonHidden = false
5960
private var isShadowHidden = false
6061
private var isDestroyed = false
62+
private var actionBar: ActionBar? = null
6163
private var backButtonInCustomView = false
6264
private var tintColor = 0
6365
private var isAttachedToWindow = false
@@ -113,6 +115,17 @@ class ScreenStackHeaderConfig(
113115
isDestroyed = true
114116
}
115117

118+
internal fun clearActionBarIfOwned(activity: AppCompatActivity?) {
119+
actionBar?.let { ownedActionBar ->
120+
activity?.let { appCompat ->
121+
if (appCompat.supportActionBar === ownedActionBar) {
122+
appCompat.setSupportActionBar(null)
123+
}
124+
}
125+
}
126+
actionBar = null
127+
}
128+
116129
/**
117130
* Native toolbar should notify the header config component that it has completed its layout.
118131
*/
@@ -243,15 +256,16 @@ class ScreenStackHeaderConfig(
243256

244257
activity.setSupportActionBar(toolbar)
245258
// non-null toolbar is set in the line above and it is used here
246-
val actionBar = requireNotNull(activity.supportActionBar)
259+
val newActionBar = requireNotNull(activity.supportActionBar)
260+
actionBar = newActionBar
247261

248262
// hide back button
249-
actionBar.setDisplayHomeAsUpEnabled(
263+
newActionBar.setDisplayHomeAsUpEnabled(
250264
screenFragment?.canNavigateBack() == true && !isBackButtonHidden,
251265
)
252266

253267
// title
254-
actionBar.title = title
268+
newActionBar.title = title
255269
if (TextUtils.isEmpty(title)) {
256270
isTitleEmpty = true
257271
}
@@ -325,7 +339,7 @@ class ScreenStackHeaderConfig(
325339
?: throw JSApplicationIllegalArgumentException(
326340
"Back button header config view should have Image as first child",
327341
)
328-
actionBar.setHomeAsUpIndicator(firstChild.drawable)
342+
newActionBar.setHomeAsUpIndicator(firstChild.drawable)
329343
i++
330344
continue
331345
}
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
import React from 'react';
2+
import { View, Text, Button, StyleSheet } from 'react-native';
3+
import { NavigationContainer, ParamListBase } from '@react-navigation/native';
4+
import {
5+
createNativeStackNavigator,
6+
NativeStackNavigationProp,
7+
} from '@react-navigation/native-stack';
8+
9+
type RouteParamList = {
10+
Home: undefined;
11+
Details: undefined;
12+
Settings: undefined;
13+
};
14+
15+
type NavigationProp<ParamList extends ParamListBase> = {
16+
navigation: NativeStackNavigationProp<ParamList>;
17+
};
18+
19+
type StackNavigationProp = NavigationProp<RouteParamList>;
20+
21+
const Stack = createNativeStackNavigator<RouteParamList>();
22+
23+
function HomeScreen({ navigation }: StackNavigationProp) {
24+
return (
25+
<View style={styles.container}>
26+
<Text style={styles.title}>Home Screen</Text>
27+
<Button
28+
title="Push Details"
29+
onPress={() => navigation.navigate('Details')}
30+
/>
31+
</View>
32+
);
33+
}
34+
35+
function DetailScreen({ navigation }: StackNavigationProp) {
36+
return (
37+
<View style={styles.container}>
38+
<Text style={styles.title}>Details Screen</Text>
39+
<Button
40+
title="Push Settings"
41+
onPress={() => navigation.navigate('Settings')}
42+
/>
43+
<View style={styles.spacer} />
44+
<Button
45+
title="Pop (Go Back)"
46+
onPress={() => navigation.goBack()}
47+
color="red"
48+
/>
49+
</View>
50+
);
51+
}
52+
53+
function DeepDetailScreen({ navigation }: StackNavigationProp) {
54+
return (
55+
<View style={styles.container}>
56+
<Text style={styles.title}>Settings Screen</Text>
57+
<View style={styles.spacer} />
58+
<Button
59+
title="Pop to Top"
60+
onPress={() => navigation.popToTop()}
61+
color="red"
62+
/>
63+
</View>
64+
);
65+
}
66+
67+
export default function App() {
68+
return (
69+
<NavigationContainer>
70+
<Stack.Navigator>
71+
<Stack.Screen
72+
name="Home"
73+
component={HomeScreen}
74+
options={{ headerShown: false }}
75+
/>
76+
<Stack.Screen name="Details" component={DetailScreen} />
77+
<Stack.Screen name="Settings" component={DeepDetailScreen} />
78+
</Stack.Navigator>
79+
</NavigationContainer>
80+
);
81+
}
82+
83+
const styles = StyleSheet.create({
84+
container: {
85+
flex: 1,
86+
alignItems: 'center',
87+
justifyContent: 'center',
88+
},
89+
title: {
90+
fontSize: 20,
91+
fontWeight: 'bold',
92+
marginBottom: 10,
93+
},
94+
spacer: {
95+
height: 20,
96+
},
97+
});

apps/src/tests/issue-tests/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ export { default as Test3793 } from './Test3793';
188188
export { default as Test3816 } from './Test3816';
189189
export { default as Test3833 } from './Test3833';
190190
export { default as Test3835 } from './Test3835';
191+
export { default as Test3867 } from './Test3867';
191192
export { default as TestScreenAnimation } from './TestScreenAnimation';
192193
// The following test was meant to demo the "go back" gesture using Reanimated
193194
// but the associated PR in react-navigation is currently put on hold

0 commit comments

Comments
 (0)