Fabric: Changes in State reconciliation

Summary:
I spent the last several days thinking about state reconciliation issues, some crashes (T65586949) that suspiciously happen somewhere inside, and a bunch of issues that might be connected to that (possibly, some of T65516263 sub-task).

I cannot see some obvious problems in the current state reconciliation algorithm that might cause the crash (because of some use-after-free or other pure C++ issues), but I suspect some of the problems we experience might be caused by some details of how we reconcile states.

In the current approach, we rank all states based on the "hierarchical" history of their creation (state version is being calculated based on the version of the base tree). That's usually fine but in some cases when trees are being constructed concurrently, a logical version of a based tree does not correspond to the local version of a committed tree. In other words, the linear history of commits does not always correspond to the "hierarchical" history of trees generation that was done by different parties (e.g. React vs native state update pipeline).

In this diff, I tried to change the approach to change the algorithm to follow this logic: If some state is `obsolete` (already been committed and then replaced with newer one), we replace that with the most recent one. This change does not introduce the `obsolete` flag; is already used by State infra to avoid cloning nodes with an outdated state.

Interestingly, it fixes the issue with an empty BottomSheet on Android (T66177144). See the attached video. The hope is that it's also will

This change theoretically might affect all things that use State, so it hard to predict what can break and how. So, if we don't see obvious problems here, I would set up a GK/QE and run the experiment in prod.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: JoshuaGross

Differential Revision: D21295137

fbshipit-source-id: e5613218d3e11a56623cab9bbf2540495b2b24e8

ReactCommon/fabric/core/shadownode/ShadowNodeFamily.cpp

@@ -119,6 +119,15 @@ void ShadowNodeFamily::setMostRecentState(State::Shared const &state) const {
   mostRecentState_ = state;
 }
 
+std::shared_ptr<State const> ShadowNodeFamily::getMostRecentStateIfObsolete(
+    State const &state) const {
+  std::unique_lock<better::shared_mutex> lock(mutex_);
+  if (!state.isObsolete_) {
+    return {};
+  }
+  return mostRecentState_;
+}
+
 void ShadowNodeFamily::dispatchRawState(
     StateUpdate &&stateUpdate,
     EventPriority priority) const {

ReactCommon/fabric/core/shadownode/ShadowNodeFamily.h

@@ -88,6 +88,15 @@ class ShadowNodeFamily {
  private:
   friend ShadowNode;
   friend ShadowNodeFamilyFragment;
+  friend State;
+
+  /*
+   * Returns the most recent state if the given `state` is obsolete,
+   * otherwise returns `nullptr`.
+   * To be used by `State` only.
+   */
+  std::shared_ptr<State const> getMostRecentStateIfObsolete(
+      State const &state) const;
 
   EventDispatcher::Weak eventDispatcher_;
   mutable std::shared_ptr<State const> mostRecentState_;

ReactCommon/fabric/core/state/State.cpp

@@ -32,6 +32,15 @@ State::Shared State::getMostRecentState() const {
   return family->getMostRecentState();
 }
 
+State::Shared State::getMostRecentStateIfObsolete() const {
+  auto family = family_.lock();
+  if (!family) {
+    return {};
+  }
+
+  return family->getMostRecentStateIfObsolete(*this);
+}
+
 size_t State::getRevision() const {
   return revision_;
 }

ReactCommon/fabric/core/state/State.h

@@ -47,6 +47,12 @@ class State {
    */
   State::Shared getMostRecentState() const;
 
+  /*
+   * Returns the most recent state (same as `getMostRecentState()` method)
+   * if this state is obsolete, otherwise returns `nullptr`.
+   */
+  State::Shared getMostRecentStateIfObsolete() const;
+
   /*
    * Returns a revision number of the `State` object.
    * The number is being automatically assigned during the creation of `State`

ReactCommon/fabric/mounting/ShadowTree.cpp

@@ -22,6 +22,141 @@
 namespace facebook {
 namespace react {
 
+/*
+ * Generates (possibly) a new tree where all nodes with non-obsolete `State`
+ * objects. If all `State` objects in the tree are not obsolete for the moment
+ * of calling, the function returns `nullptr` (as an indication that no
+ * additional work is required).
+ */
+static ShadowNode::Unshared progressState(ShadowNode const &shadowNode) {
+  auto isStateChanged = false;
+  auto areChildrenChanged = false;
+
+  auto newState = shadowNode.getState();
+  if (newState) {
+    newState = newState->getMostRecentStateIfObsolete();
+    if (newState) {
+      isStateChanged = true;
+    }
+  }
+
+  auto newChildren = ShadowNode::ListOfShared{};
+  if (shadowNode.getChildren().size() > 0) {
+    auto index = size_t{0};
+    for (auto const &childNode : shadowNode.getChildren()) {
+      auto newChildNode = progressState(*childNode);
+      if (newChildNode) {
+        if (!areChildrenChanged) {
+          // Making a copy before the first mutation.
+          newChildren = shadowNode.getChildren();
+        }
+        newChildren[index] = newChildNode;
+        areChildrenChanged = true;
+      }
+      index++;
+    }
+  }
+
+  if (!areChildrenChanged && !isStateChanged) {
+    return nullptr;
+  }
+
+  return shadowNode.clone({
+      ShadowNodeFragment::propsPlaceholder(),
+      areChildrenChanged ? std::make_shared<ShadowNode::ListOfShared const>(
+                               std::move(newChildren))
+                         : ShadowNodeFragment::childrenPlaceholder(),
+      isStateChanged ? newState : ShadowNodeFragment::statePlaceholder(),
+  });
+}
+
+/*
+ * An optimized version of the previous function (and relies on it).
+ * The function uses a given base tree to exclude unchanged (equal) parts
+ * of the three from the traversing.
+ */
+static ShadowNode::Unshared progressState(
+    ShadowNode const &shadowNode,
+    ShadowNode const &baseShadowNode) {
+  // The intuition behind the complexity:
+  // - A very few nodes have associated state, therefore it's mostly reading and
+  //   it only writes when state objects were found obsolete;
+  // - Most before-after trees are aligned, therefore most tree branches will be
+  //   skipped;
+  // - If trees are significantly different, any other algorithm will have
+  //   close to linear complexity.
+
+  auto isStateChanged = false;
+  auto areChildrenChanged = false;
+
+  auto newState = shadowNode.getState();
+  if (newState) {
+    newState = newState->getMostRecentStateIfObsolete();
+    if (newState) {
+      isStateChanged = true;
+    }
+  }
+
+  auto &children = shadowNode.getChildren();
+  auto &baseChildren = baseShadowNode.getChildren();
+  auto newChildren = ShadowNode::ListOfShared{};
+
+  auto childrenSize = children.size();
+  auto baseChildrenSize = baseChildren.size();
+  auto index = size_t{0};
+
+  // Stage 1: Aligned part.
+  for (index = 0; index < childrenSize && index < baseChildrenSize; index++) {
+    const auto &childNode = *children.at(index);
+    const auto &baseChildNode = *baseChildren.at(index);
+
+    if (&childNode == &baseChildNode) {
+      // Nodes are identical, skipping.
+      continue;
+    }
+
+    if (!ShadowNode::sameFamily(childNode, baseChildNode)) {
+      // Totally different nodes, updating is impossible.
+      break;
+    }
+
+    auto newChildNode = progressState(childNode, baseChildNode);
+    if (newChildNode) {
+      if (!areChildrenChanged) {
+        // Making a copy before the first mutation.
+        newChildren = children;
+      }
+      newChildren[index] = newChildNode;
+      areChildrenChanged = true;
+    }
+  }
+
+  // Stage 2: Misaligned part.
+  for (; index < childrenSize; index++) {
+    auto newChildNode = progressState(*children.at(index));
+    if (newChildNode) {
+      if (!areChildrenChanged) {
+        // Making a copy before the first mutation.
+        newChildren = children;
+      }
+      newChildren[index] = newChildNode;
+      areChildrenChanged = true;
+    }
+  }
+
+  if (!areChildrenChanged && !isStateChanged) {
+    return nullptr;
+  }
+
+  return shadowNode.clone({
+      ShadowNodeFragment::propsPlaceholder(),
+      areChildrenChanged ? std::make_shared<ShadowNode::ListOfShared const>(
+                               std::move(newChildren))
+                         : ShadowNodeFragment::childrenPlaceholder(),
+      isStateChanged ? newState : ShadowNodeFragment::statePlaceholder(),
+  });
+}
+
 static void updateMountedFlag(
     const SharedShadowNodeList &oldChildren,
     const SharedShadowNodeList &newChildren) {
@@ -161,14 +296,23 @@ bool ShadowTree::tryCommit(
     return false;
   }
 
-  // Compare state revisions of old and new root
-  // Children of the root node may be mutated in-place
   if (enableStateReconciliation) {
-    UnsharedShadowNode reconciledNode =
-        reconcileStateWithTree(newRootShadowNode.get(), oldRootShadowNode);
-    if (reconciledNode != nullptr) {
-      newRootShadowNode = std::make_shared<RootShadowNode>(
-          *reconciledNode, ShadowNodeFragment{});
+    if (useNewApproachToStateReconciliation_) {
+      auto updatedNewRootShadowNode =
+          progressState(*newRootShadowNode, *oldRootShadowNode);
+      if (updatedNewRootShadowNode) {
+        newRootShadowNode =
+            std::static_pointer_cast<RootShadowNode>(updatedNewRootShadowNode);
+      }
+    } else {
+      // Compare state revisions of old and new root
+      // Children of the root node may be mutated in-place
+      UnsharedShadowNode reconciledNode =
+          reconcileStateWithTree(newRootShadowNode.get(), oldRootShadowNode);
+      if (reconciledNode != nullptr) {
+        newRootShadowNode = std::make_shared<RootShadowNode>(
+            *reconciledNode, ShadowNodeFragment{});
+      }
     }
   }
 

ReactCommon/fabric/mounting/ShadowTree.h

@@ -88,6 +88,7 @@ class ShadowTree final {
   mutable ShadowTreeRevision::Number revisionNumber_{
       0}; // Protected by `commitMutex_`.
   MountingCoordinator::Shared mountingCoordinator_;
+  bool useNewApproachToStateReconciliation_{true};
 };
 
 } // namespace react