Fix crash in PreAllocation optimization

Summary:
PreAllocation currently always happens at revision 0 (after ShadowNode creation), and all CREATE mutations are triggered for ShadowNodes at revision 1 or higher (since CREATE mutations are generated by the differ, it means that all ShadowNodes have revision 1 or higher when CompleteRoot is called). This means that between PreAllocation and CREATE, we /always/ expect at least one clone.

It is possible for a node to be "non-view-forming" at revision 0, causing view preallocation to be skipped, and "view-forming" at revision 1 (causing the CREATE mutation to be thrown away, since all CREATE mutations of revision 0 or 1 are thrown away). This causes a crash. It is extremely marginal, but there are repros in the wild.

Thus, I'm introducing one new UIManager and Scheduler delegate method that allows the mounting layer to be notified of clones. If a clone from rev 0->1 results in a node going from non-view-forming to view-forming, we can preallocate then, as well.

This resolves this crash, and allows us to keep experimenting safely with this View PreAllocation optimization. I believe all edge-cases are accounted for.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D29043426

fbshipit-source-id: dff11d3140ded1cbb02a5518a3aeb52dc812cc50

React/Fabric/RCTScheduler.mm

@@ -30,10 +30,19 @@ class SchedulerDelegateProxy : public SchedulerDelegate {
     [scheduler.delegate schedulerDidFinishTransaction:mountingCoordinator];
   }
 
-  void schedulerDidRequestPreliminaryViewAllocation(SurfaceId surfaceId, const ShadowView &shadowView) override
+  void schedulerDidRequestPreliminaryViewAllocation(SurfaceId surfaceId, const ShadowNode &shadowNode) override
   {
     // Does nothing.
-    // Preemptive allocation of native views on iOS does not require this call.
+    // This delegate method is not currently used on iOS.
+  }
+
+  void schedulerDidCloneShadowNode(
+      SurfaceId surfaceId,
+      const ShadowNode &oldShadowNode,
+      const ShadowNode &newShadowNode) override
+  {
+    // Does nothing.
+    // This delegate method is not currently used on iOS.
   }
 
   void schedulerDidDispatchCommand(

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp

@@ -1128,13 +1128,9 @@ void Binding::driveCxxAnimations() {
   scheduler_->animationTick();
 }
 
-void Binding::schedulerDidRequestPreliminaryViewAllocation(
+void Binding::preallocateShadowView(
     const SurfaceId surfaceId,
     const ShadowView &shadowView) {
-  if (disablePreallocateViews_) {
-    return;
-  }
-
   jni::global_ref<jobject> localJavaUIManager = getJavaUIManager();
   if (!localJavaUIManager) {
     LOG(ERROR)
@@ -1144,11 +1140,6 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
 
   bool isLayoutableShadowNode = shadowView.layoutMetrics != EmptyLayoutMetrics;
 
-  if (disableVirtualNodePreallocation_ &&
-      !shadowView.traits.check(ShadowNodeTraits::Trait::FormsView)) {
-    return;
-  }
-
   static auto preallocateView =
       jni::findClassStatic(Binding::UIManagerJavaDescriptor)
           ->getMethod<void(
@@ -1191,6 +1182,53 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
       isLayoutableShadowNode);
 }
 
+void Binding::schedulerDidRequestPreliminaryViewAllocation(
+    const SurfaceId surfaceId,
+    const ShadowNode &shadowNode) {
+  if (disablePreallocateViews_) {
+    return;
+  }
+
+  auto shadowView = ShadowView(shadowNode);
+
+  if (disableVirtualNodePreallocation_ &&
+      !shadowView.traits.check(ShadowNodeTraits::Trait::FormsView)) {
+    return;
+  }
+
+  preallocateShadowView(surfaceId, shadowView);
+}
+
+void Binding::schedulerDidCloneShadowNode(
+    SurfaceId surfaceId,
+    const ShadowNode &oldShadowNode,
+    const ShadowNode &newShadowNode) {
+  // This is only necessary if view preallocation was skipped during
+  // createShadowNode
+  if (!disableVirtualNodePreallocation_) {
+    return;
+  }
+
+  // We may need to PreAllocate a ShadowNode at this point if this is the
+  // earliest point it is possible to do so:
+  // 1. The revision is exactly 1
+  // 2. At revision 0 (the old node), View Preallocation would have been skipped
+
+  if (newShadowNode.getProps()->revision != 1) {
+    return;
+  }
+  if (oldShadowNode.getProps()->revision != 0) {
+    return;
+  }
+
+  // If the new node is concrete and the old wasn't, we can preallocate
+  if (!oldShadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView) &&
+      newShadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView)) {
+    auto shadowView = ShadowView(newShadowNode);
+    preallocateShadowView(surfaceId, shadowView);
+  }
+}
+
 void Binding::schedulerDidDispatchCommand(
     const ShadowView &shadowView,
     std::string const &commandName,

ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h

@@ -135,9 +135,18 @@ class Binding : public jni::HybridClass<Binding>,
   void schedulerDidFinishTransaction(
       MountingCoordinator::Shared const &mountingCoordinator) override;
 
+  void preallocateShadowView(
+      const SurfaceId surfaceId,
+      const ShadowView &shadowView);
+
   void schedulerDidRequestPreliminaryViewAllocation(
       const SurfaceId surfaceId,
-      const ShadowView &shadowView) override;
+      const ShadowNode &shadowNode) override;
+
+  void schedulerDidCloneShadowNode(
+      SurfaceId surfaceId,
+      const ShadowNode &oldShadowNode,
+      const ShadowNode &newShadowNode) override;
 
   void schedulerDidDispatchCommand(
       const ShadowView &shadowView,

ReactCommon/react/renderer/scheduler/Scheduler.cpp

@@ -287,14 +287,23 @@ void Scheduler::uiManagerDidFinishTransaction(
     delegate_->schedulerDidFinishTransaction(mountingCoordinator);
   }
 }
-void Scheduler::uiManagerDidCreateShadowNode(
-    const ShadowNode::Shared &shadowNode) {
+void Scheduler::uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) {
   SystraceSection s("Scheduler::uiManagerDidCreateShadowNode");
 
   if (delegate_) {
-    auto shadowView = ShadowView(*shadowNode);
     delegate_->schedulerDidRequestPreliminaryViewAllocation(
-        shadowNode->getSurfaceId(), shadowView);
+        shadowNode.getSurfaceId(), shadowNode);
+  }
+}
+
+void Scheduler::uiManagerDidCloneShadowNode(
+    const ShadowNode &oldShadowNode,
+    const ShadowNode &newShadowNode) {
+  SystraceSection s("Scheduler::uiManagerDidCloneShadowNode");
+
+  if (delegate_) {
+    delegate_->schedulerDidCloneShadowNode(
+        newShadowNode.getSurfaceId(), oldShadowNode, newShadowNode);
   }
 }
 

ReactCommon/react/renderer/scheduler/Scheduler.h

@@ -83,8 +83,10 @@ class Scheduler final : public UIManagerDelegate {
 
   void uiManagerDidFinishTransaction(
       MountingCoordinator::Shared const &mountingCoordinator) override;
-  void uiManagerDidCreateShadowNode(
-      const ShadowNode::Shared &shadowNode) override;
+  void uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) override;
+  void uiManagerDidCloneShadowNode(
+      const ShadowNode &oldShadowNode,
+      const ShadowNode &newShadowNode) override;
   void uiManagerDidDispatchCommand(
       const ShadowNode::Shared &shadowNode,
       std::string const &commandName,

ReactCommon/react/renderer/scheduler/SchedulerDelegate.h

@@ -34,7 +34,15 @@ class SchedulerDelegate {
    */
   virtual void schedulerDidRequestPreliminaryViewAllocation(
       SurfaceId surfaceId,
-      const ShadowView &shadowView) = 0;
+      const ShadowNode &shadowView) = 0;
+
+  /*
+   * Called right after a ShadowNode is cloned.
+   */
+  virtual void schedulerDidCloneShadowNode(
+      SurfaceId surfaceId,
+      const ShadowNode &oldShadowNode,
+      const ShadowNode &newShadowNode) = 0;
 
   virtual void schedulerDidDispatchCommand(
       const ShadowView &shadowView,

ReactCommon/react/renderer/uimanager/UIManager.cpp

@@ -73,7 +73,7 @@ SharedShadowNode UIManager::createNode(
       family);
 
   if (delegate_) {
-    delegate_->uiManagerDidCreateShadowNode(shadowNode);
+    delegate_->uiManagerDidCreateShadowNode(*shadowNode.get());
   }
   if (leakChecker_) {
     leakChecker_->uiManagerDidCreateShadowNodeFamily(family);
@@ -99,6 +99,11 @@ SharedShadowNode UIManager::cloneNode(
           /* .children = */ children,
       });
 
+  if (delegate_) {
+    delegate_->uiManagerDidCloneShadowNode(
+        *shadowNode.get(), *clonedShadowNode.get());
+  }
+
   return clonedShadowNode;
 }
 

ReactCommon/react/renderer/uimanager/UIManagerDelegate.h

@@ -30,8 +30,16 @@ class UIManagerDelegate {
    * might use this to optimistically allocate a new native view
    * instances.
    */
-  virtual void uiManagerDidCreateShadowNode(
-      const ShadowNode::Shared &shadowNode) = 0;
+  virtual void uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) = 0;
+
+  /*
+   * Called each time when UIManager clones a Shadow Node. Receiver
+   * might use this to optimistically allocate a new native view
+   * instances.
+   */
+  virtual void uiManagerDidCloneShadowNode(
+      const ShadowNode &oldShadowNode,
+      const ShadowNode &newShadowNode) = 0;
 
   /*
    * Called when UIManager wants to dispatch a command to the mounting layer.