Commit 030cb44
authored
fix(iOS, Stack v4): prevent header subview memory leak (#3653)
## Description
Prevents header subview memory leak.
Closes
software-mansion/react-native-screens-labs#696.
### Details
When header items were introduced, potential memory leak was spotted:
#2987 (comment).
From my testing, it seems that after introducing a wrapper view on iOS
26+
(#3449),
UIKit manages to deallocate all necessary views. On iOS 18 however I
managed to create a memory leak (see video "before").
There is one thing that confuses me a little bit - it seems like on iOS
26, UIKit retains the reference to bar button item & wrapper view a
little bit longer than expected (e.g. via
`_UINavigationBarContentView`). When we push a screen with bar button
item, pop it, it seems like the reference remains but when we push a new
screen, it is deallocated.
Also, when first screen uses `headerShown: false`, the behavior seems
similar also on iOS 18 but this time with header subview reference. It
is also deallocated after pushing another screen.
I don't think that we need to worry about this for now but I wanted to
point that out in PR description.
### Solution
I decided to keep the reference to `UIBarButtonItem` inside
`RNSScreenStackHeaderSubview` to maintain prop update logic
(#2987 (comment)).
I don't think that we can make it a weak reference because in header
config we clear left/rightBarButtonItems from navigation item - I guess
that the button might be deallocated before we attempt to re-use it
(correct me if I'm wrong).
Because of the above, I needed a place to clear the reference. I thought
about willMoveToWindow/Superview callbacks but:
1. `willMoveToWindow` is called multiple times so it's not reliable
2. `willMoveToSuperview` is better but when we push another screen over
a screen with bar button item, we will move to nil. I think that we
don't want to clear the bar button then to maintain any (internal) state
inside the button.
At first, I wanted to use already existing logic in header config's
`unmountChildComponentView` but this callback's Paper counterpart isn't
called when entire screen is popped instead of only subview. That's why
I used `invalidate` callback. On Paper, it's available via
`RCTInvalidating`. On Fabric, this is available on RCTViewComponentView
starting from RN 0.82. As next release will require RN 0.82 for Fabric,
we can comfortably use this callback.
## Changes
- add method to clear bar button reference
- use it in `invalidate` callback
## Before & after - visual documentation
### Before
https://github.com/user-attachments/assets/fb07fd55-8462-4fec-9791-85733c78e606
### After
https://github.com/user-attachments/assets/42ab5c6c-4a97-4db1-a098-60d08d39adc8
## Test plan
Run `Test3422`. Remove `headerShown: false` from `Screen1`. Push a
screen and then pop it. Check memory graph for `HeaderSubview`. You can
also override `dealloc` for header subview.
## Checklist
- [x] Included code example that can be used to test this change.
- [ ] Ensured that CI passes1 parent bdef20a commit 030cb44
File tree
2 files changed
+22
-4
lines changed- ios
2 files changed
+22
-4
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
17 | | - | |
| 17 | + | |
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
52 | 55 | | |
53 | 56 | | |
54 | 57 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
30 | | - | |
31 | | - | |
| 30 | + | |
| 31 | + | |
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
| |||
223 | 223 | | |
224 | 224 | | |
225 | 225 | | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
226 | 232 | | |
227 | 233 | | |
228 | 234 | | |
| |||
278 | 284 | | |
279 | 285 | | |
280 | 286 | | |
281 | | - | |
| 287 | + | |
282 | 288 | | |
283 | 289 | | |
284 | 290 | | |
| |||
295 | 301 | | |
296 | 302 | | |
297 | 303 | | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
298 | 313 | | |
299 | 314 | | |
300 | 315 | | |
| |||
0 commit comments