Skip to content

Commit

Permalink
Fabric: Fixing a deadlock in RCTSurfacePresenter
Browse files Browse the repository at this point in the history 
Summary:
This is another attempt to fix an issue very similar to T59424871. The previous attempt was in D19249490. I don't know why we don't see production crashes (stalls) but it happened to me (and not to me) in the debugger. The previous attempt didn't work because we still could have a deadlock because we tried to acquired shared mutex already owned exclusively by the `suspend` method.

Here is another approach: Instead of using one shared mutex, now we use two. One is similar to what we had and another that protects `suspend` and `resume`. Besides that, now we pass a Scheduler to functions that use that explicitly. This way we can be more explicit about acquiring mutexes and the overall flow of execution. The idea is: Now an arbitrary code that can be reentrant does not cover with mutexes, so the deadlock is not possible.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D20639228

fbshipit-source-id: 98515742f00f2ae94b50b585c9f1f0611e169ebe
  • Loading branch information
@shergin @facebook-github-bot
shergin authored and facebook-github-bot committed Mar 26, 2020
1 parent 89d04b5 commit 7344801
Showing 1 changed file with 91 additions and 51 deletions.
142 changes: 91 additions & 51 deletions React/Fabric/RCTSurfacePresenter.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,11 @@ @implementation RCTSurfacePresenter {
RCTMountingManager *_mountingManager; // Thread-safe.
RCTSurfaceRegistry *_surfaceRegistry; // Thread-safe.

better::shared_mutex _schedulerMutex;
RCTScheduler *_Nullable _scheduler; // Thread-safe. Pointer is protected by `_schedulerMutex`.
ContextContainer::Shared _contextContainer; // Protected by `_schedulerMutex`.
RuntimeExecutor _runtimeExecutor; // Protected by `_schedulerMutex`.
std::mutex _schedulerAccessMutex;
std::mutex _schedulerLifeCycleMutex;
RCTScheduler *_Nullable _scheduler; // Thread-safe. Pointer is protected by `_schedulerAccessMutex`.
ContextContainer::Shared _contextContainer; // Protected by `_schedulerLifeCycleMutex`.
RuntimeExecutor _runtimeExecutor; // Protected by `_schedulerLifeCycleMutex`.

better::shared_mutex _observerListMutex;
NSMutableArray<id<RCTSurfacePresenterObserver>> *_observers;
Expand All @@ -74,56 +75,63 @@ - (instancetype)initWithContextContainer:(ContextContainer::Shared)contextContai
return self;
}

- (RCTScheduler *_Nullable)_scheduler
{
std::lock_guard<std::mutex> lock(_schedulerAccessMutex);
return _scheduler;
}

- (ContextContainer::Shared)contextContainer
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);
return _contextContainer;
}

- (void)setContextContainer:(ContextContainer::Shared)contextContainer
{
std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);
_contextContainer = contextContainer;
}

- (RuntimeExecutor)runtimeExecutor
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);
return _runtimeExecutor;
}

- (void)setRuntimeExecutor:(RuntimeExecutor)runtimeExecutor
{
std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);
_runtimeExecutor = runtimeExecutor;
}

#pragma mark - Internal Surface-dedicated Interface

- (void)registerSurface:(RCTFabricSurface *)surface
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
RCTScheduler *scheduler = [self _scheduler];
[_surfaceRegistry registerSurface:surface];
if (_scheduler) {
[self _startSurface:surface];
if (scheduler) {
[self _startSurface:surface scheduler:scheduler];
}
}

- (void)unregisterSurface:(RCTFabricSurface *)surface
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
if (_scheduler) {
[self _stopSurface:surface];
RCTScheduler *scheduler = [self _scheduler];
if (scheduler) {
[self _stopSurface:surface scheduler:scheduler];
}
[_surfaceRegistry unregisterSurface:surface];
}

- (void)setProps:(NSDictionary *)props surface:(RCTFabricSurface *)surface
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
// This implementation is suboptimal indeed but still better than nothing for now.
[self _stopSurface:surface];
[self _startSurface:surface];
RCTScheduler *scheduler = [self _scheduler];
if (scheduler) {
[self _stopSurface:surface scheduler:scheduler];
[self _startSurface:surface scheduler:scheduler];
}
}

- (RCTFabricSurface *)surfaceForRootTag:(ReactTag)rootTag
Expand All @@ -135,39 +143,50 @@ - (CGSize)sizeThatFitsMinimumSize:(CGSize)minimumSize
maximumSize:(CGSize)maximumSize
surface:(RCTFabricSurface *)surface
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
RCTScheduler *scheduler = [self _scheduler];
if (!scheduler) {
return minimumSize;
}
LayoutContext layoutContext = {.pointScaleFactor = RCTScreenScale()};
LayoutConstraints layoutConstraints = {.minimumSize = RCTSizeFromCGSize(minimumSize),
.maximumSize = RCTSizeFromCGSize(maximumSize),
.layoutDirection = RCTLayoutDirection([[RCTI18nUtil sharedInstance] isRTL])};
return [_scheduler measureSurfaceWithLayoutConstraints:layoutConstraints
layoutContext:layoutContext
surfaceId:surface.rootTag];
return [scheduler measureSurfaceWithLayoutConstraints:layoutConstraints
layoutContext:layoutContext
surfaceId:surface.rootTag];
}

- (void)setMinimumSize:(CGSize)minimumSize maximumSize:(CGSize)maximumSize surface:(RCTFabricSurface *)surface
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
RCTScheduler *scheduler = [self _scheduler];
if (!scheduler) {
return;
}

LayoutContext layoutContext = {.pointScaleFactor = RCTScreenScale()};
LayoutConstraints layoutConstraints = {.minimumSize = RCTSizeFromCGSize(minimumSize),
.maximumSize = RCTSizeFromCGSize(maximumSize),
.layoutDirection = RCTLayoutDirection([[RCTI18nUtil sharedInstance] isRTL])};
[_scheduler constraintSurfaceLayoutWithLayoutConstraints:layoutConstraints
layoutContext:layoutContext
surfaceId:surface.rootTag];
[scheduler constraintSurfaceLayoutWithLayoutConstraints:layoutConstraints
layoutContext:layoutContext
surfaceId:surface.rootTag];
}

- (BOOL)synchronouslyUpdateViewOnUIThread:(NSNumber *)reactTag props:(NSDictionary *)props
{
std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
RCTScheduler *scheduler = [self _scheduler];
if (!scheduler) {
return NO;
}

ReactTag tag = [reactTag integerValue];
UIView<RCTComponentViewProtocol> *componentView =
[_mountingManager.componentViewRegistry findComponentViewWithTag:tag];
if (componentView == nil) {
return NO; // This view probably isn't managed by Fabric
}
ComponentHandle handle = [[componentView class] componentDescriptorProvider].handle;
auto *componentDescriptor = [_scheduler findComponentDescriptorByHandle_DO_NOT_USE_THIS_IS_BROKEN:handle];
auto *componentDescriptor = [scheduler findComponentDescriptorByHandle_DO_NOT_USE_THIS_IS_BROKEN:handle];

if (!componentDescriptor) {
return YES;
Expand All @@ -179,7 +198,12 @@ - (BOOL)synchronouslyUpdateViewOnUIThread:(NSNumber *)reactTag props:(NSDictiona

- (BOOL)synchronouslyWaitSurface:(RCTFabricSurface *)surface timeout:(NSTimeInterval)timeout
{
auto mountingCoordinator = [_scheduler mountingCoordinatorWithSurfaceId:surface.rootTag];
RCTScheduler *scheduler = [self _scheduler];
if (!scheduler) {
return NO;
}

auto mountingCoordinator = [scheduler mountingCoordinatorWithSurfaceId:surface.rootTag];

if (!mountingCoordinator->waitForTransaction(std::chrono::duration<NSTimeInterval>(timeout))) {
return NO;
Expand All @@ -192,28 +216,44 @@ - (BOOL)synchronouslyWaitSurface:(RCTFabricSurface *)surface timeout:(NSTimeInte

- (BOOL)suspend
{
std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);

if (!_scheduler) {
return NO;
RCTScheduler *scheduler;
{
std::lock_guard<std::mutex> accessLock(_schedulerAccessMutex);

if (!_scheduler) {
return NO;
}
scheduler = _scheduler;
_scheduler = nil;
}

[self _stopAllSurfaces];
_scheduler = nil;
[self _stopAllSurfacesWithScheduler:scheduler];

return YES;
}

- (BOOL)resume
{
std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
std::lock_guard<std::mutex> lock(_schedulerLifeCycleMutex);

if (_scheduler) {
return NO;
RCTScheduler *scheduler;
{
std::lock_guard<std::mutex> accessLock(_schedulerAccessMutex);

if (_scheduler) {
return NO;
}
scheduler = [self _createScheduler];
}

_scheduler = [self _createScheduler];
[self _startAllSurfaces];
[self _startAllSurfacesWithScheduler:scheduler];

{
std::lock_guard<std::mutex> accessLock(_schedulerAccessMutex);
_scheduler = scheduler;
}

return YES;
}
Expand Down Expand Up @@ -250,7 +290,7 @@ - (RCTScheduler *)_createScheduler
return scheduler;
}

- (void)_startSurface:(RCTFabricSurface *)surface
- (void)_startSurface:(RCTFabricSurface *)surface scheduler:(RCTScheduler *)scheduler
{
RCTMountingManager *mountingManager = _mountingManager;
RCTExecuteOnMainQueue(^{
Expand All @@ -264,16 +304,16 @@ - (void)_startSurface:(RCTFabricSurface *)surface
.maximumSize = RCTSizeFromCGSize(surface.maximumSize),
.layoutDirection = RCTLayoutDirection([[RCTI18nUtil sharedInstance] isRTL])};

[_scheduler startSurfaceWithSurfaceId:surface.rootTag
moduleName:surface.moduleName
initialProps:surface.properties
layoutConstraints:layoutConstraints
layoutContext:layoutContext];
[scheduler startSurfaceWithSurfaceId:surface.rootTag
moduleName:surface.moduleName
initialProps:surface.properties
layoutConstraints:layoutConstraints
layoutContext:layoutContext];
}

- (void)_stopSurface:(RCTFabricSurface *)surface
- (void)_stopSurface:(RCTFabricSurface *)surface scheduler:(RCTScheduler *)scheduler
{
[_scheduler stopSurfaceWithSurfaceId:surface.rootTag];
[scheduler stopSurfaceWithSurfaceId:surface.rootTag];

RCTMountingManager *mountingManager = _mountingManager;
RCTExecuteOnMainQueue(^{
Expand All @@ -287,20 +327,20 @@ - (void)_stopSurface:(RCTFabricSurface *)surface
[surface _unsetStage:(RCTSurfaceStagePrepared | RCTSurfaceStageMounted)];
}

- (void)_startAllSurfaces
- (void)_startAllSurfacesWithScheduler:(RCTScheduler *)scheduler
{
[_surfaceRegistry enumerateWithBlock:^(NSEnumerator<RCTFabricSurface *> *enumerator) {
for (RCTFabricSurface *surface in enumerator) {
[self _startSurface:surface];
[self _startSurface:surface scheduler:scheduler];
}
}];
}

- (void)_stopAllSurfaces
- (void)_stopAllSurfacesWithScheduler:(RCTScheduler *)scheduler
{
[_surfaceRegistry enumerateWithBlock:^(NSEnumerator<RCTFabricSurface *> *enumerator) {
for (RCTFabricSurface *surface in enumerator) {
[self _stopSurface:surface];
[self _stopSurface:surface scheduler:scheduler];
}
}];
}
Expand Down

0 comments on commit 7344801

Please sign in to comment.