Fabric: Stop commiting an empty tree in ShadowTree destructor

Summary:
The code is moved from the destructor a separate method that we now call in the Scheduler::stopSurface.
That makes the code more readable and resilient to possible races and ownership-related changes.

Reviewed By: JoshuaGross

Differential Revision: D17272294

fbshipit-source-id: 948d76d074577beb3dda6defdf09261b5c8abb98

Author: shergin

ReactCommon/fabric/mounting/ShadowTree.cpp

@@ -103,19 +103,6 @@ ShadowTree::ShadowTree(
 }
 
 ShadowTree::~ShadowTree() {
-  commit(
-      [](const SharedRootShadowNode &oldRootShadowNode) {
-        return std::make_shared<RootShadowNode>(
-            *oldRootShadowNode,
-            ShadowNodeFragment{
-                /* .tag = */ ShadowNodeFragment::tagPlaceholder(),
-                /* .surfaceId = */ ShadowNodeFragment::surfaceIdPlaceholder(),
-                /* .props = */ ShadowNodeFragment::propsPlaceholder(),
-                /* .eventEmitter = */
-                ShadowNodeFragment::eventEmitterPlaceholder(),
-                /* .children = */ ShadowNode::emptySharedShadowNodeSharedList(),
-            });
-      });
   mountingCoordinator_->revoke();
 }
 
@@ -206,6 +193,23 @@ bool ShadowTree::tryCommit(ShadowTreeCommitTransaction transaction) const {
   return true;
 }
 
+void ShadowTree::commitEmptyTree() const {
+  commit(
+      [](const SharedRootShadowNode &oldRootShadowNode)
+          -> UnsharedRootShadowNode {
+        return std::make_shared<RootShadowNode>(
+            *oldRootShadowNode,
+            ShadowNodeFragment{
+                /* .tag = */ ShadowNodeFragment::tagPlaceholder(),
+                /* .surfaceId = */ ShadowNodeFragment::surfaceIdPlaceholder(),
+                /* .props = */ ShadowNodeFragment::propsPlaceholder(),
+                /* .eventEmitter = */
+                ShadowNodeFragment::eventEmitterPlaceholder(),
+                /* .children = */ ShadowNode::emptySharedShadowNodeSharedList(),
+            });
+      });
+}
+
 void ShadowTree::emitLayoutEvents(
     std::vector<LayoutableShadowNode const *> &affectedLayoutableNodes) const {
   SystraceSection s("ShadowTree::emitLayoutEvents");

ReactCommon/fabric/mounting/ShadowTree.h

@@ -58,6 +58,11 @@ class ShadowTree final {
    */
   void commit(ShadowTreeCommitTransaction transaction) const;
 
+  /*
+   * Commit an empty tree (a new `RootShadowNode` with no children).
+   */
+  void commitEmptyTree() const;
+
 #pragma mark - Delegate
 
   /*

ReactCommon/fabric/uimanager/Scheduler.cpp

@@ -151,29 +151,24 @@ void Scheduler::renderTemplateToSurface(
 void Scheduler::stopSurface(SurfaceId surfaceId) const {
   SystraceSection s("Scheduler::stopSurface");
 
+  // Note, we have to do in inside `visit` function while the Shadow Tree
+  // is still being registered.
   uiManager_->getShadowTreeRegistry().visit(
-      surfaceId, [](const ShadowTree &shadowTree) {
-        // As part of stopping the Surface, we have to commit an empty tree.
-        return shadowTree.tryCommit([&](const SharedRootShadowNode
-                                            &oldRootShadowNode) {
-          return std::make_shared<RootShadowNode>(
-              *oldRootShadowNode,
-              ShadowNodeFragment{
-                  /* .tag = */ ShadowNodeFragment::tagPlaceholder(),
-                  /* .surfaceId = */
-                  ShadowNodeFragment::surfaceIdPlaceholder(),
-                  /* .props = */ ShadowNodeFragment::propsPlaceholder(),
-                  /* .eventEmitter = */
-                  ShadowNodeFragment::eventEmitterPlaceholder(),
-                  /* .children = */
-                  ShadowNode::emptySharedShadowNodeSharedList(),
-              });
-        });
+      surfaceId, [](ShadowTree const &shadowTree) {
+        // 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 that will perform that.
+        shadowTree.commitEmptyTree();
       });
 
-  auto shadowTree = uiManager_->getShadowTreeRegistry().remove(surfaceId);
-  shadowTree->setDelegate(nullptr);
+  // Waiting for all concurrent commits to be finished and unregistering the
+  // `ShadowTree`.
+  uiManager_->getShadowTreeRegistry().remove(surfaceId);
 
+  // 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
+  // fail silently.
   auto uiManager = uiManager_;
   runtimeExecutor_([=](jsi::Runtime &runtime) {
     uiManager->visitBinding([&](UIManagerBinding const &uiManagerBinding) {