From bb5d04366ad66273fd908586a9f4bc7513b4276c Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 4 May 2020 21:25:56 -0700 Subject: [PATCH] Differ: fix TinyMap to prevent possible crashes in `find()` and `begin()`, 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 --- .../fabric/mounting/Differentiator.cpp | 22 ++++++++--- .../fabric/mounting/tests/MountingTest.cpp | 38 +++++++++++++++++++ .../tests/ShadowTreeLifeCycleTest.cpp | 4 +- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/ReactCommon/fabric/mounting/Differentiator.cpp b/ReactCommon/fabric/mounting/Differentiator.cpp index 9f9f60207dc..6661d542440 100644 --- a/ReactCommon/fabric/mounting/Differentiator.cpp +++ b/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; } diff --git a/ReactCommon/fabric/mounting/tests/MountingTest.cpp b/ReactCommon/fabric/mounting/tests/MountingTest.cpp index 4783516bf1c..52361e23dc5 100644 --- a/ReactCommon/fabric/mounting/tests/MountingTest.cpp +++ b/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{ childB, childA, childD, childF, childE, childC})}); + auto shadowNodeV7 = shadowNodeV6->clone(ShadowNodeFragment{ + generateDefaultProps(viewComponentDescriptor), + std::make_shared(SharedShadowNodeList{childF, + childE, + childC, + childD, + childG, + childH, + childI, + childJ, + childK})}); // Injecting a tree into the root node. auto rootNodeV1 = std::static_pointer_cast( @@ -139,6 +155,11 @@ TEST(MountingTest, testMinimalInstructionGeneration) { ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(), std::make_shared( SharedShadowNodeList{shadowNodeV6})})); + auto rootNodeV7 = std::static_pointer_cast( + rootNodeV6->ShadowNode::clone( + ShadowNodeFragment{ShadowNodeFragment::propsPlaceholder(), + std::make_shared( + SharedShadowNodeList{shadowNodeV7})})); // Layout and diff std::vector 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 diff --git a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp b/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp index d065a696663..ab2ad89b990 100644 --- a/ReactCommon/fabric/mounting/tests/ShadowTreeLifeCycleTest.cpp +++ b/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);