Fabric: Reviving of RN_SHADOW_TREE_INTROSPECTION in DEBUG mode

shergin · facebook-github-bot · commit 6729a3e0bfc0 · 2020-08-28T10:22:41.000-07:00
Summary:
Shadow tree introspection was disabled for a while, now we need it back working. This diff also restructures the logic of `MountingCoordinator::pullTransaction()` splitting it into two sections, first one for the base case and the second for the overriding case.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: mdvacca

Differential Revision: D23374948

fbshipit-source-id: 0b5f1c598975bceb3dcb6a0eaee67ff58ef9dda1
diff --git a/ReactCommon/react/renderer/mounting/MountingCoordinator.cpp b/ReactCommon/react/renderer/mounting/MountingCoordinator.cpp
@@ -80,98 +80,98 @@ void MountingCoordinator::resetLatestRevision() const {
   lastRevision_.reset();
 }
 
-#ifdef RN_SHADOW_TREE_INTROSPECTION
-void MountingCoordinator::validateTransactionAgainstStubViewTree(
-    ShadowViewMutationList const &mutations,
-    bool assertEquality) const {
-  std::string line;
-
-  std::stringstream ssMutations(getDebugDescription(mutations, {}));
-  while (std::getline(ssMutations, line, '\n')) {
-    LOG(ERROR) << "Mutations:" << line;
-  }
-
-  stubViewTree_.mutate(mutations);
-  auto stubViewTree =
-      stubViewTreeFromShadowNode(lastRevision_->getRootShadowNode());
+better::optional<MountingTransaction> MountingCoordinator::pullTransaction()
+    const {
+  std::lock_guard<std::mutex> lock(mutex_);
 
-  std::stringstream ssOldTree(
-      baseRevision_.getRootShadowNode().getDebugDescription());
-  while (std::getline(ssOldTree, line, '\n')) {
-    LOG(ERROR) << "Old tree:" << line;
-  }
+  auto transaction = better::optional<MountingTransaction>{};
 
-  std::stringstream ssNewTree(
-      lastRevision_->getRootShadowNode().getDebugDescription());
-  while (std::getline(ssNewTree, line, '\n')) {
-    LOG(ERROR) << "New tree:" << line;
-  }
+  // Base case
+  if (lastRevision_.has_value()) {
+    number_++;
 
-  if (assertEquality) {
-    assert(stubViewTree_ == stubViewTree);
-  }
-}
-#endif
+    auto telemetry = lastRevision_->getTelemetry();
 
-better::optional<MountingTransaction> MountingCoordinator::pullTransaction()
-    const {
-  std::lock_guard<std::mutex> lock(mutex_);
+    telemetry.willDiff();
 
-  auto mountingOverrideDelegate = mountingOverrideDelegate_.lock();
+    auto mutations = calculateShadowViewMutations(
+        baseRevision_.getRootShadowNode(), lastRevision_->getRootShadowNode());
 
-  bool shouldOverridePullTransaction = mountingOverrideDelegate &&
-      mountingOverrideDelegate->shouldOverridePullTransaction();
+    telemetry.didDiff();
 
-  if (!shouldOverridePullTransaction && !lastRevision_.has_value()) {
-    return {};
-  }
+    baseRevision_ = std::move(*lastRevision_);
+    lastRevision_.reset();
 
-  number_++;
-
-  ShadowViewMutation::List diffMutations{};
-  auto telemetry =
-      (lastRevision_.hasValue() ? lastRevision_->getTelemetry()
-                                : TransactionTelemetry{});
-  if (!lastRevision_.hasValue()) {
-    telemetry.willLayout();
-    telemetry.didLayout();
-    telemetry.willCommit();
-    telemetry.didCommit();
-  }
-  telemetry.willDiff();
-  if (lastRevision_.hasValue()) {
-    diffMutations = calculateShadowViewMutations(
-        baseRevision_.getRootShadowNode(),
-        lastRevision_->getRootShadowNode(),
-        enableReparentingDetection_);
+    transaction = MountingTransaction{
+        surfaceId_, number_, std::move(mutations), telemetry};
   }
-  telemetry.didDiff();
 
-  better::optional<MountingTransaction> transaction{};
+  // Override case
+  auto mountingOverrideDelegate = mountingOverrideDelegate_.lock();
+  auto shouldOverridePullTransaction = mountingOverrideDelegate &&
+      mountingOverrideDelegate->shouldOverridePullTransaction();
 
-  // The override delegate can provide custom mounting instructions,
-  // even if there's no `lastRevision_`. Consider cases of animation frames
-  // in between React tree updates.
   if (shouldOverridePullTransaction) {
+    auto mutations = ShadowViewMutation::List{};
+    auto telemetry = TransactionTelemetry{};
+
+    if (transaction.has_value()) {
+      mutations = transaction->getMutations();
+      telemetry = transaction->getTelemetry();
+    } else {
+      telemetry.willLayout();
+      telemetry.didLayout();
+      telemetry.willCommit();
+      telemetry.didCommit();
+    }
+
     transaction = mountingOverrideDelegate->pullTransaction(
-        surfaceId_, number_, telemetry, std::move(diffMutations));
-  } else if (lastRevision_.hasValue()) {
-    transaction = MountingTransaction{
-        surfaceId_, number_, std::move(diffMutations), telemetry};
+        surfaceId_, number_, telemetry, std::move(mutations));
   }
 
-  if (lastRevision_.hasValue()) {
 #ifdef RN_SHADOW_TREE_INTROSPECTION
-    // Only validate non-animated transactions - it's garbage to validate
-    // animated transactions, since the stub view tree likely won't match
-    // the committed tree during an animation.
-    this->validateTransactionAgainstStubViewTree(
-        transaction->getMutations(), !shouldOverridePullTransaction);
-#endif
-
-    baseRevision_ = std::move(*lastRevision_);
-    lastRevision_.reset();
+  if (transaction.has_value()) {
+    // We have something to validate.
+    auto mutations = transaction->getMutations();
+
+    // No matter what the source of the transaction is, it must be able to
+    // mutate the existing stub view tree.
+    stubViewTree_.mutate(mutations);
+
+    // If the transaction was overridden, we don't have a model of the shadow
+    // tree therefore we cannot validate the validity of the mutation
+    // instructions.
+    if (!shouldOverridePullTransaction) {
+      auto line = std::string{};
+
+      auto stubViewTree =
+          stubViewTreeFromShadowNode(baseRevision_.getRootShadowNode());
+
+      if (stubViewTree_ != stubViewTree) {
+        std::stringstream ssOldTree(
+            baseRevision_.getRootShadowNode().getDebugDescription());
+        while (std::getline(ssOldTree, line, '\n')) {
+          LOG(ERROR) << "Old tree:" << line;
+        }
+
+        std::stringstream ssMutations(getDebugDescription(mutations, {}));
+        while (std::getline(ssMutations, line, '\n')) {
+          LOG(ERROR) << "Mutations:" << line;
+        }
+
+        std::stringstream ssNewTree(
+            lastRevision_->getRootShadowNode().getDebugDescription());
+        while (std::getline(ssNewTree, line, '\n')) {
+          LOG(ERROR) << "New tree:" << line;
+        }
+      }
+
+      assert(
+          (stubViewTree_ == stubViewTree) &&
+          "Incorrect set of mutations detected.");
+    }
   }
+#endif
 
   return transaction;
 }
diff --git a/ReactCommon/react/renderer/mounting/MountingCoordinator.h b/ReactCommon/react/renderer/mounting/MountingCoordinator.h
@@ -17,6 +17,10 @@
 #include <react/renderer/mounting/TelemetryController.h>
 #include "ShadowTreeRevision.h"
 
+#ifndef NDEBUG
+#define RN_SHADOW_TREE_INTROSPECTION 1
+#endif
+
 #ifdef RN_SHADOW_TREE_INTROSPECTION
 #include <react/renderer/mounting/stubs.h>
 #endif
@@ -113,9 +117,6 @@ class MountingCoordinator final {
   bool enableReparentingDetection_{false}; // temporary
 
 #ifdef RN_SHADOW_TREE_INTROSPECTION
-  void validateTransactionAgainstStubViewTree(
-      ShadowViewMutationList const &mutations,
-      bool assertEquality) const;
   mutable StubViewTree stubViewTree_; // Protected by `mutex_`.
 #endif
 };
