Skip to content

Commit

Permalink
Fabric: Fixed a crash caused by calling [RCTFabricSurface start] of…
Browse files Browse the repository at this point in the history
…f the main thread

Summary:
We need to register a root view component here synchronously because right after we start a surface, it can initiate an update that can query the root component.

This fixes an issue in Fabric Surface caused by a data race between registration of root component and concurrent call to `constraintLayout` on the main thread.
This how it happens:
1. We call `startSurface` on some background thread;
2. As a result `[RCTFabricSurface start]` schedules a lambda to the main thread that will register a root component.
3. Meanwhile some other code on the main thread changes the layout and requests the relayout of a surface.
4. Because the surface is already running, the `constraintLayout` method works as expected and schedules a mutation instruction that changes the size of the root component.
5. The mutation instruction is being delivered synchronously on the main thread and fails because the root component was not yet registered. Boom.

For now, it's not fully clear how to solve this problem properly. I suspect that the good solution would be to shift the responsibility of registering a root component to the MountingCoordinator side (so, we will have an explicit instruction prescribing creating and registering the root component). This will probably be a quite invasive change though.

For now, I think it's fine to make the block synchronous to avoid the race.

Reviewed By: mdvacca

Differential Revision: D26802418

fbshipit-source-id: d49484c90d1ac61ac595caf486562fc6f4843e2f
  • Loading branch information
shergin authored and facebook-github-bot committed Mar 4, 2021
1 parent c7d28bc commit 913c958
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions React/Fabric/Surface/RCTFabricSurface.mm
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,16 @@ - (BOOL)start
return NO;
}

_surfaceHandler->start();
[self _propagateStageChange];

RCTExecuteOnMainQueue(^{
// We need to register a root view component here synchronously because right after
// we start a surface, it can initiate an update that can query the root component.
RCTUnsafeExecuteOnMainQueueSync(^{
[self->_surfacePresenter.mountingManager attachSurfaceToView:self.view
surfaceId:self->_surfaceHandler->getSurfaceId()];
});

_surfaceHandler->start();
[self _propagateStageChange];

[_surfacePresenter setupAnimationDriverWithSurfaceHandler:*_surfaceHandler];
return YES;
}
Expand Down

0 comments on commit 913c958

Please sign in to comment.