Skip to content

Commit

Permalink
Add debugger class that tracks animatedState lifecycles
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Fabio Carballo authored and facebook-github-bot committed Dec 11, 2024
1 parent 7665163 commit c391c73
Show file tree
Hide file tree
Showing 2 changed files with 168 additions and 9 deletions.
Original file line number Diff line number Diff line change
@@ -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<TransitionId, RemovalInfo>()

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<AnimationState>) {
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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -118,7 +120,7 @@ public interface OnAnimationCompleteListener<T> {
}

/** 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
Expand All @@ -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
Expand Down Expand Up @@ -208,6 +210,7 @@ public TransitionManager(
mOnAnimationCompleteListener = onAnimationCompleteListener;
mDebugTag = debugTag;
mTracer = systracer;
mAnimationInconsistencyDebugger = AnimationInconsistencyDebugger.Factory.get();
}

void setupTransitions(
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<PropertyHandle> keys = mAnimationsToPropertyHandles.remove(binding);
if (keys == null) {
return;
Expand Down Expand Up @@ -1132,7 +1159,7 @@ private void finishAnimation(AnimationBinding binding) {
if (mOnAnimationCompleteListener != null) {
mOnAnimationCompleteListener.onAnimationComplete(transitionId);
}
mAnimationStates.remove(transitionId);
removeAnimationState(transitionId, animationFinishTrigger);
clearLayoutOutputs(animationState);
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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
}
}

0 comments on commit c391c73

Please sign in to comment.