From 9c1412882edbd309a4bed0eec760513738c1cba6 Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Mon, 30 Dec 2019 11:13:07 -0800
Subject: [PATCH] Fabric: Using shared mutex to protect scheduler object in
 RCTSurfacePresenter

Summary:
This fixes a deadlock caused by recursive calling of `-[RCTSurfacePresenter setMinimumSize:minimumSize:]` (because of an attempt to lock the mutex which was already locked upper the call stack).
Seems there is no reason why this operation should not be allowed. This diff replaces a regular mutex with a shared one which allows multiple shared locks and this prevents the deadlock. A Scheduler is already a thread-safe object and the mutex only protected the mutation of a pointer to it.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D19249490

fbshipit-source-id: 00e8934c6502328f34b78ea6ec004b7216665db1
---
 React/Fabric/RCTSurfacePresenter.mm | 45 +++++++++++++++--------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm
index 4186843809..adbb20892f 100644
--- a/React/Fabric/RCTSurfacePresenter.mm
+++ b/React/Fabric/RCTSurfacePresenter.mm
@@ -40,13 +40,14 @@ using namespace facebook::react;
 @end
 
 @implementation RCTSurfacePresenter {
-  std::mutex _schedulerMutex;
-  RCTScheduler
-      *_Nullable _scheduler; // Thread-safe. Mutation of the instance variable is protected by `_schedulerMutex`.
-  ContextContainer::Shared _contextContainer;
-  RuntimeExecutor _runtimeExecutor;
   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`.
+
   better::shared_mutex _observerListMutex;
   NSMutableArray<id<RCTSurfacePresenterObserver>> *_observers;
 }
@@ -74,33 +75,33 @@ using namespace facebook::react;
 
 - (ContextContainer::Shared)contextContainer
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   return _contextContainer;
 }
 
 - (void)setContextContainer:(ContextContainer::Shared)contextContainer
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
   _contextContainer = contextContainer;
 }
 
-- (void)setRuntimeExecutor:(RuntimeExecutor)runtimeExecutor
-{
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
-  _runtimeExecutor = runtimeExecutor;
-}
-
 - (RuntimeExecutor)runtimeExecutor
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   return _runtimeExecutor;
 }
 
+- (void)setRuntimeExecutor:(RuntimeExecutor)runtimeExecutor
+{
+  std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
+  _runtimeExecutor = runtimeExecutor;
+}
+
 #pragma mark - Internal Surface-dedicated Interface
 
 - (void)registerSurface:(RCTFabricSurface *)surface
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   [_surfaceRegistry registerSurface:surface];
   if (_scheduler) {
     [self _startSurface:surface];
@@ -109,7 +110,7 @@ using namespace facebook::react;
 
 - (void)unregisterSurface:(RCTFabricSurface *)surface
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   if (_scheduler) {
     [self _stopSurface:surface];
   }
@@ -118,7 +119,7 @@ using namespace facebook::react;
 
 - (void)setProps:(NSDictionary *)props surface:(RCTFabricSurface *)surface
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  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];
@@ -133,7 +134,7 @@ using namespace facebook::react;
                       maximumSize:(CGSize)maximumSize
                           surface:(RCTFabricSurface *)surface
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   LayoutContext layoutContext = {.pointScaleFactor = RCTScreenScale()};
   LayoutConstraints layoutConstraints = {.minimumSize = RCTSizeFromCGSize(minimumSize),
                                          .maximumSize = RCTSizeFromCGSize(maximumSize)};
@@ -144,7 +145,7 @@ using namespace facebook::react;
 
 - (void)setMinimumSize:(CGSize)minimumSize maximumSize:(CGSize)maximumSize surface:(RCTFabricSurface *)surface
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   LayoutContext layoutContext = {.pointScaleFactor = RCTScreenScale()};
   LayoutConstraints layoutConstraints = {.minimumSize = RCTSizeFromCGSize(minimumSize),
                                          .maximumSize = RCTSizeFromCGSize(maximumSize)};
@@ -155,7 +156,7 @@ using namespace facebook::react;
 
 - (BOOL)synchronouslyUpdateViewOnUIThread:(NSNumber *)reactTag props:(NSDictionary *)props
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::shared_lock<better::shared_mutex> lock(_schedulerMutex);
   ReactTag tag = [reactTag integerValue];
   UIView<RCTComponentViewProtocol> *componentView =
       [_mountingManager.componentViewRegistry findComponentViewWithTag:tag];
@@ -175,7 +176,7 @@ using namespace facebook::react;
 
 - (BOOL)suspend
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
 
   if (!_scheduler) {
     return NO;
@@ -189,7 +190,7 @@ using namespace facebook::react;
 
 - (BOOL)resume
 {
-  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  std::unique_lock<better::shared_mutex> lock(_schedulerMutex);
 
   if (_scheduler) {
     return NO;