Differ: fix TinyMap to prevent possible crashes in find() and `begi…

…n()`, and prevent erased elements from being iterated over

Summary:
The core issue solved in D21389371 was that erased elements of a TinyMap were being iterated over, because TinyMap has somewhat-complicated logic around cleaning out the underlying vector. In some very marginal cases, vectors were not being cleaned and an iterator pointing at erased elements was being returned.

The diff prevents some possible crashes in `begin()` and `find()` while making it much less likely to iterate over erased elements.

We also add a unit test to catch the case fixed in D21389371, in particular.

We also are keeping the code added in D21389371 (for now) since it's a cheap check, and will be a safeguard until we have rigorous testing around TinyMap. To be clear that logic should noop currently, but will prevent crashes in case guarantees around TinyMap change in the future.

Currently there is only one line of code that actually uses the TinyMap iterator, so this should be safe.

Reviewed By: shergin

Differential Revision: D21392762

fbshipit-source-id: 36dc998958c230fad01af93338974f8889cbcf55

ReactCommon/fabric/mounting/Differentiator.cpp

@@ -54,7 +54,13 @@ class TinyMap final {
     // then we don't need to clean.
     cleanVector(erasedAtFront_ != numErased_);
 
-    return begin_();
+    Iterator it = begin_();
+
+    if (it != nullptr) {
+      return it + erasedAtFront_;
+    }
+
+    return nullptr;
   }
 
   inline Iterator end() {
@@ -71,6 +77,10 @@ class TinyMap final {
 
     assert(key != 0);
 
+    if (begin_() == nullptr) {
+      return end();
+    }
+
     for (auto it = begin_() + erasedAtFront_; it != end(); it++) {
       if (it->first == key) {
         return it;
@@ -86,14 +96,14 @@ class TinyMap final {
   }
 
   inline void erase(Iterator iterator) {
-    numErased_++;
-
     // Invalidate tag.
     iterator->first = 0;
 
     if (iterator == begin_() + erasedAtFront_) {
       erasedAtFront_++;
     }
+
+    numErased_++;
   }
 
  private:
@@ -643,7 +653,7 @@ static void calculateShadowViewMutationsOptimizedMoves(
       }
 
       // At this point, oldTag is -1 or is in the new list, and hasn't been
-      // inserted or matched yet We're not sure yet if the new node is in the
+      // inserted or matched yet. We're not sure yet if the new node is in the
       // old list - generate an insert instruction for the new node.
       auto const &newChildPair = newChildPairs[newIndex];
       insertMutations.push_back(ShadowViewMutation::InsertMutation(
@@ -656,7 +666,9 @@ static void calculateShadowViewMutationsOptimizedMoves(
     for (auto it = newInsertedPairs.begin(); it != newInsertedPairs.end();
          it++) {
       // Erased elements of a TinyMap will have a Tag/key of 0 - skip those
-      // These *should* be removed by the map, but are not always.
+      // These *should* be removed by the map; there are currently no KNOWN
+      // cases where TinyMap will do the wrong thing, but there are not yet
+      // any unit tests explicitly for TinyMap, so this is safer for now.
       if (it->first == 0) {
         continue;
       }

ReactCommon/fabric/mounting/tests/MountingTest.cpp

@@ -77,6 +77,11 @@ TEST(MountingTest, testMinimalInstructionGeneration) {
   auto childD = makeNode(viewComponentDescriptor, 103, {});
   auto childE = makeNode(viewComponentDescriptor, 104, {});
   auto childF = makeNode(viewComponentDescriptor, 105, {});
+  auto childG = makeNode(viewComponentDescriptor, 106, {});
+  auto childH = makeNode(viewComponentDescriptor, 107, {});
+  auto childI = makeNode(viewComponentDescriptor, 108, {});
+  auto childJ = makeNode(viewComponentDescriptor, 109, {});
+  auto childK = makeNode(viewComponentDescriptor, 110, {});
 
   auto family = viewComponentDescriptor.createFamily(
       {10, SurfaceId(1), nullptr}, nullptr);
@@ -107,6 +112,17 @@ TEST(MountingTest, testMinimalInstructionGeneration) {
       generateDefaultProps(viewComponentDescriptor),
       std::make_shared<SharedShadowNodeList>(SharedShadowNodeList{
           childB, childA, childD, childF, childE, childC})});
+  auto shadowNodeV7 = shadowNodeV6->clone(ShadowNodeFragment{
+      generateDefaultProps(viewComponentDescriptor),
+      std::make_shared<SharedShadowNodeList>(SharedShadowNodeList{childF,
+                                                                  childE,
+                                                                  childC,
+                                                                  childD,
+                                                                  childG,
+                                                                  childH,
+                                                                  childI,
+                                                                  childJ,
+                                                                  childK})});
 
   // Injecting a tree into the root node.
   auto rootNodeV1 = std::static_pointer_cast<RootShadowNode const>(
@@ -139,6 +155,11 @@ TEST(MountingTest, testMinimalInstructionGeneration) {
           ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(),
                              std::make_shared<SharedShadowNodeList>(
                                  SharedShadowNodeList{shadowNodeV6})}));
+  auto rootNodeV7 = std::static_pointer_cast<RootShadowNode const>(
+      rootNodeV6->ShadowNode::clone(
+          ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(),
+                             std::make_shared<SharedShadowNodeList>(
+                                 SharedShadowNodeList{shadowNodeV7})}));
 
   // Layout and diff
   std::vector<LayoutableShadowNode const *> affectedLayoutableNodesV1{};
@@ -307,6 +328,23 @@ TEST(MountingTest, testMinimalInstructionGeneration) {
   assert(mutations5[3].type == ShadowViewMutation::Insert);
   assert(mutations5[3].newChildShadowView.tag == 105);
   assert(mutations5[3].index == 3);
+
+  auto mutations6 = calculateShadowViewMutations(
+      DifferentiatorMode::OptimizedMoves, *rootNodeV6, *rootNodeV7);
+
+  // The order and exact mutation instructions here may change at any time.
+  // This test just ensures that any changes are intentional.
+  // This test, in particular, ensures that a bug has been fixed: that with
+  // a particular sequence of inserts/removes/moves, we don't unintentionally
+  // create more "CREATE" mutations than necessary.
+  // The actual nodes that should be created in this transaction have a tag >
+  // 105.
+  assert(mutations6.size() == 25);
+  for (int i = 0; i < mutations6.size(); i++) {
+    if (mutations6[i].type == ShadowViewMutation::Create) {
+      assert(mutations6[i].newChildShadowView.tag > 105);
+    }
+  }
 }
 
 } // namespace react

ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp

@@ -169,7 +169,7 @@ TEST(MountingTest, stableSmallerTreeMoreIterationsClassic) {
 TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) {
   testShadowNodeTreeLifeCycle(
       DifferentiatorMode::OptimizedMoves,
-      /* seed */ 1,
+      /* seed */ 0,
       /* size */ 512,
       /* repeats */ 32,
       /* stages */ 32);
@@ -178,7 +178,7 @@ TEST(MountingTest, stableBiggerTreeFewerIterationsOptimizedMoves) {
 TEST(MountingTest, stableSmallerTreeMoreIterationsOptimizedMoves) {
   testShadowNodeTreeLifeCycle(
       DifferentiatorMode::OptimizedMoves,
-      /* seed */ 1,
+      /* seed */ 0,
       /* size */ 16,
       /* repeats */ 512,
       /* stages */ 32);