From 83f8d13a996cbddf5c9d8bbd8205ea26d36887ea Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Tue, 10 Sep 2019 21:30:53 -0700
Subject: [PATCH] Fabric: Explicit Scheduler creation and destruction
 management in RCTSurfacePresenter

Summary: Previously, the `_scheduler` method in `RCTSurfacePresenter` was implemented as a lazy getter. The only problem with that is that Scheduler instance might be (re)created in the middle of the hot-reloading process (e.g. external request to relayout some Surface might trigger that). Since it does not make any sense to create an empty Scheduler during the reloading process, now the Scheduler creation only happens in constructor and right after the VM is reloaded.

Reviewed By: JoshuaGross

Differential Revision: D17299441

fbshipit-source-id: 273451bbb03e8cdf532131adfdf3bc60c34e997e
---
 .../RCTSurfaceHostingView.mm                  |  2 -
 React/Fabric/RCTSurfacePresenter.mm           | 67 +++++++++----------
 2 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/React/Base/Surface/SurfaceHostingView/RCTSurfaceHostingView.mm b/React/Base/Surface/SurfaceHostingView/RCTSurfaceHostingView.mm
index 9afbaad0ff2..335169fd8b1 100644
--- a/React/Base/Surface/SurfaceHostingView/RCTSurfaceHostingView.mm
+++ b/React/Base/Surface/SurfaceHostingView/RCTSurfaceHostingView.mm
@@ -80,10 +80,8 @@ RCT_NOT_IMPLEMENTED(- (nullable instancetype)initWithCoder:(NSCoder *)coder)
     &maximumSize
   );
 
-  if (RCTSurfaceStageIsRunning(_stage)) {
     [_surface setMinimumSize:minimumSize
                  maximumSize:maximumSize];
-  }
 }
 
 - (CGSize)intrinsicContentSize
diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm
index 5ea23732df2..0f108d37199 100644
--- a/React/Fabric/RCTSurfacePresenter.mm
+++ b/React/Fabric/RCTSurfacePresenter.mm
@@ -51,7 +51,6 @@ using namespace facebook::react;
 
 @implementation RCTSurfacePresenter {
   std::mutex _schedulerMutex;
-  std::mutex _contextContainerMutex;
   RCTScheduler
       *_Nullable _scheduler; // Thread-safe. Mutation of the instance variable is protected by `_schedulerMutex`.
   RCTMountingManager *_mountingManager; // Thread-safe.
@@ -59,6 +58,7 @@ using namespace facebook::react;
   RCTBridge *_bridge; // Unsafe. We are moving away from Bridge.
   RCTBridge *_batchedBridge;
   std::shared_ptr<const ReactNativeConfig> _reactNativeConfig;
+  ContextContainer::Shared _contextContainer;
   better::shared_mutex _observerListMutex;
   NSMutableArray<id<RCTSurfacePresenterObserver>> *_observers;
   RCTImageLoader *_imageLoader;
@@ -88,6 +88,8 @@ using namespace facebook::react;
       _reactNativeConfig = std::make_shared<const EmptyReactNativeConfig>();
     }
 
+    _contextContainer = std::make_shared<ContextContainer>();
+
     _observers = [NSMutableArray array];
 
     [[NSNotificationCenter defaultCenter] addObserver:self
@@ -98,6 +100,8 @@ using namespace facebook::react;
                                              selector:@selector(handleJavaScriptDidLoadNotification:)
                                                  name:RCTJavaScriptDidLoadNotification
                                                object:_bridge];
+
+    [self _createScheduler];
   }
 
   return self;
@@ -186,13 +190,15 @@ using namespace facebook::react;
 
 #pragma mark - Private
 
-- (RCTScheduler *)_scheduler
+- (nullable RCTScheduler *)_scheduler
 {
   std::lock_guard<std::mutex> lock(_schedulerMutex);
+  return _scheduler;
+}
 
-  if (_scheduler) {
-    return _scheduler;
-  }
+- (void)_createScheduler
+{
+  std::lock_guard<std::mutex> lock(_schedulerMutex);
 
   auto componentRegistryFactory = [factory = wrapManagedObject(self.componentViewFactory)](
                                       EventDispatcher::Weak const &eventDispatcher,
@@ -203,6 +209,8 @@ using namespace facebook::react;
 
   auto runtimeExecutor = [self getRuntimeExecutor];
 
+  [self _updateContextContainerIfNeeded_DEPRECATED];
+
   auto toolbox = SchedulerToolbox{};
   toolbox.contextContainer = self.contextContainer;
   toolbox.componentRegistryFactory = componentRegistryFactory;
@@ -218,11 +226,13 @@ using namespace facebook::react;
 
   _scheduler = [[RCTScheduler alloc] initWithToolbox:toolbox];
   _scheduler.delegate = self;
-
-  return _scheduler;
 }
 
-@synthesize contextContainer = _contextContainer;
+- (void)_destroyScheduler
+{
+  std::lock_guard<std::mutex> lock(_schedulerMutex);
+  _scheduler = nil;
+}
 
 - (RuntimeExecutor)getRuntimeExecutor
 {
@@ -250,16 +260,6 @@ using namespace facebook::react;
 
 - (ContextContainer::Shared)contextContainer
 {
-  std::lock_guard<std::mutex> lock(_contextContainerMutex);
-
-  if (_contextContainer) {
-    return _contextContainer;
-  }
-
-  _contextContainer = std::make_shared<ContextContainer>();
-
-  [self _updateContextContainerIfNeeded_DEPRECATED];
-
   return _contextContainer;
 }
 
@@ -409,34 +409,27 @@ using namespace facebook::react;
 
 - (void)handleBridgeWillReloadNotification:(NSNotification *)notification
 {
-  {
-    std::lock_guard<std::mutex> lock(_schedulerMutex);
-    if (!_scheduler) {
-      // Seems we are already in the realoding process.
-      return;
-    }
+  if (!self._scheduler) {
+    // Seems we are already in the reloading process.
+    return;
   }
 
   [self _stopAllSurfaces];
-
-  {
-    std::lock_guard<std::mutex> lock(_schedulerMutex);
-    _scheduler = nil;
-  }
+  [self _destroyScheduler];
 }
 
 - (void)handleJavaScriptDidLoadNotification:(NSNotification *)notification
 {
   RCTBridge *bridge = notification.userInfo[@"bridge"];
-  if (bridge != _batchedBridge) {
-    _batchedBridge = bridge;
-
-    // Some of the injected dependencies are tight to a particular instance of Bridge,
-    // so they need to be reinjected.
-    [self _updateContextContainerIfNeeded_DEPRECATED];
-
-    [self _startAllSurfaces];
+  if (bridge == _batchedBridge) {
+    // Nothing really changed.
+    return;
   }
+
+  _batchedBridge = bridge;
+
+  [self _createScheduler];
+  [self _startAllSurfaces];
 }
 
 @end