From 9b1f3b16b0675622b9bc663a143509f0c2d1a1b7 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Tue, 19 Jan 2021 00:26:56 -0800 Subject: [PATCH] Back out hacks to fix T83141606 Summary: Original commit changeset: 3ed8e78e31b0 Backing-out D25938851 (https://github.com/facebook/react-native/commit/69b3016171bb2f994dd4a62c34c2c4645b5a7d56) and D25935785 (https://github.com/facebook/react-native/commit/bdea479a1faa0f1f7d7c9d9162212cce94bc9720). Based on analysis documented in T83141606, I believe this issue should be fixed in JS. Additionally, this crash actually has nothing to do with (un)flattening or the differ; it is a side-effect of stale ShadowNodes being cloned, which I believe is either UB or a contract violation. Either way, it should probably be fixed either in JS, or in node cloning. So this isn't the right solution for this issue and should be reverted. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D25949569 fbshipit-source-id: 8cf1094a767da98fff4430da60d223412e029545 --- .../java/com/facebook/react/fabric/jni/Binding.cpp | 2 +- .../react/renderer/mounting/Differentiator.cpp | 11 +++++------ .../react/renderer/mounting/ShadowViewMutation.cpp | 12 ++++-------- .../react/renderer/mounting/ShadowViewMutation.h | 5 +---- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 30bffa9720e..b6161d0db09 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -553,7 +553,7 @@ void Binding::schedulerDidFinishTransaction( switch (mutationType) { case ShadowViewMutation::Create: { if (disablePreallocateViews_ || - newChildShadowView.props->revision > 1 || mutation.recreated) { + newChildShadowView.props->revision > 1) { cppCommonMountItems.push_back( CppMountItem::CreateMountItem(newChildShadowView)); } diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index b4637217086..5545674c3e2 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -678,8 +678,7 @@ static void calculateShadowViewMutationsFlattener( !newTreeNodePair.inOtherTree) { if (newTreeNodePair.isConcreteView) { mutationInstructionContainer.createMutations.push_back( - ShadowViewMutation::CreateMutation( - newTreeNodePair.shadowView, true)); + ShadowViewMutation::CreateMutation(newTreeNodePair.shadowView)); } else { mutationInstructionContainer.deleteMutations.push_back( ShadowViewMutation::DeleteMutation(newTreeNodePair.shadowView)); @@ -974,8 +973,8 @@ static void calculateShadowViewMutationsV2( parentShadowView, newChildPair.shadowView, newChildPair.mountIndex)); - createMutations.push_back(ShadowViewMutation::CreateMutation( - newChildPair.shadowView, true)); + createMutations.push_back( + ShadowViewMutation::CreateMutation(newChildPair.shadowView)); } else { removeMutations.push_back(ShadowViewMutation::RemoveMutation( parentShadowView, @@ -1209,8 +1208,8 @@ static void calculateShadowViewMutationsV2( // but not Remove if (oldChildPair.isConcreteView != newChildPair.isConcreteView) { if (newChildPair.isConcreteView) { - createMutations.push_back(ShadowViewMutation::CreateMutation( - newChildPair.shadowView, true)); + createMutations.push_back( + ShadowViewMutation::CreateMutation(newChildPair.shadowView)); } else { removeMutations.push_back(ShadowViewMutation::RemoveMutation( parentShadowView, diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp index fcc9b73d0d9..dbf4afd227c 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp @@ -10,16 +10,14 @@ namespace facebook { namespace react { -ShadowViewMutation ShadowViewMutation::CreateMutation( - ShadowView shadowView, - bool recreated) { +ShadowViewMutation ShadowViewMutation::CreateMutation(ShadowView shadowView) { return { /* .type = */ Create, /* .parentShadowView = */ {}, /* .oldChildShadowView = */ {}, /* .newChildShadowView = */ shadowView, /* .index = */ -1, - /* .recreated = */ recreated}; + }; } ShadowViewMutation ShadowViewMutation::DeleteMutation(ShadowView shadowView) { @@ -29,7 +27,6 @@ ShadowViewMutation ShadowViewMutation::DeleteMutation(ShadowView shadowView) { /* .oldChildShadowView = */ shadowView, /* .newChildShadowView = */ {}, /* .index = */ -1, - /* .recreated = */ false, }; } @@ -43,7 +40,6 @@ ShadowViewMutation ShadowViewMutation::InsertMutation( /* .oldChildShadowView = */ {}, /* .newChildShadowView = */ childShadowView, /* .index = */ index, - /* .recreated = */ false, }; } @@ -57,7 +53,7 @@ ShadowViewMutation ShadowViewMutation::RemoveMutation( /* .oldChildShadowView = */ childShadowView, /* .newChildShadowView = */ {}, /* .index = */ index, - /* .recreated = */ false}; + }; } ShadowViewMutation ShadowViewMutation::UpdateMutation( @@ -69,7 +65,7 @@ ShadowViewMutation ShadowViewMutation::UpdateMutation( /* .oldChildShadowView = */ oldChildShadowView, /* .newChildShadowView = */ newChildShadowView, /* .index = */ -1, - /* .recreated = */ false}; + }; } #if RN_DEBUG_STRING_CONVERTIBLE diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h index 1ea0adbe51f..3bd0067d5fd 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h @@ -28,9 +28,7 @@ struct ShadowViewMutation final { /* * Creates and returns an `Create` mutation. */ - static ShadowViewMutation CreateMutation( - ShadowView shadowView, - bool recreated = false); + static ShadowViewMutation CreateMutation(ShadowView shadowView); /* * Creates and returns an `Delete` mutation. @@ -71,7 +69,6 @@ struct ShadowViewMutation final { ShadowView oldChildShadowView = {}; ShadowView newChildShadowView = {}; int index = -1; - bool recreated; // for Create mutations, for platform-specific optimizations }; using ShadowViewMutationList = std::vector;