Fabric: Fixed deadlock in Surface Handler

Summary:
As part of stoping a Surface, we have to commit an empty shadow node tree. And to avoid a deadlock we have to do it not under a `linkMutex_`. To do so, we need to move the part that commits an empty tree from UIManager to SurfaceHandler. And this diff does exactly this.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D27010535

fbshipit-source-id: 60cc79b1c81d7340b550e78653737d2cc5bec24d

ReactCommon/react/renderer/mounting/ShadowTree.h

@@ -31,6 +31,8 @@ using ShadowTreeCommitTransaction = std::function<RootShadowNode::Unshared(
  */
 class ShadowTree final {
  public:
+  using Unique = std::unique_ptr<ShadowTree>;
+
   /*
    * Represents a result of a `commit` operation.
    */

ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp

@@ -82,13 +82,22 @@ void SurfaceHandler::start() const noexcept {
 }
 
 void SurfaceHandler::stop() const noexcept {
-  std::unique_lock<better::shared_mutex> lock(linkMutex_);
-  react_native_assert(
-      link_.status == Status::Running && "Surface must be running.");
+  auto shadowTree = ShadowTree::Unique{};
+  {
+    std::unique_lock<better::shared_mutex> lock(linkMutex_);
+    react_native_assert(
+        link_.status == Status::Running && "Surface must be running.");
 
-  link_.status = Status::Registered;
-  link_.shadowTree = nullptr;
-  link_.uiManager->stopSurface(parameters_.surfaceId);
+    link_.status = Status::Registered;
+    link_.shadowTree = nullptr;
+    shadowTree = link_.uiManager->stopSurface(parameters_.surfaceId);
+  }
+
+  // As part of stopping a Surface, we need to properly destroy all
+  // mounted views, so we need to commit an empty tree to trigger all
+  // side-effects (including destroying and removing mounted views).
+  react_native_assert(shadowTree && "`shadowTree` must not be null.");
+  shadowTree->commitEmptyTree();
 }
 
 void SurfaceHandler::setDisplayMode(DisplayMode displayMode) const noexcept {

ReactCommon/react/renderer/uimanager/UIManager.cpp

@@ -146,7 +146,7 @@ ShadowTree const &UIManager::startSurface(
   return *shadowTreePointer;
 }
 
-void UIManager::stopSurface(SurfaceId surfaceId) const {
+ShadowTree::Unique UIManager::stopSurface(SurfaceId surfaceId) const {
   SystraceSection s("UIManager::stopSurface");
 
   // Stop any ongoing animations.
@@ -156,13 +156,6 @@ void UIManager::stopSurface(SurfaceId surfaceId) const {
   // `ShadowTree`.
   auto shadowTree = getShadowTreeRegistry().remove(surfaceId);
 
-  // As part of stopping a Surface, we need to properly destroy all
-  // mounted views, so we need to commit an empty tree to trigger all
-  // side-effects (including destroying and removing mounted views).
-  if (shadowTree) {
-    shadowTree->commitEmptyTree();
-  }
-
   // We execute JavaScript/React part of the process at the very end to minimize
   // any visible side-effects of stopping the Surface. Any possible commits from
   // the JavaScript side will not be able to reference a `ShadowTree` and will
@@ -175,6 +168,8 @@ void UIManager::stopSurface(SurfaceId surfaceId) const {
 
     uiManagerBinding->stopSurface(runtime, surfaceId);
   });
+
+  return shadowTree;
 }
 
 ShadowNode::Shared UIManager::getNewestCloneOfShadowNode(

ReactCommon/react/renderer/uimanager/UIManager.h

@@ -90,7 +90,7 @@ class UIManager final : public ShadowTreeDelegate {
       LayoutConstraints const &layoutConstraints,
       LayoutContext const &layoutContext) const;
 
-  void stopSurface(SurfaceId surfaceId) const;
+  ShadowTree::Unique stopSurface(SurfaceId surfaceId) const;
 
 #pragma mark - ShadowTreeDelegate