-
-
Notifications
You must be signed in to change notification settings - Fork 518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(iOS,Fabric): prevent memory leak by calling invalidate
on deleted screens
#2402
Conversation
if (!self->_toBeDeletedScreens.empty()) { | ||
__weak RNSScreenStackView *weakSelf = self; | ||
// We want to run after container updates are performed (transitions etc.) | ||
dispatch_async(dispatch_get_main_queue(), ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you sure that it will run in the correct moment? Maybe it would be better to do it in some platform methods from UINavigationController
or in viewDidDisappear
? Not sure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewDidDisappear
is called e.g. for views that are no longer on top of the stack (e.g. new screen has been pushed in), so it does not serve our need.
Only method that would make sense here is didMoveToParentViewController:
called with nil
, however I'm not sure of its exact timing with respect to react methods & I believe (haven't tested it though), that it has the same drawback as viewDidDisappear
.
Only concern here is that the transitions started in maybeAddToParentAndUpdateContainer
will break in result of screenView._controller
being set to nil
during or before the transition. It seems that this could happen, if the transitions:
- were asynchronous,
- held only weak reference to the
RNSScreen
's used in the transition, - or referenced the
RNSScreen
's only viascreenView->_controller
.
I assumed that 3rd point is unlikely, because _controller
is our private field, and AFAIK (just checked in UIKit docs) UIView
does not expose any accessor to its owning view controller in UIKit API, thus system does not do that.
Our animation code uses only reversed reference: it obtains RNSScreenView
instance from RNSScreen
, hence we're safe on this front.
2nd point seems to be false, haven't found any information on this, saying only empirically.
1st point: 🤷🏻♂️ Not really sure how transitions work & haven't managed to find any information on threading etc.
Summary: This surely can be implemented in other ways, however did not see a better approach at this moment, while the suggested one seems to solve the issue w/o introducing regressions (at least my testing suggests so).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewDidDisappear is called e.g. for views that are no longer on top of the stack (e.g. new screen has been pushed in), so it does not serve our need.
I think we have some heuristics already for when it is the top screen that is dismissed but yeah, if it works then we can stick with that.
…ed screens (#2402) On new architecture there is no callback that notifies components that they're deleted and won't be used anymore. `RCTComponentViewProtocol#prepareForRecycle` was supposed to fulfill this role, however after it became possible to disable view recycling for given class of components, it is not always called. In our case, we need such callback in `Screen` to break the retain (strong reference) cycle between `ScreenView` and its `Screen` (controller), otherwise we leak the `RNSScreenView` and `RNSScreen` instances. Overrode the `mountingTransactionWillMount:withSurfaceTelemetry:` in `RNSScreenStack`, where screens that are meant to receive `Delete` mutation are retained, and later in `mountingTransactionDidMount:withSurfaceTelemetry:` we dispatch a series of `invalidate` calls & release the components. <img width="557" alt="image" src="https://github.com/user-attachments/assets/546050e2-5eeb-4c2f-b0f9-5d1d4212889d"> <img width="539" alt="image" src="https://github.com/user-attachments/assets/e92a2778-b7c0-41e6-9ebb-68a8270aa786"> > [!note] > Please note, that these screenshots are done with a patch presented below 👇🏻, w/o it, the memory leak is not that big. Added `TestMemoryLeak` test screen to our example app. The easiest way to test this is to apply following patch: <details> <summary>See bigFatMemoryChunk</summary> ```c diff --git a/ios/RNSScreen.mm b/ios/RNSScreen.mm index 6d069499f..0e3a572b5 100644 --- a/ios/RNSScreen.mm +++ b/ios/RNSScreen.mm @@ -61,6 +61,7 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; @implementation RNSScreenView { __weak ReactScrollViewBase *_sheetsScrollView; BOOL _didSetSheetAllowedDetentsOnController; + NSMutableArray<UIView *> *bigFatMemoryChunk; #ifdef RCT_NEW_ARCH_ENABLED RCTSurfaceTouchHandler *_touchHandler; react::RNSScreenShadowNode::ConcreteState::Shared _state; @@ -89,7 +90,12 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; _props = defaultProps; _reactSubviews = [NSMutableArray new]; _contentWrapper = nil; + bigFatMemoryChunk = [[NSMutableArray alloc] init]; + for (int i = 0; i < 1024 * 5; ++i) { + [bigFatMemoryChunk addObject:[[UIView alloc] initWithFrame:frame]]; + } [self initCommonProps]; +// NSLog(@"[ALLOC][%ld] %p, memory chunk at %p, %@", self.tag, self, bigFatMemoryChunk, bigFatMemoryChunk[1023]); } return self; } @@ -753,6 +759,7 @@ constexpr NSInteger SHEET_LARGEST_UNDIMMED_DETENT_NONE = -1; { _controller = nil; [_sheetsScrollView removeObserver:self forKeyPath:@"bounds" context:nil]; + [bigFatMemoryChunk removeAllObjects]; } #if !TARGET_OS_TV && !TARGET_OS_VISION ``` </details> Try to remove call to `[strongSelf invalidate]` in `mountingTransactionDidMount:withSurfaceTelemetry:` and see that the screens are indeed retained indefinitely. - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes (cherry picked from commit a116e7d)
Description
On new architecture there is no callback that notifies components that they're deleted
and won't be used anymore.
RCTComponentViewProtocol#prepareForRecycle
was supposed to fulfillthis role, however after it became possible to disable view recycling for given class of components,
it is not always called.
In our case, we need such callback in
Screen
to break the retain (strong reference) cycle betweenScreenView
and itsScreen
(controller),otherwise we leak the
RNSScreenView
andRNSScreen
instances.Changes
Overrode the
mountingTransactionWillMount:withSurfaceTelemetry:
inRNSScreenStack
, where screens that are meant to receiveDelete
mutation are retained, and later inmountingTransactionDidMount:withSurfaceTelemetry:
we dispatch a series ofinvalidate
calls & release the components.Before
After
Note
Please note, that these screenshots are done with a patch presented below 👇🏻, w/o it, the memory leak is not that big.
Test code and steps to reproduce
Added
TestMemoryLeak
test screen to our example app. The easiest way to test this is to apply following patch:See bigFatMemoryChunk
Try to remove call to
[strongSelf invalidate]
inmountingTransactionDidMount:withSurfaceTelemetry:
and see that the screens are indeed retained indefinitely.Checklist