From b161241db2ef74d2e4bff36d4972f5f0312dcc44 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 20 May 2021 12:52:01 -0700 Subject: [PATCH] Proposed fix for ScrollView race condition between C++ state update and onScroll Summary: FlatList relies heavily on onScroll events + the measure API. In Fabric, usage of `measure` relies on C++ having an accurate view of the current scroll position of the ScrollView. We already have a mechanism for updating the scroll position in C++ using UpdateState. But, it is only used currently at the /beginning/ and /end/ of scrolling, and UpdateState is not called /during/ scrolling at all. This means that we will see a series of events like this while scrolling: ``` Scrolling begins UPDATE C++ STATE: scrollLeft = 0, scrollTop = 0 JS event: onScroll x=0, y=0 JS event: onScroll x=0, y=100 JS event: onScroll x=0, y=200 ... JS event: onScroll x=0, y=1000 UPDATE C++ STATE: scrollLeft = 0, scrollTop = 1000 ``` Notably, not many C++ state updates are queued; and the last one is queued AFTER the JS event is sent. The last JS event and UpdateState will race, which means that sometimes the C++ update will /lose/ and C++ will have an inaccurate view of the world when FlatList receives the onScroll event and calls `measure`. My proposed solution, gated behind a feature flag, is to delay /some/ onScroll events until the C++ UpdateState has made its way back to Java, and send UpdateStates more frequently. The balance here is that UpdateState is a relatively expensive operation, so we probably still want to call it /less/ than we call onScroll. This means that `measure` will still return some incorrect results but will return correct results more frequently. Win? Changelog: [Internal[ Reviewed By: mdvacca Differential Revision: D28558380 fbshipit-source-id: 11c7cd714fae67ee5a94c4413be988481413ec03 --- .../react/config/ReactFeatureFlags.java | 23 +++++-- .../react/views/scroll/ReactScrollView.java | 66 ++++++++++++++++--- .../views/scroll/ReactScrollViewManager.java | 1 + 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java index 01dce548624..de6a271bd00 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java +++ b/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java @@ -19,6 +19,11 @@ import com.facebook.proguard.annotations.DoNotStripAny; @DoNotStripAny public class ReactFeatureFlags { + /** An interface used to compute flags on demand. */ + public interface FlagProvider { + boolean get(); + } + /** * Should this application use TurboModules? If yes, then any module that inherits {@link * com.facebook.react.turbomodule.core.interfaces.TurboModule} will NOT be passed in to C++ @@ -63,11 +68,6 @@ public class ReactFeatureFlags { /** Feature flag to configure eager initialization of MapBuffer So file */ public static boolean enableEagerInitializeMapBufferSoFile = false; - /** An interface used to compute flags on demand. */ - public interface FlagProvider { - boolean get(); - } - /** Should the RuntimeExecutor call JSIExecutor::flush()? */ private static FlagProvider enableRuntimeExecutorFlushingProvider = null; @@ -96,4 +96,17 @@ public class ReactFeatureFlags { /** Enables Fabric for LogBox */ public static boolean enableFabricInLogBox = false; + + // + // ScrollView C++ UpdateState vs onScroll race fixes + // + + /* Enables a "state race condition fix" for ScrollViews StateUpdate + onScroll event emitter */ + public static boolean enableScrollViewStateEventRaceFix = false; + + /* Enables another "state race condition fix" for ScrollViews StateUpdate + onScroll event emitter. Races a StateUpdate with every onScroll event. */ + public static boolean enableScrollViewStateEventAlwaysRace = false; + + /* Configure a min scroll delta for UpdateState to be called while still actively scrolling. */ + public static int scrollViewUpdateStateMinScrollDelta = 0; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java index 21a99dfd81b..6a2273b9899 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java @@ -30,12 +30,15 @@ import com.facebook.react.bridge.ReactContext; import com.facebook.react.bridge.WritableMap; import com.facebook.react.bridge.WritableNativeMap; import com.facebook.react.common.ReactConstants; +import com.facebook.react.config.ReactFeatureFlags; import com.facebook.react.uimanager.FabricViewStateManager; import com.facebook.react.uimanager.MeasureSpecAssertions; import com.facebook.react.uimanager.PixelUtil; import com.facebook.react.uimanager.ReactClippingViewGroup; import com.facebook.react.uimanager.ReactClippingViewGroupHelper; import com.facebook.react.uimanager.ViewProps; +import com.facebook.react.uimanager.common.UIManagerType; +import com.facebook.react.uimanager.common.ViewUtil; import com.facebook.react.uimanager.events.NativeGestureUtil; import com.facebook.react.views.view.ReactViewBackgroundManager; import java.lang.reflect.Field; @@ -98,6 +101,7 @@ public class ReactScrollView extends ScrollView private int mScrollAwayPaddingTop = 0; + private boolean mWaitingForStateUpdateRoundTrip = false; private int mLastStateUpdateScrollX = -1; private int mLastStateUpdateScrollY = -1; @@ -284,10 +288,22 @@ public class ReactScrollView extends ScrollView updateClippingRect(); } - ReactScrollViewHelper.emitScrollEvent( - this, - mOnScrollDispatchHelper.getXFlingVelocity(), - mOnScrollDispatchHelper.getYFlingVelocity()); + // Another potential UpdateState vs onScroll fix: race an UpdateState with every onScroll + // TODO T91209139: if this mechanism works well, port it to HorizontalScrollView + if (ReactFeatureFlags.enableScrollViewStateEventAlwaysRace) { + updateStateOnScroll(); + } + + // TODO T91209139: if this mechanism works well, port it to HorizontalScrollView + boolean deferEvent = + ReactFeatureFlags.enableScrollViewStateEventRaceFix + && (mWaitingForStateUpdateRoundTrip || updateStateOnScroll()); + if (!deferEvent) { + ReactScrollViewHelper.emitScrollEvent( + this, + mOnScrollDispatchHelper.getXFlingVelocity(), + mOnScrollDispatchHelper.getYFlingVelocity()); + } } } @@ -997,23 +1013,57 @@ public class ReactScrollView extends ScrollView setRemoveClippedSubviews(mRemoveClippedSubviews); } + /** + * If we know we are sending a State update, we defer emitting scroll events until the State + * update makes it back to Java. When that happens, we should immediately emit the scroll event. + */ + void onStateUpdate() { + mWaitingForStateUpdateRoundTrip = false; + + // For now we don't dedupe these - we want to send an event whenever the metrics have changed + // from the perspective of C++ + if (ReactFeatureFlags.enableScrollViewStateEventRaceFix) { + ReactScrollViewHelper.emitScrollEvent( + this, + mOnScrollDispatchHelper.getXFlingVelocity(), + mOnScrollDispatchHelper.getYFlingVelocity()); + } + } + /** * Called on any stabilized onScroll change to propagate content offset value to a Shadow Node. */ - private void updateStateOnScroll(final int scrollX, final int scrollY) { + private boolean updateStateOnScroll(final int scrollX, final int scrollY) { + if (ViewUtil.getUIManagerType(getId()) == UIManagerType.DEFAULT) { + return false; + } + // Dedupe events to reduce JNI traffic if (scrollX == mLastStateUpdateScrollX && scrollY == mLastStateUpdateScrollY) { - return; + return false; + } + + // Require a certain delta if we're still scrolling + if (mActivelyScrolling) { + int MIN_DELTA_TO_UPDATE_SCROLL_STATE = ReactFeatureFlags.scrollViewUpdateStateMinScrollDelta; + int deltaX = Math.abs(mLastStateUpdateScrollX - scrollX); + int deltaY = Math.abs(mLastStateUpdateScrollY - scrollY); + if (deltaX < MIN_DELTA_TO_UPDATE_SCROLL_STATE && deltaY < MIN_DELTA_TO_UPDATE_SCROLL_STATE) { + return false; + } } mLastStateUpdateScrollX = scrollX; mLastStateUpdateScrollY = scrollY; + mWaitingForStateUpdateRoundTrip = true; forceUpdateState(); + + return true; } - private void updateStateOnScroll() { - updateStateOnScroll(getScrollX(), getScrollY()); + private boolean updateStateOnScroll() { + return updateStateOnScroll(getScrollX(), getScrollY()); } private void updateScrollAwayState(int scrollAwayPaddingTop) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java index f8add8ad32a..c7eed9b6e28 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollViewManager.java @@ -320,6 +320,7 @@ public class ReactScrollViewManager extends ViewGroupManager public Object updateState( ReactScrollView view, ReactStylesDiffMap props, @Nullable StateWrapper stateWrapper) { view.getFabricViewStateManager().setStateWrapper(stateWrapper); + view.onStateUpdate(); return null; }