From 3fde61f87c66828dce9eafeef6bb135c160242ea Mon Sep 17 00:00:00 2001 From: Lijin Shen Date: Wed, 18 Jan 2023 22:08:49 +0000 Subject: [PATCH] [M110] Fix hiding animation crash Merge of https://chromium-review.googlesource.com/c/chromium/src/+/4134744 https://chromium-review.googlesource.com/c/chromium/src/+/4116873 Bug: 1382275 Change-Id: Ie1b99c551800a741cb0ee69c8dbbec5448a96948 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4178556 Commit-Queue: Lijin Shen Reviewed-by: Aishwarya Rajesh Cr-Commit-Position: refs/branch-heads/5481@{#437} Cr-Branched-From: 130f3e4d850f4bc7387cfb8d08aa993d288a67a9-refs/heads/main@{#1084008} --- .../messages/MessageAnimationCoordinator.java | 27 ++++- .../MessageAnimationCoordinatorUnitTest.java | 103 +++++++++++++++++- .../components/messages/MessageContainer.java | 7 ++ 3 files changed, 128 insertions(+), 9 deletions(-) diff --git a/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinator.java b/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinator.java index d1919b002e151..e0f252f92a948 100644 --- a/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinator.java +++ b/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinator.java @@ -265,10 +265,14 @@ public void updateWithStacking( if (currentFront == null) { // No message is being displayed now: trigger #onStartShowing. mCurrentDisplayedMessages = new ArrayList<>(candidates); + // Use ref because when startShowing is finished, other animation might have been + // triggered such that those two member variables have been mutated. + var frontAnimator = mFrontAnimator; + var backAnimator = mBackAnimator; mMessageQueueDelegate.onStartShowing(() -> { if (candidates.get(0) == mCurrentDisplayedMessages.get(0) && candidates.get(1) == mCurrentDisplayedMessages.get(1)) { - triggerStackingAnimation(candidates, onFinished); + triggerStackingAnimation(candidates, onFinished, frontAnimator, backAnimator); } }); } else if (nextFront == null) { @@ -278,20 +282,27 @@ public void updateWithStacking( mCurrentDisplayedMessages = new ArrayList<>(candidates); onFinished.run(); }; - triggerStackingAnimation(candidates, runnable); + triggerStackingAnimation(candidates, runnable, mFrontAnimator, mBackAnimator); } else { mCurrentDisplayedMessages = new ArrayList<>(candidates); - triggerStackingAnimation(candidates, onFinished); + triggerStackingAnimation(candidates, onFinished, mFrontAnimator, mBackAnimator); } } - private void triggerStackingAnimation(List candidates, Runnable onFinished) { + private void triggerStackingAnimation(List candidates, Runnable onFinished, + Animator frontAnimator, Animator backAnimator) { Runnable runnable = () -> { + // While the runnable is waiting to be triggered, hiding animation might be triggered: + // while the hiding animation is running, declare this runnable as obsolete so that + // it won't cancel the hiding animation. + if (isAnimatorExpired(frontAnimator, backAnimator)) { + return; + } mAnimatorSet.cancel(); mAnimatorSet.removeAllListeners(); mAnimatorSet = new AnimatorSet(); - mAnimatorSet.play(mFrontAnimator); - mAnimatorSet.play(mBackAnimator); + mAnimatorSet.play(frontAnimator); + mAnimatorSet.play(backAnimator); mAnimatorSet.addListener(new MessageAnimationListener(() -> { mMessageQueueDelegate.onAnimationEnd(); onFinished.run(); @@ -306,6 +317,10 @@ private void triggerStackingAnimation(List candidates, Runnable on } } + private boolean isAnimatorExpired(Animator frontAnimator, Animator backAnimator) { + return mFrontAnimator != frontAnimator || mBackAnimator != backAnimator; + } + @Override public void onSwipeStart() { // Message shouldn't consume swipe for now because animation is running, e.g.: diff --git a/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinatorUnitTest.java b/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinatorUnitTest.java index 22995b64b5eb4..9ce423885685a 100644 --- a/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinatorUnitTest.java +++ b/components/messages/android/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinatorUnitTest.java @@ -11,7 +11,6 @@ import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyInt; import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -83,7 +82,6 @@ public void onAnimationEnd() {} @Before public void setUp() { mAnimationCoordinator = new MessageAnimationCoordinator(mContainer, Animator::start); - doNothing().when(mAnimatorStartCallback).onResult(any()); mAnimationCoordinator.setMessageQueueDelegate(mQueueDelegate); } @@ -443,6 +441,102 @@ public void onAnimationEnd() {} verify(m2.handler).hide(anyInt(), anyInt(), anyBoolean()); } + // Test showing animation is triggered after hiding animation is started. + @Test + @SmallTest + public void testObsoleteShowingAnimation() { + mAnimationCoordinator = new MessageAnimationCoordinator(mContainer, mAnimatorStartCallback); + mAnimationCoordinator.setMessageQueueDelegate(mQueueDelegate); + var currentMessages = mAnimationCoordinator.getCurrentDisplayedMessages(); + Assert.assertArrayEquals(new MessageState[] {null, null}, currentMessages.toArray()); + HistogramDelta d1 = new HistogramDelta(MessagesMetrics.STACKING_HISTOGRAM_NAME, + MessagesMetrics.StackingAnimationType.SHOW_ALL); + HistogramDelta d2 = new HistogramDelta(MessagesMetrics.STACKING_ACTION_HISTOGRAM_PREFIX + + MessagesMetrics.stackingAnimationActionToHistogramSuffix( + MessagesMetrics.StackingAnimationAction.INSERT_AT_FRONT), + 1); + HistogramDelta d3 = new HistogramDelta(MessagesMetrics.STACKING_ACTION_HISTOGRAM_PREFIX + + MessagesMetrics.stackingAnimationActionToHistogramSuffix( + MessagesMetrics.StackingAnimationAction.INSERT_AT_BACK), + 2); + MessageState m1 = buildMessageState(); + setMessageIdentifier(m1, 1); + MessageState m2 = buildMessageState(); + setMessageIdentifier(m2, 2); + ArgumentCaptor captor = ArgumentCaptor.forClass(Runnable.class); + mAnimationCoordinator.updateWithStacking(Arrays.asList(m1, m2), false, () -> {}); + verify(mContainer).runAfterInitialMessageLayout(captor.capture()); + Assert.assertEquals(1, d1.getDelta()); + Assert.assertEquals(1, d2.getDelta()); + Assert.assertEquals(1, d3.getDelta()); + verify(m1.handler).show(Position.INVISIBLE, Position.FRONT); + verify(m2.handler).show(Position.FRONT, Position.BACK); + currentMessages = mAnimationCoordinator.getCurrentDisplayedMessages(); + Assert.assertArrayEquals(new MessageState[] {m1, m2}, currentMessages.toArray()); + verify(mAnimatorStartCallback, never()).onResult(any()); + + mAnimationCoordinator.updateWithStacking(Arrays.asList(null, null), false, () -> {}); + verify(m1.handler).hide(anyInt(), anyInt(), anyBoolean()); + verify(m2.handler).hide(anyInt(), anyInt(), anyBoolean()); + verify(mQueueDelegate, times(1)).onAnimationStart(); + verify(mAnimatorStartCallback, times(1)).onResult(any()); + + // Trigger showing animation after hiding animation is started. + captor.getValue().run(); + verify(mQueueDelegate, times(1)).onAnimationStart(); + verify(mAnimatorStartCallback, times(1)).onResult(any()); + } + + // Test showing animation is triggered after hiding animation is started. + @Test + @SmallTest + public void testObsoleteShowingAnimationByOnStartedShowing() { + mAnimationCoordinator = new MessageAnimationCoordinator(mContainer, mAnimatorStartCallback); + MessageQueueDelegate queueDelegate = Mockito.mock(MessageQueueDelegate.class); + mAnimationCoordinator.setMessageQueueDelegate(queueDelegate); + var currentMessages = mAnimationCoordinator.getCurrentDisplayedMessages(); + Assert.assertArrayEquals(new MessageState[] {null, null}, currentMessages.toArray()); + HistogramDelta d1 = new HistogramDelta(MessagesMetrics.STACKING_HISTOGRAM_NAME, + + MessagesMetrics.StackingAnimationType.SHOW_ALL); + HistogramDelta d2 = new HistogramDelta(MessagesMetrics.STACKING_ACTION_HISTOGRAM_PREFIX + + MessagesMetrics.stackingAnimationActionToHistogramSuffix( + MessagesMetrics.StackingAnimationAction.INSERT_AT_FRONT), + 1); + HistogramDelta d3 = new HistogramDelta(MessagesMetrics.STACKING_ACTION_HISTOGRAM_PREFIX + + MessagesMetrics.stackingAnimationActionToHistogramSuffix( + MessagesMetrics.StackingAnimationAction.INSERT_AT_BACK), + 2); + MessageState m1 = buildMessageState(); + setMessageIdentifier(m1, 1); + MessageState m2 = buildMessageState(); + setMessageIdentifier(m2, 2); + ArgumentCaptor captor = ArgumentCaptor.forClass(Runnable.class); + mAnimationCoordinator.updateWithStacking(Arrays.asList(m1, m2), false, () -> {}); + verify(queueDelegate).onStartShowing(captor.capture()); + Assert.assertEquals(1, d1.getDelta()); + Assert.assertEquals(1, d2.getDelta()); + Assert.assertEquals(1, d3.getDelta()); + verify(m1.handler).show(Position.INVISIBLE, Position.FRONT); + verify(m2.handler).show(Position.FRONT, Position.BACK); + currentMessages = mAnimationCoordinator.getCurrentDisplayedMessages(); + Assert.assertArrayEquals(new MessageState[] {m1, m2}, currentMessages.toArray()); + verify(mAnimatorStartCallback, never()).onResult(any()); + + mAnimationCoordinator.updateWithStacking(Arrays.asList(null, null), false, () -> {}); + verify(m1.handler).hide(anyInt(), anyInt(), anyBoolean()); + verify(m2.handler).hide(anyInt(), anyInt(), anyBoolean()); + verify(queueDelegate, times(1)).onAnimationStart(); + verify(mAnimatorStartCallback, times(1)).onResult(any()); + + // Trigger showing animation after hiding animation is started. + captor.getValue().run(); + verify(mContainer).runAfterInitialMessageLayout(captor.capture()); + captor.getValue().run(); + verify(queueDelegate, times(1)).onAnimationStart(); + verify(mAnimatorStartCallback, times(1)).onResult(any()); + } + // Test when suspension cancels a hiding animation. @Test @SmallTest @@ -493,6 +587,9 @@ private void setMessageIdentifier(MessageState message, int messageIdentifier) { } private MessageState buildMessageState() { - return new MessageState(null, null, Mockito.mock(MessageStateHandler.class), false); + var handler = Mockito.mock(MessageStateHandler.class); + doReturn(ValueAnimator.ofInt(1, 2)).when(handler).show(anyInt(), anyInt()); + doReturn(null).when(handler).hide(anyInt(), anyInt(), anyBoolean()); + return new MessageState(null, null, handler, false); } } diff --git a/components/messages/android/java/src/org/chromium/components/messages/MessageContainer.java b/components/messages/android/java/src/org/chromium/components/messages/MessageContainer.java index e8ac38d88d3cb..2397aad1863f1 100644 --- a/components/messages/android/java/src/org/chromium/components/messages/MessageContainer.java +++ b/components/messages/android/java/src/org/chromium/components/messages/MessageContainer.java @@ -16,12 +16,15 @@ import androidx.core.view.ViewCompat; import androidx.core.view.accessibility.AccessibilityNodeInfoCompat.AccessibilityActionCompat; +import org.chromium.base.Log; import org.chromium.base.TraceEvent; /** * Container holding messages. */ public class MessageContainer extends FrameLayout { + private static final String TAG = "MessageContainer"; + interface MessageContainerA11yDelegate { void onA11yFocused(); void onA11yFocusCleared(); @@ -99,6 +102,10 @@ void removeMessage(View view) { public int getMessageBannerHeight() { assert getChildCount() > 0; + // TODO(https://crbug.com/1382275): remove this log after fix. + if (getChildAt(0) == null) { + Log.w(TAG, "Null child in message container; child count %s", getChildCount()); + } return getChildAt(0).getHeight(); }