From c391c73710943b40e4823e32efb01ba625e8667c Mon Sep 17 00:00:00 2001 From: Fabio Carballo Date: Wed, 11 Dec 2024 02:38:32 -0800 Subject: [PATCH] Add debugger class that tracks animatedState lifecycles Summary: # Context We are trying to resolve the crash associated to this task (T209041862) which happens due to an inconsistency between `initialStatesToRestore` and `animatedStates` in Litho's `TransitionManager`. Concretely, there is a missing `animatedState` for a given entry on `initialStatesToRestore`. # This diff In this diff we introduce this `AnimationInconsistencyDebugger`, which tracks whenever an `AnimatedState` is added or removed from `animatedStates`. Then on the custom exception we collect this data, so that we can try to understand when the missing `animatedState` was removed (if ever was removed). Below there is a high level overview of the points I identify that we add or remove (in red) animation states. Therefore, I believe that one of these flows is triggered somehow before the initial states are restored, and that is where the bug is (and hopefully I can identify a pattern after this is landed). {F1973763534} Reviewed By: zielinskimz Differential Revision: D67034475 fbshipit-source-id: 79ac843153d12cfa620c0329da3c3bc638533566 --- .../litho/AnimationInconsistencyDebugger.kt | 117 ++++++++++++++++++ .../com/facebook/litho/TransitionManager.java | 60 +++++++-- 2 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 litho-rendercore-transitions/src/main/java/com/facebook/litho/AnimationInconsistencyDebugger.kt diff --git a/litho-rendercore-transitions/src/main/java/com/facebook/litho/AnimationInconsistencyDebugger.kt b/litho-rendercore-transitions/src/main/java/com/facebook/litho/AnimationInconsistencyDebugger.kt new file mode 100644 index 0000000000..49557a0b9e --- /dev/null +++ b/litho-rendercore-transitions/src/main/java/com/facebook/litho/AnimationInconsistencyDebugger.kt @@ -0,0 +1,117 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.facebook.litho + +import com.facebook.litho.TransitionManager.AnimationCleanupTrigger +import com.facebook.litho.TransitionManager.AnimationState +import com.facebook.litho.TransitionManager.changeTypeToString + +/** + * This class aids to collect information about which animation states were removed and where from. + * The idea is that we can retrieve this information when the inconsistency crash happens and report + * it in a custom exception. + * + * This debugger is disabled by default and to be enabled clients should use + * [AnimationInconsistencyDebuggerConfig.isEnabled] and then get an instance of it. + */ +internal class AnimationInconsistencyDebugger private constructor() { + + private val removedInfo = LinkedHashMap() + + fun trackAnimationStateRemoved( + transitionId: TransitionId, + animationFinishTrigger: AnimationCleanupTrigger, + animationState: AnimationState? + ) { + removedInfo[transitionId] = RemovalInfo(transitionId, animationFinishTrigger, animationState) + } + + fun trackAnimationStateCreated(transitionId: TransitionId) { + removedInfo.remove(transitionId) + } + + fun reset(transitionIdMap: TransitionIdMap) { + transitionIdMap.ids().forEach { id -> + val animationState = transitionIdMap[id] + removedInfo[id] = RemovalInfo(id, AnimationCleanupTrigger.RESET, animationState) + } + } + + fun getReadableStatus(): String { + return removedInfo.entries.joinToString(separator = ",") { it.value.toString() } + } + + class RemovalInfo( + val transitionId: TransitionId, + private val animationCleanupTrigger: AnimationCleanupTrigger, + private val animationState: AnimationState? + ) { + + override fun toString(): String { + return buildString { + var currentIndent = 0 + fun indentLine(value: String) { + append(" ".repeat(currentIndent)).appendLine(value) + } + + indentLine("[") + currentIndent++ + + indentLine("transitionId=$transitionId") + indentLine("animationCleanupTrigger=$animationCleanupTrigger") + + if (animationState != null) { + indentLine("changeType=${changeTypeToString(animationState.changeType)}") + indentLine( + "shouldFinishUndeclaredAnimation=${animationState.shouldFinishUndeclaredAnimation}") + indentLine("seenInLastTransition=${animationState.seenInLastTransition}") + indentLine("hasDisappearingAnimation=${animationState.hasDisappearingAnimation}") + if (animationState.propertyStates.isNotEmpty()) { + indentLine("propertyStates:") + currentIndent++ + animationState.propertyStates.forEach { (property, state) -> + indentLine("${property.getName()}:") + currentIndent++ + indentLine("targetValue=${state.targetValue}") + indentLine("lastMountedValue=${state.lastMountedValue}") + indentLine("numPendingAnimations=${state.numPendingAnimations}") + currentIndent-- + } + } + } + indentLine("]") + } + } + } + + companion object Factory { + + @JvmName("get") + internal fun get(): AnimationInconsistencyDebugger? { + return if (AnimationInconsistencyDebuggerConfig.isEnabled) { + AnimationInconsistencyDebugger() + } else { + null + } + } + } +} + +object AnimationInconsistencyDebuggerConfig { + + @JvmField var isEnabled = false +} diff --git a/litho-rendercore-transitions/src/main/java/com/facebook/litho/TransitionManager.java b/litho-rendercore-transitions/src/main/java/com/facebook/litho/TransitionManager.java index f58568f3a9..21ca851ebd 100644 --- a/litho-rendercore-transitions/src/main/java/com/facebook/litho/TransitionManager.java +++ b/litho-rendercore-transitions/src/main/java/com/facebook/litho/TransitionManager.java @@ -97,6 +97,8 @@ */ public class TransitionManager { + @Nullable private final AnimationInconsistencyDebugger mAnimationInconsistencyDebugger; + /** * Whether a piece of content identified by a transition key is appearing, disappearing, or just * possibly changing some properties. @@ -118,7 +120,7 @@ public interface OnAnimationCompleteListener { } /** The animation state of a single property (e.g. X, Y, ALPHA) on a piece of mount content. */ - private static class PropertyState { + protected static class PropertyState { /** * The {@link AnimatedPropertyNode} for this property: it contains the current animated value @@ -144,7 +146,7 @@ private static class PropertyState { * transition key, such as whether it's appearing, disappearing, or changing, as well as * information about any animating properties on this mount content. */ - private static class AnimationState { + static class AnimationState { /** * The states for all the properties of this mount content that have an animated value (e.g. a @@ -208,6 +210,7 @@ public TransitionManager( mOnAnimationCompleteListener = onAnimationCompleteListener; mDebugTag = debugTag; mTracer = systracer; + mAnimationInconsistencyDebugger = AnimationInconsistencyDebugger.Factory.get(); } void setupTransitions( @@ -312,7 +315,8 @@ void finishUndeclaredTransitions() { final AnimationBinding animationBinding = propertyState.animation; if (animationBinding != null) { animationBinding.stop(); - mAnimationBindingListener.finishAnimation(animationBinding); + mAnimationBindingListener.finishAnimation( + animationBinding, AnimationCleanupTrigger.UNDECLARED_TRANSITIONS); } } } @@ -377,12 +381,16 @@ boolean isDisappearing(TransitionId transitionId) { /** To be called when a MountState is recycled for a new component tree. Clears all animations. */ void reset() { + if (mAnimationInconsistencyDebugger != null) { + mAnimationInconsistencyDebugger.reset(mAnimationStates); + } for (TransitionId transitionId : mAnimationStates.ids()) { final AnimationState animationState = mAnimationStates.get(transitionId); setMountContentInner(transitionId, animationState, null); clearLayoutOutputs(animationState); } mAnimationStates.clear(); + mTraceNames.clear(); // Clear these so that stopping animations below doesn't cause us to trigger any useless @@ -416,6 +424,9 @@ private void recordLayoutOutputsGroupDiff( if (animationState == null) { animationState = new AnimationState(); mAnimationStates.put(transitionId, animationState); + if (mAnimationInconsistencyDebugger != null) { + mAnimationInconsistencyDebugger.trackAnimationStateCreated(transitionId); + } } if (currentLayoutOutputsGroup == null && nextLayoutOutputsGroup == null) { @@ -786,6 +797,11 @@ public String getMessage() { .append("\n"); } + if (mAnimationInconsistencyDebugger != null) { + message.append("\n - Animation Inconsistency Debugger data: \n"); + message.append(mAnimationInconsistencyDebugger.getReadableStatus()); + } + return message.toString(); } } @@ -888,7 +904,17 @@ private void cleanupNonAnimatingAnimationStates() { } for (TransitionId transitionId : toRemove) { - mAnimationStates.remove(transitionId); + removeAnimationState(transitionId, AnimationCleanupTrigger.NON_ANIMATING_CLEANUP); + } + } + + private void trackAnimationStateRemoval( + TransitionId transitionId, + AnimationCleanupTrigger animationFinishTrigger, + @Nullable AnimationState animationState) { + if (mAnimationInconsistencyDebugger != null) { + mAnimationInconsistencyDebugger.trackAnimationStateRemoved( + transitionId, animationFinishTrigger, animationState); } } @@ -902,7 +928,7 @@ private void debugLogStartingAnimations() { // TODO(t20726089): Restore introspection of animations } - private static String changeTypeToString(int changeType) { + protected static String changeTypeToString(int changeType) { switch (changeType) { case ChangeType.APPEARED: return "APPEARED"; @@ -973,12 +999,12 @@ public void onFinish(AnimationBinding binding) { mOnAnimationCompleteListener.onAnimationUnitComplete(propertyHandle, binding.getTag()); } } - finishAnimation(binding); + finishAnimation(binding, AnimationCleanupTrigger.FINISHED_TO_CONCLUSION); } @Override public void onCanceledBeforeStart(AnimationBinding binding) { - finishAnimation(binding); + finishAnimation(binding, AnimationCleanupTrigger.CANCELED_BEFORE_START); } @Override @@ -1061,7 +1087,8 @@ private void updateAnimationStates(AnimationBinding binding) { mTempPropertyAnimations.clear(); } - private void finishAnimation(AnimationBinding binding) { + private void finishAnimation( + AnimationBinding binding, AnimationCleanupTrigger animationFinishTrigger) { final List keys = mAnimationsToPropertyHandles.remove(binding); if (keys == null) { return; @@ -1132,7 +1159,7 @@ private void finishAnimation(AnimationBinding binding) { if (mOnAnimationCompleteListener != null) { mOnAnimationCompleteListener.onAnimationComplete(transitionId); } - mAnimationStates.remove(transitionId); + removeAnimationState(transitionId, animationFinishTrigger); clearLayoutOutputs(animationState); } } @@ -1157,6 +1184,13 @@ private boolean areAllDisappearingAnimationsFinished(AnimationState animationSta } } + private void removeAnimationState( + TransitionId transitionId, AnimationCleanupTrigger animationFinishTrigger) { + AnimationState animationState = mAnimationStates.get(transitionId); + mAnimationStates.remove(transitionId); + trackAnimationStateRemoval(transitionId, animationFinishTrigger, animationState); + } + private class TransitionsResolver implements Resolver { @Override @@ -1218,4 +1252,12 @@ public boolean shouldStart(AnimationBinding binding) { return true; } } + + enum AnimationCleanupTrigger { + CANCELED_BEFORE_START, + FINISHED_TO_CONCLUSION, + NON_ANIMATING_CLEANUP, + UNDECLARED_TRANSITIONS, + RESET + } }