Skip to content

Commit e869f82

Browse files
fkgozalifacebook-github-bot
authored andcommitted
Fix LayoutAnimation crash from mutation sort ordering
Summary: `LayoutAnimationDriver::animationMutationsForFrame` was sorting animation-frame Update mutations together with structural mutations (Remove/Insert/Delete) from completed animations. Animation-frame Updates reference views by their pre-structural-change parentTag. When `std::stable_sort` reordered these Updates after Remove/Insert pairs that reparented a view, `StubViewTree::mutate` hit a parentTag mismatch assertion (`react_native_assert(oldStubView->parentTag == mutation.parentTag)`) and crashed with SIGABRT. The fix records the boundary between animation-frame Updates and final structural mutations, then only sorts the structural portion. This preserves the invariant that animation-frame Updates execute before any structural changes. Additionally, the mutation comparator in `utils.h` was missing an explicit `Update < Insert` ordering rule, which could cause nondeterministic sort results between Update and Insert mutations of different types. This is now handled. The test fixture is converted to `TEST_F` with a `LayoutAnimationTest` class that enables the `fixDifferentiatorParentTagForUnflattenCase` feature flag, ensuring consistent parentTag assignment across platforms. Changelog: [Internal] Differential Revision: D102696875
1 parent c20a58a commit e869f82

4 files changed

Lines changed: 60 additions & 14 deletions

File tree

packages/react-native/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ void LayoutAnimationDriver::animationMutationsForFrame(
7474
}
7575
}
7676

77+
// Record boundary: animation-frame Updates above this point carry
78+
// parentTag from before any structural changes and must execute first.
79+
auto animationUpdatesEnd = mutationsList.size();
80+
7781
// Clear out finished animations
7882
for (auto it = inflightAnimations_.begin();
7983
it != inflightAnimations_.end();) {
@@ -99,10 +103,11 @@ void LayoutAnimationDriver::animationMutationsForFrame(
99103
}
100104
}
101105

102-
// Final step: make sure that all operations execute in the proper order.
103-
// REMOVE operations with highest indices must operate first.
106+
// Sort only the final mutations from completed animations.
107+
// Animation-frame Updates must stay before structural mutations because
108+
// they reference views by their pre-structural-change parentTag.
104109
std::stable_sort(
105-
mutationsList.begin(),
110+
mutationsList.begin() + static_cast<ptrdiff_t>(animationUpdatesEnd),
106111
mutationsList.end(),
107112
&shouldFirstComeBeforeSecondMutation);
108113
}

packages/react-native/ReactCommon/react/renderer/animations/tests/LayoutAnimationTest.cpp

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626
#include <react/test_utils/MockClock.h>
2727
#include <react/test_utils/shadowTreeGeneration.h>
2828

29+
#include <react/featureflags/ReactNativeFeatureFlags.h>
30+
#include <react/featureflags/ReactNativeFeatureFlagsDefaults.h>
31+
2932
// Uncomment when random test blocks are uncommented below.
3033
// #include <algorithm>
3134
// #include <random>
@@ -34,6 +37,17 @@ MockClock::time_point MockClock::time_ = {};
3437

3538
namespace facebook::react {
3639

40+
namespace {
41+
42+
class TestFeatureFlags : public ReactNativeFeatureFlagsDefaults {
43+
public:
44+
bool fixDifferentiatorParentTagForUnflattenCase() override {
45+
return true;
46+
}
47+
};
48+
49+
} // namespace
50+
3751
static void testShadowNodeTreeLifeCycleLayoutAnimations(
3852
uint_fast32_t seed,
3953
int treeSize,
@@ -326,7 +340,18 @@ static void testShadowNodeTreeLifeCycleLayoutAnimations(
326340

327341
using namespace facebook::react;
328342

329-
TEST(
343+
class LayoutAnimationTest : public ::testing::Test {
344+
protected:
345+
void SetUp() override {
346+
ReactNativeFeatureFlags::override(std::make_unique<TestFeatureFlags>());
347+
}
348+
349+
void TearDown() override {
350+
ReactNativeFeatureFlags::dangerouslyReset();
351+
}
352+
};
353+
354+
TEST_F(
330355
LayoutAnimationTest,
331356
stableSmallerTreeFewRepeatsFewStages_NonOverlapping_2029343357) {
332357
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -341,7 +366,7 @@ TEST(
341366
/* delay_ms_between_repeats */ 2000);
342367
}
343368

344-
TEST(
369+
TEST_F(
345370
LayoutAnimationTest,
346371
stableSmallerTreeFewRepeatsFewStages_NonOverlapping_3619914559) {
347372
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -356,7 +381,7 @@ TEST(
356381
/* delay_ms_between_repeats */ 2000);
357382
}
358383

359-
TEST(
384+
TEST_F(
360385
LayoutAnimationTest,
361386
stableSmallerTreeFewRepeatsFewStages_NonOverlapping_597132284) {
362387
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -371,7 +396,7 @@ TEST(
371396
/* delay_ms_between_repeats */ 2000);
372397
}
373398

374-
TEST(
399+
TEST_F(
375400
LayoutAnimationTest,
376401
stableSmallerTreeFewRepeatsFewStages_NonOverlapping_774986518) {
377402
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -386,7 +411,7 @@ TEST(
386411
/* delay_ms_between_repeats */ 2000);
387412
}
388413

389-
TEST(
414+
TEST_F(
390415
LayoutAnimationTest,
391416
stableSmallerTreeFewRepeatsFewStages_NonOverlapping_1450614414) {
392417
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -401,7 +426,9 @@ TEST(
401426
/* delay_ms_between_repeats */ 2000);
402427
}
403428

404-
TEST(LayoutAnimationTest, stableBiggerTreeFewRepeatsFewStages_NonOverlapping) {
429+
TEST_F(
430+
LayoutAnimationTest,
431+
stableBiggerTreeFewRepeatsFewStages_NonOverlapping) {
405432
testShadowNodeTreeLifeCycleLayoutAnimations(
406433
/* seed */ 2029343357,
407434
/* size */ 512,
@@ -414,7 +441,9 @@ TEST(LayoutAnimationTest, stableBiggerTreeFewRepeatsFewStages_NonOverlapping) {
414441
/* delay_ms_between_repeats */ 2000);
415442
}
416443

417-
TEST(LayoutAnimationTest, stableBiggerTreeFewRepeatsManyStages_NonOverlapping) {
444+
TEST_F(
445+
LayoutAnimationTest,
446+
stableBiggerTreeFewRepeatsManyStages_NonOverlapping) {
418447
testShadowNodeTreeLifeCycleLayoutAnimations(
419448
/* seed */ 2029343357,
420449
/* size */ 512,
@@ -454,7 +483,7 @@ TEST(LayoutAnimationTest, stableBiggerTreeFewRepeatsManyStages_NonOverlapping) {
454483
// before the previous animation completes.
455484
//
456485

457-
TEST(
486+
TEST_F(
458487
LayoutAnimationTest,
459488
stableSmallerTreeFewRepeatsFewStages_Overlapping_2029343357) {
460489
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -472,7 +501,7 @@ TEST(
472501
/* final_animation_delay */ 10000 + 1);
473502
}
474503

475-
TEST(
504+
TEST_F(
476505
LayoutAnimationTest,
477506
stableSmallerTreeFewRepeatsFewStages_Overlapping_597132284) {
478507
testShadowNodeTreeLifeCycleLayoutAnimations(
@@ -490,7 +519,7 @@ TEST(
490519
/* final_animation_delay */ 10000 + 1);
491520
}
492521

493-
TEST(
522+
TEST_F(
494523
LayoutAnimationTest,
495524
stableSmallerTreeFewRepeatsFewStages_Overlapping_ManyConflicts_597132284) {
496525
GTEST_SKIP();
@@ -511,7 +540,7 @@ TEST(
511540
/* final_animation_delay */ 50000 + 1);
512541
}
513542

514-
TEST(
543+
TEST_F(
515544
LayoutAnimationTest,
516545
stableBiggerTreeFewRepeatsManyStages_Overlapping_ManyConflicts_2029343357) {
517546
GTEST_SKIP();

packages/react-native/ReactCommon/react/renderer/animations/tests/MutationComparatorTest.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ TEST(MutationComparatorTest, TypeOrdering) {
117117
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(create, insert));
118118
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(insert, create));
119119

120+
// Update comes before Insert
121+
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(update, insert));
122+
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(insert, update));
123+
120124
// Remove comes before Update
121125
EXPECT_TRUE(shouldFirstComeBeforeSecondMutation(remove, update));
122126
EXPECT_FALSE(shouldFirstComeBeforeSecondMutation(update, remove));

packages/react-native/ReactCommon/react/renderer/animations/utils.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,14 @@ static inline bool shouldFirstComeBeforeSecondMutation(
8888
return false;
8989
}
9090

91+
// Update comes before Insert
92+
if (lhs.type == ShadowViewMutation::Type::Update && rhs.type == ShadowViewMutation::Type::Insert) {
93+
return true;
94+
}
95+
if (rhs.type == ShadowViewMutation::Type::Update && lhs.type == ShadowViewMutation::Type::Insert) {
96+
return false;
97+
}
98+
9199
} else {
92100
// Make sure that removes on the same level are sorted - highest indices
93101
// must come first.

0 commit comments

Comments
 (0)