[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 <lazzzis@google.com>
Reviewed-by: Aishwarya Rajesh <aishwaryarj@google.com>
Cr-Commit-Position: refs/branch-heads/5481@{#437}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

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<MessageState> candidates, Runnable onFinished) {
+    private void triggerStackingAnimation(List<MessageState> 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<MessageState> 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.:

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<Runnable> 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<Runnable> 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);
     }
 }

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();
     }