Skip to content

Commit

Permalink
Fix: Make RCTSurfacePresenter weakly retain its observers (microsoft#…
Browse files Browse the repository at this point in the history
…1493)

Summary:
Changelog: [Fabric][iOS] Fix: Make RCTSurfacePresenter weakly retain its observers

There is retain cycle because RCTSurfacePresenter is keeping an array of  RCTSurfacePresenterObserver, which is strongly retaining the class that owns this RCTSurfacePresenter.

This diff makes RCTSurfacePresenter weakly retain observers instead.

Reviewed By: RSNara

Differential Revision: D35439589

fbshipit-source-id: ddc7813976b543de12af6173b2f1b31c69b043a8

# Conflicts:
#	React/Fabric/RCTSurfacePresenter.mm

Co-authored-by: Paige Sun <paigesun@fb.com>
  • Loading branch information
2 people authored and Shawn Dempsey committed Mar 10, 2023
1 parent 39d50f9 commit 8aa9f91
Showing 1 changed file with 33 additions and 10 deletions.
43 changes: 33 additions & 10 deletions React/Fabric/RCTSurfacePresenter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ @implementation RCTSurfacePresenter {
RuntimeExecutor _runtimeExecutor; // Protected by `_schedulerLifeCycleMutex`.

butter::shared_mutex _observerListMutex;
NSMutableArray<id<RCTSurfacePresenterObserver>> *_observers;
RCTImageLoader *_imageLoader;
std::vector<__weak id<RCTSurfacePresenterObserver>> _observers; // Protected by `_observerListMutex`.
}

- (instancetype)initWithContextContainer:(ContextContainer::Shared)contextContainer
Expand All @@ -97,8 +96,6 @@ - (instancetype)initWithContextContainer:(ContextContainer::Shared)contextContai
_mountingManager.contextContainer = contextContainer;
_mountingManager.delegate = self;

_observers = [NSMutableArray array];

_scheduler = [self _createScheduler];
}

Expand Down Expand Up @@ -364,13 +361,17 @@ - (void)schedulerDidSetIsJSResponder:(BOOL)isJSResponder
- (void)addObserver:(id<RCTSurfacePresenterObserver>)observer
{
std::unique_lock<butter::shared_mutex> lock(_observerListMutex);
[self->_observers addObject:observer];
_observers.push_back(observer);
}

- (void)removeObserver:(id<RCTSurfacePresenterObserver>)observer
{
std::unique_lock<butter::shared_mutex> lock(_observerListMutex);
[self->_observers removeObject:observer];
std::vector<__weak id<RCTSurfacePresenterObserver>>::const_iterator it =
std::find(_observers.begin(), _observers.end(), observer);
if (it != _observers.end()) {
_observers.erase(it);
}
}

#pragma mark - RCTMountingManagerDelegate
Expand All @@ -379,8 +380,13 @@ - (void)mountingManager:(RCTMountingManager *)mountingManager willMountComponent
{
RCTAssertMainQueue();

std::shared_lock<butter::shared_mutex> lock(_observerListMutex);
for (id<RCTSurfacePresenterObserver> observer in _observers) {
NSArray<id<RCTSurfacePresenterObserver>> *observersCopy;
{
std::shared_lock<butter::shared_mutex> lock(_observerListMutex);
observersCopy = [self _getObservers];
}

for (id<RCTSurfacePresenterObserver> observer in observersCopy) {
if ([observer respondsToSelector:@selector(willMountComponentsWithRootTag:)]) {
[observer willMountComponentsWithRootTag:rootTag];
}
Expand All @@ -391,12 +397,29 @@ - (void)mountingManager:(RCTMountingManager *)mountingManager didMountComponents
{
RCTAssertMainQueue();

std::shared_lock<butter::shared_mutex> lock(_observerListMutex);
for (id<RCTSurfacePresenterObserver> observer in _observers) {
NSArray<id<RCTSurfacePresenterObserver>> *observersCopy;
{
std::shared_lock<butter::shared_mutex> lock(_observerListMutex);
observersCopy = [self _getObservers];
}

for (id<RCTSurfacePresenterObserver> observer in observersCopy) {
if ([observer respondsToSelector:@selector(didMountComponentsWithRootTag:)]) {
[observer didMountComponentsWithRootTag:rootTag];
}
}
}

- (NSArray<id<RCTSurfacePresenterObserver>> *)_getObservers
{
NSMutableArray<id<RCTSurfacePresenterObserver>> *observersCopy = [NSMutableArray new];
for (id<RCTSurfacePresenterObserver> observer : _observers) {
if (observer) {
[observersCopy addObject:observer];
}
}

return observersCopy;
}

@end

0 comments on commit 8aa9f91

Please sign in to comment.