Fix a crash in pop-up tab strip

In the popup TabStrip used by Duet-TabStrip integration, there is a
crash in RecycleView when user very intensively clicks on the TabStrip
favicon to close multiple tabs. The stack indicates that the
recyclerView is trying to remove the same ViewHolder in two consecutive
remove operations, and the second call would crash. The fix introduced
in this CL is to early end the animation if we know for sure that this
item is going to be removed. This should help with better identifying
the ViewHolder to remove.

This fix is inspired by the discussion here:
https://b.corp.google.com/issues/34184109
and the CL here:
https://critique.corp.google.com/#review/188055979/depot/google3/java/com/google/android/apps/play/music/app/ui/common/MediaListRecyclerFragment.java
Even though we early end the animation, there should be no visible
animation change from the UI perspective.

Bug: 1045944
Change-Id: I0d8a7b098e17cdc86c4ae3227cd9dc1ba37b2adc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036874
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738392}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListCoordinator.java

@@ -221,7 +221,8 @@ public class TabListCoordinator implements Destroyable {
         mMediator = new TabListMediator(context, modelList, tabModelSelector, thumbnailProvider,
                 titleProvider, tabListFaviconProvider, actionOnRelatedTabs,
                 createGroupButtonProvider, selectionDelegateProvider,
-                gridCardOnClickListenerProvider, dialogHandler, componentName, itemType);
+                gridCardOnClickListenerProvider, dialogHandler, this::endItemAnimationForPosition,
+                componentName, itemType);
 
         if (mMode == TabListMode.GRID) {
             GridLayoutManager gridLayoutManager =
@@ -399,4 +400,12 @@ public class TabListCoordinator implements Destroyable {
     void removeSpecialListItem(@UiType int uiType, int itemIdentifier) {
         mMediator.removeSpecialItemFromModel(uiType, itemIdentifier);
     }
+
+    private void endItemAnimationForPosition(int pos) {
+        RecyclerView.ViewHolder viewHolder = mRecyclerView.findViewHolderForAdapterPosition(pos);
+        if (viewHolder == null || mRecyclerView.getItemAnimator() == null) {
+            return;
+        }
+        mRecyclerView.getItemAnimator().endAnimation(viewHolder);
+    }
 }

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java

@@ -21,6 +21,7 @@ import android.os.Bundle;
 import android.os.Handler;
 import android.support.v7.content.res.AppCompatResources;
 import android.support.v7.widget.GridLayoutManager;
+import android.support.v7.widget.RecyclerView;
 import android.support.v7.widget.helper.ItemTouchHelper;
 import android.util.Pair;
 import android.view.View;
@@ -182,6 +183,19 @@ class TabListMediator {
         }
     }
 
+    /**
+     * An interface used to end current item animation in {@link RecyclerView} at certain position.
+     */
+    interface ItemAnimationStopper {
+        /**
+         * This method stops the item animation from {@link RecyclerView.ItemAnimator} for item in
+         * {@code position}.
+         *
+         * @param position  The position of the item whose animation will end.
+         */
+        void endItemAnimationForPosition(int position);
+    }
+
     /**
      * An interface to show IPH for a tab.
      */
@@ -432,8 +446,8 @@ class TabListMediator {
             @Nullable CreateGroupButtonProvider createGroupButtonProvider,
             @Nullable SelectionDelegateProvider selectionDelegateProvider,
             @Nullable GridCardOnClickListenerProvider gridCardOnClickListenerProvider,
-            @Nullable TabGridDialogHandler dialogHandler, String componentName,
-            @UiType int uiType) {
+            @Nullable TabGridDialogHandler dialogHandler, ItemAnimationStopper itemAnimationStopper,
+            String componentName, @UiType int uiType) {
         mContext = context;
         mTabModelSelector = tabModelSelector;
         mThumbnailProvider = thumbnailProvider;
@@ -533,9 +547,14 @@ class TabListMediator {
 
             @Override
             public void willCloseTab(Tab tab, boolean animate) {
-                if (mModel.indexFromId(tab.getId()) == TabModel.INVALID_TAB_INDEX) return;
+                int index = mModel.indexFromId(tab.getId());
+                if (index == TabModel.INVALID_TAB_INDEX) return;
                 tab.removeObserver(mTabObserver);
-                mModel.removeAt(mModel.indexFromId(tab.getId()));
+                // TODO(crbug.com/1045944): This line is for a specific crash, it should not be
+                // needed. Investigate why the crash is happening only in pop-up tab strip. Maybe
+                // related to how RecyclerView layouts in a wrap_content PopupWindow.
+                if (uiType == UiType.STRIP) itemAnimationStopper.endItemAnimationForPosition(index);
+                mModel.removeAt(index);
             }
 
             @Override

chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java

@@ -210,6 +210,8 @@ public class TabListMediatorUnitTest {
     UrlUtilities.Natives mUrlUtilitiesJniMock;
     @Mock
     TabListMediator.TabGridAccessibilityHelper mTabGridAccessibilityHelper;
+    @Mock
+    TabListMediator.ItemAnimationStopper mItemAnimationStopper;
 
     @Captor
     ArgumentCaptor<TabModelObserver> mTabModelObserverCaptor;
@@ -317,7 +319,7 @@ public class TabListMediatorUnitTest {
         mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
                 mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
                 mTabListFaviconProvider, false, null, null, mGridCardOnClickListenerProvider, null,
-                getClass().getSimpleName(), 0);
+                mItemAnimationStopper, getClass().getSimpleName(), 0);
         mMediator.registerOrientationListener(mGridLayoutManager);
         TrackerFactory.setTrackerForTests(mTracker);
         ContextUtils.initApplicationContextForTests(mContext);
@@ -604,6 +606,20 @@ public class TabListMediatorUnitTest {
         assertThat(mModel.size(), equalTo(2));
     }
 
+    @Test
+    @Features.EnableFeatures({ChromeFeatureList.TAB_GROUPS_ANDROID,
+            ChromeFeatureList.TAB_GROUPS_UI_IMPROVEMENTS_ANDROID})
+    // clang-format off
+    public void tabClosure_ForceStopItemAnimationForStrip() {
+        // clang-format on
+        setUpForTabGroupOperation(TabListMediatorType.TAB_STRIP);
+
+        mMediatorTabModelObserver.willCloseTab(mTab2, false);
+
+        assertThat(mModel.size(), equalTo(1));
+        verify(mItemAnimationStopper).endItemAnimationForPosition(eq(1));
+    }
+
     @Test
     public void tabAddition_RestoreNotComplete() {
         initAndAssertAllProperties();
@@ -1861,12 +1877,19 @@ public class TabListMediatorUnitTest {
         TabListMediator.TabGridDialogHandler handler =
                 type == TabListMediatorType.TAB_GRID_DIALOG ? mTabGridDialogHandler : null;
         boolean actionOnRelatedTabs = type == TabListMediatorType.TAB_SWITCHER;
+        int uiType = 0;
+        if (type == TabListMediatorType.TAB_SWITCHER
+                || type == TabListMediatorType.TAB_GRID_DIALOG) {
+            uiType = TabProperties.UiType.CLOSABLE;
+        } else if (type == TabListMediatorType.TAB_STRIP) {
+            uiType = TabProperties.UiType.STRIP;
+        }
         CachedFeatureFlags.setTabGroupsAndroidEnabledForTesting(true);
 
         mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
                 mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
                 mTabListFaviconProvider, actionOnRelatedTabs, null, null, null, handler,
-                getClass().getSimpleName(), 0);
+                mItemAnimationStopper, getClass().getSimpleName(), uiType);
 
         // There are two TabModelObserver and two TabGroupModelFilter.Observer added when
         // initializing TabListMediator, one set from TabListMediator and the other from