From 660c69d2ea910fe5221ec59bd3116c278f56b4c5 Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Thu, 24 Aug 2023 06:10:12 -0700 Subject: [PATCH] Ship the fix for local reference overflow in Yoga (#954) Summary: Pull Request resolved: https://github.com/facebook/litho/pull/954 X-link: https://github.com/facebook/react-native/pull/39132 X-link: https://github.com/facebook/yoga/pull/1347 # Context Reviewed By: NickGerleman, astreet Differential Revision: D48607502 fbshipit-source-id: 83601ba7c1f347ac0c2e141e1d59d1f6f01a1329 --- lib/yoga/src/main/cpp/yoga/YGEnums.cpp | 2 - lib/yoga/src/main/cpp/yoga/YGEnums.h | 3 +- .../yoga/YogaExperimentalFeature.java | 4 +- lib/yogajni/src/main/cpp/jni/YGJNIVanilla.cpp | 39 ++++++++----------- .../java/com/facebook/litho/NodeConfig.kt | 10 +---- .../litho/config/ComponentsConfiguration.java | 2 - 6 files changed, 19 insertions(+), 41 deletions(-) diff --git a/lib/yoga/src/main/cpp/yoga/YGEnums.cpp b/lib/yoga/src/main/cpp/yoga/YGEnums.cpp index e8ace4b38ef..f7220eff720 100644 --- a/lib/yoga/src/main/cpp/yoga/YGEnums.cpp +++ b/lib/yoga/src/main/cpp/yoga/YGEnums.cpp @@ -107,8 +107,6 @@ const char* YGExperimentalFeatureToString(const YGExperimentalFeature value) { return "web-flex-basis"; case YGExperimentalFeatureAbsolutePercentageAgainstPaddingEdge: return "absolute-percentage-against-padding-edge"; - case YGExperimentalFeatureFixJNILocalRefOverflows: - return "fix-jnilocal-ref-overflows"; } return "unknown"; } diff --git a/lib/yoga/src/main/cpp/yoga/YGEnums.h b/lib/yoga/src/main/cpp/yoga/YGEnums.h index a502d39b161..7abe5d92ecb 100644 --- a/lib/yoga/src/main/cpp/yoga/YGEnums.h +++ b/lib/yoga/src/main/cpp/yoga/YGEnums.h @@ -65,8 +65,7 @@ YG_DEFINE_ENUM_FLAG_OPERATORS(YGErrata) YG_ENUM_SEQ_DECL( YGExperimentalFeature, YGExperimentalFeatureWebFlexBasis, - YGExperimentalFeatureAbsolutePercentageAgainstPaddingEdge, - YGExperimentalFeatureFixJNILocalRefOverflows) + YGExperimentalFeatureAbsolutePercentageAgainstPaddingEdge) YG_ENUM_SEQ_DECL( YGFlexDirection, diff --git a/lib/yoga/src/main/java/com/facebook/yoga/YogaExperimentalFeature.java b/lib/yoga/src/main/java/com/facebook/yoga/YogaExperimentalFeature.java index daa87bf0f83..a9e621ef5a7 100644 --- a/lib/yoga/src/main/java/com/facebook/yoga/YogaExperimentalFeature.java +++ b/lib/yoga/src/main/java/com/facebook/yoga/YogaExperimentalFeature.java @@ -11,8 +11,7 @@ public enum YogaExperimentalFeature { WEB_FLEX_BASIS(0), - ABSOLUTE_PERCENTAGE_AGAINST_PADDING_EDGE(1), - FIX_JNILOCAL_REF_OVERFLOWS(2); + ABSOLUTE_PERCENTAGE_AGAINST_PADDING_EDGE(1); private final int mIntValue; @@ -28,7 +27,6 @@ public static YogaExperimentalFeature fromInt(int value) { switch (value) { case 0: return WEB_FLEX_BASIS; case 1: return ABSOLUTE_PERCENTAGE_AGAINST_PADDING_EDGE; - case 2: return FIX_JNILOCAL_REF_OVERFLOWS; default: throw new IllegalArgumentException("Unknown enum value: " + value); } } diff --git a/lib/yogajni/src/main/cpp/jni/YGJNIVanilla.cpp b/lib/yogajni/src/main/cpp/jni/YGJNIVanilla.cpp index 26511ca0787..bc108b5dafd 100644 --- a/lib/yogajni/src/main/cpp/jni/YGJNIVanilla.cpp +++ b/lib/yogajni/src/main/cpp/jni/YGJNIVanilla.cpp @@ -282,8 +282,7 @@ static void YGTransferLayoutOutputsRecursive( JNIEnv* env, jobject thiz, YGNodeRef root, - void* layoutContext, - bool shouldCleanLocalRef) { + void* layoutContext) { if (!YGNodeGetHasNewLayout(root)) { return; } @@ -337,28 +336,26 @@ static void YGTransferLayoutOutputsRecursive( arr[borderStartIndex + 3] = YGNodeLayoutGetBorder(root, YGEdgeBottom); } - // Don't change this field name without changing the name of the field in - // Database.java - auto objectClass = facebook::yoga::vanillajni::make_local_ref( - env, env->GetObjectClass(obj.get())); - static const jfieldID arrField = facebook::yoga::vanillajni::getFieldId( - env, objectClass.get(), "arr", "[F"); - - ScopedLocalRef arrFinal = - make_local_ref(env, env->NewFloatArray(arrSize)); - env->SetFloatArrayRegion(arrFinal.get(), 0, arrSize, arr); - env->SetObjectField(obj.get(), arrField, arrFinal.get()); - - if (shouldCleanLocalRef) { - objectClass.reset(); - arrFinal.reset(); + // Create scope to make sure to release any local refs created here + { + // Don't change this field name without changing the name of the field in + // Database.java + auto objectClass = facebook::yoga::vanillajni::make_local_ref( + env, env->GetObjectClass(obj.get())); + static const jfieldID arrField = facebook::yoga::vanillajni::getFieldId( + env, objectClass.get(), "arr", "[F"); + + ScopedLocalRef arrFinal = + make_local_ref(env, env->NewFloatArray(arrSize)); + env->SetFloatArrayRegion(arrFinal.get(), 0, arrSize, arr); + env->SetObjectField(obj.get(), arrField, arrFinal.get()); } YGNodeSetHasNewLayout(root, false); for (uint32_t i = 0; i < YGNodeGetChildCount(root); i++) { YGTransferLayoutOutputsRecursive( - env, thiz, YGNodeGetChild(root, i), layoutContext, shouldCleanLocalRef); + env, thiz, YGNodeGetChild(root, i), layoutContext); } } @@ -380,17 +377,13 @@ static void jni_YGNodeCalculateLayoutJNI( } const YGNodeRef root = _jlong2YGNodeRef(nativePointer); - const bool shouldCleanLocalRef = - root->getConfig()->isExperimentalFeatureEnabled( - YGExperimentalFeatureFixJNILocalRefOverflows); YGNodeCalculateLayoutWithContext( root, static_cast(width), static_cast(height), YGNodeStyleGetDirection(_jlong2YGNodeRef(nativePointer)), layoutContext); - YGTransferLayoutOutputsRecursive( - env, obj, root, layoutContext, shouldCleanLocalRef); + YGTransferLayoutOutputsRecursive(env, obj, root, layoutContext); } catch (const YogaJniException& jniException) { ScopedLocalRef throwable = jniException.getThrowable(); if (throwable.get()) { diff --git a/litho-core/src/main/java/com/facebook/litho/NodeConfig.kt b/litho-core/src/main/java/com/facebook/litho/NodeConfig.kt index 51267ab33ee..68c6033ab67 100644 --- a/litho-core/src/main/java/com/facebook/litho/NodeConfig.kt +++ b/litho-core/src/main/java/com/facebook/litho/NodeConfig.kt @@ -16,10 +16,8 @@ package com.facebook.litho -import com.facebook.litho.config.ComponentsConfiguration import com.facebook.litho.yoga.LithoYogaFactory import com.facebook.yoga.YogaConfig -import com.facebook.yoga.YogaExperimentalFeature import com.facebook.yoga.YogaNode import kotlin.jvm.JvmField @@ -33,13 +31,7 @@ object NodeConfig { @JvmField @Volatile var yogaNodeFactory: InternalYogaNodeFactory? = null /** Allows access to the internal YogaConfig instance */ - @get:JvmStatic - val yogaConfig: YogaConfig = - LithoYogaFactory.createYogaConfig().apply { - if (ComponentsConfiguration.enableFixForJniLocalRefOverflow) { - setExperimentalFeatureEnabled(YogaExperimentalFeature.FIX_JNILOCAL_REF_OVERFLOWS, true) - } - } + @get:JvmStatic val yogaConfig: YogaConfig = LithoYogaFactory.createYogaConfig() @JvmStatic fun createYogaNode(): YogaNode { diff --git a/litho-core/src/main/java/com/facebook/litho/config/ComponentsConfiguration.java b/litho-core/src/main/java/com/facebook/litho/config/ComponentsConfiguration.java index ea8a7d9a152..9bb7d593a98 100644 --- a/litho-core/src/main/java/com/facebook/litho/config/ComponentsConfiguration.java +++ b/litho-core/src/main/java/com/facebook/litho/config/ComponentsConfiguration.java @@ -236,8 +236,6 @@ public static boolean isSplitResolveAndLayoutWithSplitHandlers() { public static boolean enableLayoutCaching = false; - public static boolean enableFixForJniLocalRefOverflow = false; - public static int recyclerBinderStrategy = 0; public static boolean enableMountableRecycler = false;