From 774dec1e17f6f250172a6d4d944121b82fa36efb Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Wed, 5 Aug 2020 01:42:19 -0700 Subject: [PATCH] Introduce general API for setting C++ State from the View layer and getting a notification if it fails, with Android impl Summary: iOS will need to be implemented separately, but the shared C++ bits are in place. Explanation: there is currently no way for the View layer to /know/ if an UpdateState call has succeeded or failed. Generally we just assume it succeeds, but if it fails we have no way of knowing or retrying. This can cause some UI bugs. To mitigate this, I'm introducing a "failure" notification callback mechanism. The JNI bridging for this is a little complicated to avoid passing Runnable across the JNI, but it should be much simpler on iOS. In development this seems to make View components much more reliable. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D22940187 fbshipit-source-id: 917f2932ae22d421f91fe8f4fca3f07dc089f820 --- .../react/fabric/StateWrapperImpl.java | 32 +++++++ .../react/fabric/jni/NodeStateWrapper.cpp | 44 --------- .../react/fabric/jni/NodeStateWrapper.h | 36 -------- .../react/fabric/jni/StateWrapperImpl.cpp | 29 +++++- .../react/fabric/jni/StateWrapperImpl.h | 6 ++ .../uimanager/FabricStateBaseViewGroup.java | 73 --------------- .../uimanager/FabricViewStateManager.java | 90 +++++++++++++++++++ .../react/uimanager/StateWrapper.java | 6 ++ .../react/renderer/core/ConcreteState.h | 14 ++- ReactCommon/react/renderer/core/State.h | 4 +- ReactCommon/react/renderer/core/StateUpdate.h | 2 + .../react/renderer/uimanager/UIManager.cpp | 36 ++++---- 12 files changed, 197 insertions(+), 175 deletions(-) delete mode 100644 ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.cpp delete mode 100644 ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.h delete mode 100644 ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricStateBaseViewGroup.java create mode 100644 ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricViewStateManager.java diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/StateWrapperImpl.java b/ReactAndroid/src/main/java/com/facebook/react/fabric/StateWrapperImpl.java index 2d96776f882..a40bcaca7ba 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/StateWrapperImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/StateWrapperImpl.java @@ -8,11 +8,13 @@ package com.facebook.react.fabric; import android.annotation.SuppressLint; +import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import com.facebook.jni.HybridData; import com.facebook.proguard.annotations.DoNotStrip; import com.facebook.react.bridge.NativeMap; import com.facebook.react.bridge.ReadableNativeMap; +import com.facebook.react.bridge.UiThreadUtil; import com.facebook.react.bridge.WritableMap; import com.facebook.react.uimanager.StateWrapper; @@ -28,6 +30,9 @@ public class StateWrapperImpl implements StateWrapper { @DoNotStrip private final HybridData mHybridData; + private Runnable mFailureCallback = null; + private int mUpdateStateId = 0; + private static native HybridData initHybrid(); private StateWrapperImpl() { @@ -39,8 +44,35 @@ public class StateWrapperImpl implements StateWrapper { public native void updateStateImpl(@NonNull NativeMap map); + public native void updateStateWithFailureCallbackImpl( + @NonNull NativeMap map, Object self, int updateStateId); + @Override public void updateState(@NonNull WritableMap map) { + mUpdateStateId++; updateStateImpl((NativeMap) map); } + + @Override + public void updateState(@NonNull WritableMap map, Runnable failureCallback) { + mUpdateStateId++; + mFailureCallback = failureCallback; + updateStateWithFailureCallbackImpl((NativeMap) map, this, mUpdateStateId); + } + + @DoNotStrip + @AnyThread + public void updateStateFailed(int callbackRefId) { + // If the callback ref ID doesn't match the ID of the most-recent updateState call, + // then it's an outdated failure callback and we ignore it. + if (callbackRefId != mUpdateStateId) { + return; + } + + final Runnable failureCallback = mFailureCallback; + mFailureCallback = null; + if (failureCallback != null) { + UiThreadUtil.runOnUiThread(failureCallback); + } + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.cpp deleted file mode 100644 index 40bf0c392d6..00000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.cpp +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#include "NodeStateWrapper.h" -#include -#include - -using namespace facebook::jni; - -namespace facebook { -namespace react { - -jni::local_ref NodeStateWrapper::initHybrid( - jni::alias_ref) { - return makeCxxInstance(); -} - -jni::local_ref NodeStateWrapper::getState() { - folly::dynamic map = state_->getDynamic(); - local_ref readableNativeMap = - ReadableNativeMap::newObjectCxxArgs(map); - return readableNativeMap; -} - -void NodeStateWrapper::updateState(ReadableNativeMap *map) { - // Get folly::dynamic from map - auto dynamicMap = map->consume(); - // Set state - state_->updateState(dynamicMap); -} - -void NodeStateWrapper::registerNatives() { - registerHybrid({ - makeNativeMethod("getState", NodeStateWrapper::getState), - makeNativeMethod("updateState", NodeStateWrapper::updateState), - }); -} - -} // namespace react -} // namespace facebook diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.h deleted file mode 100644 index 672b8e95b16..00000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/NodeStateWrapper.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include -#include -#include - -namespace facebook { -namespace react { - -class NodeStateWrapper : public jni::HybridClass { - public: - constexpr static const char *const kJavaDescriptor = - "Lcom/facebook/react/fabric/NodeStateWrapper;"; - - NodeStateWrapper() {} - - static void registerNatives(); - - jni::local_ref getState(); - void updateState(ReadableNativeMap *map); - - const State *state_; - - private: - static jni::local_ref initHybrid(jni::alias_ref); -}; - -} // namespace react -} // namespace facebook diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.cpp index 9a5b929e9af..afbe9c71552 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.cpp @@ -33,7 +33,31 @@ void StateWrapperImpl::updateStateImpl(NativeMap *map) { // Get folly::dynamic from map auto dynamicMap = map->consume(); // Set state - state_->updateState(dynamicMap); + state_->updateState(dynamicMap, nullptr); +} + +void StateWrapperImpl::updateStateWithFailureCallbackImpl( + NativeMap *map, + jni::alias_ref self, + int callbackRefId) { + // Get folly::dynamic from map + auto dynamicMap = map->consume(); + // Turn the alias into a global_ref + // Note: this whole thing feels really janky, making StateWrapperImpl.java + // pass "this" into a function it's calling on "this". But after struggling + // for a while I couldn't figure out how to get a reference to the Java side + // of "this" in C++ in a way that's reasonably safe, and it maybe is even + // discouraged. Anyway, it might be weird, but this seems to work and be safe. + jni::global_ref globalSelf = make_global(self); + // Set state + state_->updateState( + dynamicMap, [globalSelf = std::move(globalSelf), callbackRefId]() { + static auto method = + jni::findClassStatic( + StateWrapperImpl::StateWrapperImplJavaDescriptor) + ->getMethod("updateStateFailed"); + method(globalSelf, callbackRefId); + }); } void StateWrapperImpl::registerNatives() { @@ -41,6 +65,9 @@ void StateWrapperImpl::registerNatives() { makeNativeMethod("initHybrid", StateWrapperImpl::initHybrid), makeNativeMethod("getState", StateWrapperImpl::getState), makeNativeMethod("updateStateImpl", StateWrapperImpl::updateStateImpl), + makeNativeMethod( + "updateStateWithFailureCallbackImpl", + StateWrapperImpl::updateStateWithFailureCallbackImpl), }); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h index 056a11738ea..9a88eb0271b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/StateWrapperImpl.h @@ -20,11 +20,17 @@ class StateWrapperImpl : public jni::HybridClass { public: constexpr static const char *const kJavaDescriptor = "Lcom/facebook/react/fabric/StateWrapperImpl;"; + constexpr static auto StateWrapperImplJavaDescriptor = + "com/facebook/react/fabric/StateWrapperImpl"; static void registerNatives(); jni::local_ref getState(); void updateStateImpl(NativeMap *map); + void updateStateWithFailureCallbackImpl( + NativeMap *map, + jni::alias_ref self, + int callbackRefId); State::Shared state_; diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricStateBaseViewGroup.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricStateBaseViewGroup.java deleted file mode 100644 index af24da786bc..00000000000 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricStateBaseViewGroup.java +++ /dev/null @@ -1,73 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -package com.facebook.react.uimanager; - -import android.view.ViewGroup; -import com.facebook.react.bridge.UiThreadUtil; -import com.facebook.react.bridge.WritableMap; -import com.facebook.react.config.ReactFeatureFlags; - -/** This is a helper base class for ViewGroups that use Fabric State. */ -public abstract class FabricStateBaseViewGroup extends ViewGroup { - public interface StateUpdateCallback { - WritableMap getStateUpdate(); - } - - private StateWrapper mStateWrapper = null; - - public FabricStateBaseViewGroup(ThemedReactContext context) { - super(context); - } - - public void setStateWrapper(StateWrapper stateWrapper) { - mStateWrapper = stateWrapper; - } - - private static void setState( - final FabricStateBaseViewGroup view, - final StateWrapper stateWrapper, - final StateUpdateCallback stateUpdateCallback, - final int numTries) { - // The StateWrapper will change, breaking this loop, whenever the UpdateState MountItem - // is executed. - // The caller is responsible for detecting if data is up-to-date, and doing nothing, or - // detecting if state is stale and calling setState again. - if (stateWrapper != view.mStateWrapper) { - return; - } - // We arbitrarily bail out after a certain number of retries. - // This is a pretty large number: in practice I've seen this number go over 50 - // with minimal/no visual jank. - if (numTries > 5 * 60) { - return; - } - - stateWrapper.updateState(stateUpdateCallback.getStateUpdate()); - - // An `updateState` call can fail, and there's no way to verify if it succeeds besides - // waiting for a corresponding `StateUpdate` MountItem to be executed on some future UI tick. - // So.... to resolve conflicts with updateState, we just keep firing it until it succeeds or - // the View goes away. - if (ReactFeatureFlags.enableExperimentalStateUpdateRetry) { - UiThreadUtil.runOnUiThread( - new Runnable() { - @Override - public void run() { - setState(view, stateWrapper, stateUpdateCallback, numTries + 1); - } - }); - } - } - - public static void setState( - final FabricStateBaseViewGroup view, - final StateWrapper stateWrapper, - final StateUpdateCallback stateUpdateCallback) { - setState(view, stateWrapper, stateUpdateCallback, 0); - } -} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricViewStateManager.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricViewStateManager.java new file mode 100644 index 00000000000..454fd2bbb7d --- /dev/null +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/FabricViewStateManager.java @@ -0,0 +1,90 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.uimanager; + +import androidx.annotation.Nullable; +import com.facebook.common.logging.FLog; +import com.facebook.react.bridge.WritableMap; +import com.facebook.react.config.ReactFeatureFlags; + +/** + * This is a helper base class for ViewGroups that use Fabric State. + * + *

Reason to use this: UpdateState calls from the View layer to the Fabric core can fail, and + * optionally Fabric will call a "failure callback" if that happens. This class abstracts that and + * makes it easier ensure that State in Fabric is always up-to-date. + * + *

1. Whenever ViewManager.updateState is called, call View.setStateWrapper. 2. Instead of + * calling StateWrapper.updateState directly, call View.setState and it will automatically keep + * retrying the UpdateState call until it succeeds; or you call setState again; or the View layer is + * updated with a newer StateWrapper. + */ +public class FabricViewStateManager { + private static final String TAG = "FabricViewStateManager"; + + public interface HasFabricViewStateManager { + FabricViewStateManager getFabricViewStateManager(); + } + + public interface StateUpdateCallback { + WritableMap getStateUpdate(); + } + + @Nullable private StateWrapper mStateWrapper = null; + + public void setStateWrapper(StateWrapper stateWrapper) { + mStateWrapper = stateWrapper; + } + + public boolean hasStateWrapper() { + return mStateWrapper != null; + } + + private void setState( + @Nullable final StateWrapper stateWrapper, + final StateUpdateCallback stateUpdateCallback, + final int numTries) { + // The StateWrapper will change, breaking the async loop, whenever the UpdateState MountItem + // is executed. + // The caller is responsible for detecting if data is up-to-date, and doing nothing, or + // detecting if state is stale and calling setState again. + if (stateWrapper == null) { + FLog.e(TAG, "setState called without a StateWrapper"); + return; + } + if (stateWrapper != mStateWrapper) { + return; + } + // We bail out after an arbitrary number of tries. In practice this should never go higher + // than 2 or 3, but there's nothing guaranteeing that. + if (numTries > 60) { + return; + } + + Runnable failureRunnable = null; + if (ReactFeatureFlags.enableExperimentalStateUpdateRetry) { + failureRunnable = + new Runnable() { + @Override + // Run on the UI thread + public void run() { + FLog.e(TAG, "UpdateState failed - retrying! " + numTries); + setState(stateWrapper, stateUpdateCallback, numTries + 1); + } + }; + } + stateWrapper.updateState( + stateUpdateCallback.getStateUpdate(), + // Failure callback - this is run if the updateState call fails + failureRunnable); + } + + public void setState(final StateUpdateCallback stateUpdateCallback) { + setState(mStateWrapper, stateUpdateCallback, 0); + } +} diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java index 6ac419a62d9..e56a16cd50f 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/uimanager/StateWrapper.java @@ -23,4 +23,10 @@ public interface StateWrapper { /** Pass a map of values back to the C++ layer. */ void updateState(WritableMap map); + + /** + * Pass a map of values back to the C++ layer. /Last/ runnable passed into updateState is called + * if an updateState call fails. + */ + void updateState(WritableMap map, Runnable failureCallback); } diff --git a/ReactCommon/react/renderer/core/ConcreteState.h b/ReactCommon/react/renderer/core/ConcreteState.h index 6b8c73cbc35..c57a6518bc6 100644 --- a/ReactCommon/react/renderer/core/ConcreteState.h +++ b/ReactCommon/react/renderer/core/ConcreteState.h @@ -60,11 +60,13 @@ class ConcreteState : public State { */ void updateState( Data &&newData, + std::function failureCallback = nullptr, EventPriority priority = EventPriority::AsynchronousUnbatched) const { updateState( [data = std::move(newData)](Data const &oldData) mutable -> Data && { return std::move(data); }, + failureCallback, priority); } @@ -78,6 +80,7 @@ class ConcreteState : public State { */ void updateState( std::function callback, + std::function failureCallback = nullptr, EventPriority priority = EventPriority::AsynchronousBatched) const { auto family = family_.lock(); @@ -88,11 +91,13 @@ class ConcreteState : public State { } auto stateUpdate = StateUpdate{ - family, [=](StateData::Shared const &oldData) -> StateData::Shared { + family, + [=](StateData::Shared const &oldData) -> StateData::Shared { assert(oldData); return std::make_shared( callback(*std::static_pointer_cast(oldData))); - }}; + }, + failureCallback}; family->dispatchRawState(std::move(stateUpdate), priority); } @@ -102,8 +107,9 @@ class ConcreteState : public State { return getData().getDynamic(); } - void updateState(folly::dynamic data) const override { - updateState(std::move(Data(getData(), data))); + void updateState(folly::dynamic data, std::function failureCallback) + const override { + updateState(std::move(Data(getData(), data)), failureCallback); } #endif }; diff --git a/ReactCommon/react/renderer/core/State.h b/ReactCommon/react/renderer/core/State.h index c0472fec057..ae636f37e42 100644 --- a/ReactCommon/react/renderer/core/State.h +++ b/ReactCommon/react/renderer/core/State.h @@ -65,7 +65,9 @@ class State { #ifdef ANDROID virtual folly::dynamic getDynamic() const = 0; - virtual void updateState(folly::dynamic data) const = 0; + virtual void updateState( + folly::dynamic data, + std::function failureCallback) const = 0; #endif protected: diff --git a/ReactCommon/react/renderer/core/StateUpdate.h b/ReactCommon/react/renderer/core/StateUpdate.h index 5e52cdc575a..a77abb90dc6 100644 --- a/ReactCommon/react/renderer/core/StateUpdate.h +++ b/ReactCommon/react/renderer/core/StateUpdate.h @@ -21,9 +21,11 @@ class StateUpdate { public: using Callback = std::function; + using FailureCallback = std::function; SharedShadowNodeFamily family; Callback callback; + FailureCallback failureCallback; }; } // namespace react diff --git a/ReactCommon/react/renderer/uimanager/UIManager.cpp b/ReactCommon/react/renderer/uimanager/UIManager.cpp index 980de3b5414..91827e3fb94 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManager.cpp @@ -238,23 +238,27 @@ void UIManager::updateState(StateUpdate const &stateUpdate) const { shadowTreeRegistry_.visit( family->getSurfaceId(), [&](ShadowTree const &shadowTree) { - shadowTree.tryCommit([&](RootShadowNode::Shared const - &oldRootShadowNode) { - return std::static_pointer_cast< - RootShadowNode>(oldRootShadowNode->cloneTree( - *family, [&](ShadowNode const &oldShadowNode) { - auto newData = - callback(oldShadowNode.getState()->getDataPointer()); - auto newState = - componentDescriptor.createState(*family, newData); + bool updateSucceeded = shadowTree.tryCommit( + [&](RootShadowNode::Shared const &oldRootShadowNode) { + return std::static_pointer_cast< + RootShadowNode>(oldRootShadowNode->cloneTree( + *family, [&](ShadowNode const &oldShadowNode) { + auto newData = + callback(oldShadowNode.getState()->getDataPointer()); + auto newState = + componentDescriptor.createState(*family, newData); - return oldShadowNode.clone({ - /* .props = */ ShadowNodeFragment::propsPlaceholder(), - /* .children = */ ShadowNodeFragment::childrenPlaceholder(), - /* .state = */ newState, - }); - })); - }); + return oldShadowNode.clone({ + /* .props = */ ShadowNodeFragment::propsPlaceholder(), + /* .children = */ + ShadowNodeFragment::childrenPlaceholder(), + /* .state = */ newState, + }); + })); + }); + if (!updateSucceeded && stateUpdate.failureCallback) { + stateUpdate.failureCallback(); + } }); }