Avoid using stale tab index when updating favicon

Favicon in GTS updates via a callback. The latest tab index should be
used in the callback because the original tab index can become invalid.
This CL avoids using stale tab index by using the same favicon callback
for single/group tab in GTS.

Change-Id: I322793bb2ce0979b9f36986873ddc7f7b9963ae5
Bug: 1070135
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2148127
Commit-Queue: Mei Liang <meiliang@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758915}

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

@@ -1365,6 +1365,16 @@ class TabListMediator {
         if (modelIndex == Tab.INVALID_TAB_ID) return;
         List<Tab> relatedTabList = getRelatedTabsForId(pseudoTab.getId());
 
+        Callback<Drawable> faviconCallback = drawable -> {
+            assert drawable != null;
+            // Need to re-get the index because the original index can be stale when callback is
+            // triggered.
+            int index = mModel.indexFromId(pseudoTab.getId());
+            if (index != TabModel.INVALID_TAB_INDEX && drawable != null) {
+                mModel.get(index).model.set(TabProperties.FAVICON, drawable);
+            }
+        };
+
         if (mActionsOnAllRelatedTabs && relatedTabList.size() > 1) {
             if (!TabUiFeatureUtilities.isTabGroupsAndroidContinuationEnabled()) {
                 // For tab group card in grid tab switcher, the favicon is set to be null.
@@ -1382,10 +1392,7 @@ class TabListMediator {
 
             // For tab group card in grid tab switcher, the favicon is the composed favicon.
             mTabListFaviconProvider.getComposedFaviconImageAsync(
-                    urls, pseudoTab.isIncognito(), (drawable) -> {
-                        assert drawable != null;
-                        mModel.get(modelIndex).model.set(TabProperties.FAVICON, drawable);
-                    });
+                    urls, pseudoTab.isIncognito(), faviconCallback);
 
             return;
         }
@@ -1400,15 +1407,7 @@ class TabListMediator {
             mModel.get(modelIndex).model.set(TabProperties.FAVICON, drawable);
             return;
         }
-        Callback<Drawable> faviconCallback = drawable -> {
-            assert drawable != null;
-            // Need to re-get the index because the original index can be stale when callback is
-            // triggered.
-            int index = mModel.indexFromId(pseudoTab.getId());
-            if (index != Tab.INVALID_TAB_ID && drawable != null) {
-                mModel.get(index).model.set(TabProperties.FAVICON, drawable);
-            }
-        };
+
         mTabListFaviconProvider.getFaviconForUrlAsync(
                 pseudoTab.getUrl(), pseudoTab.isIncognito(), faviconCallback);
     }

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

@@ -2025,6 +2025,84 @@ public class TabListMediatorUnitTest {
         assertThat(mModel.get(1).model.get(TabProperties.FAVICON), equalTo(mFaviconDrawable));
     }
 
+    @Test
+    @Features.EnableFeatures({TAB_GROUPS_CONTINUATION_ANDROID})
+    public void testUpdateFaviconForGroup_StaleIndex_SelectAnotherTabWithinGroup() {
+        setUpForTabGroupOperation(TabListMediatorType.TAB_SWITCHER);
+        mMediator.setActionOnAllRelatedTabsForTesting(true);
+        mModel.get(0).model.set(TabProperties.FAVICON, null);
+        mModel.get(1).model.set(TabProperties.FAVICON, null);
+        doNothing()
+                .when(mTabListFaviconProvider)
+                .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+
+        TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
+        List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
+        createTabGroup(group1, TAB2_ID);
+        assertEquals(1, mModel.indexFromId(TAB2_ID));
+
+        mTabObserverCaptor.getValue().onFaviconUpdated(mTab2, mFaviconBitmap);
+
+        // Simulate selecting another Tab within TabGroup before callback in
+        // getComposedFaviconImageAsync triggers
+        mModel.get(1).model.set(TabProperties.TAB_ID, TAB3_ID);
+        mCallbackCaptor.getValue().onResult(mFaviconDrawable);
+
+        assertNotEquals(1, mModel.indexFromId(TAB2_ID));
+        assertNull(mModel.get(1).model.get(TabProperties.FAVICON));
+    }
+
+    @Test
+    @Features.EnableFeatures({TAB_GROUPS_CONTINUATION_ANDROID})
+    public void testUpdateFaviconForGroup_StaleIndex_CloseTab() {
+        setUpForTabGroupOperation(TabListMediatorType.TAB_SWITCHER);
+        mMediator.setActionOnAllRelatedTabsForTesting(true);
+        mModel.get(0).model.set(TabProperties.FAVICON, null);
+        mModel.get(1).model.set(TabProperties.FAVICON, null);
+        doNothing()
+                .when(mTabListFaviconProvider)
+                .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+
+        TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
+        List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
+        createTabGroup(group1, TAB2_ID);
+        assertEquals(1, mModel.indexFromId(TAB2_ID));
+
+        mTabObserverCaptor.getValue().onFaviconUpdated(mTab2, mFaviconBitmap);
+
+        // Simulate closing mTab1 at index 0 before callback in getComposedFaviconImageAsync
+        // triggers.
+        mModel.removeAt(0);
+        mCallbackCaptor.getValue().onResult(mFaviconDrawable);
+
+        assertEquals(0, mModel.indexFromId(TAB2_ID));
+        assertEquals(mFaviconDrawable, mModel.get(0).model.get(TabProperties.FAVICON));
+    }
+
+    @Test
+    @Features.EnableFeatures({TAB_GROUPS_CONTINUATION_ANDROID})
+    public void testUpdateFaviconForGroup_StaleIndex_Reset() {
+        setUpForTabGroupOperation(TabListMediatorType.TAB_SWITCHER);
+        mMediator.setActionOnAllRelatedTabsForTesting(true);
+        mModel.get(0).model.set(TabProperties.FAVICON, null);
+        mModel.get(1).model.set(TabProperties.FAVICON, null);
+        doNothing()
+                .when(mTabListFaviconProvider)
+                .getComposedFaviconImageAsync(any(), anyBoolean(), mCallbackCaptor.capture());
+
+        TabImpl tab3 = prepareTab(TAB3_ID, TAB3_TITLE, TAB3_URL);
+        List<Tab> group1 = new ArrayList<>(Arrays.asList(mTab2, tab3));
+        createTabGroup(group1, TAB2_ID);
+        assertEquals(1, mModel.indexFromId(TAB2_ID));
+
+        mTabObserverCaptor.getValue().onFaviconUpdated(mTab2, mFaviconBitmap);
+
+        // Simulate TabListMediator reset with null before callback in getComposedFaviconImageAsync
+        // triggers.
+        mModel.set(new ArrayList<>());
+        mCallbackCaptor.getValue().onResult(mFaviconDrawable);
+    }
+
     private void initAndAssertAllProperties() {
         List<Tab> tabs = new ArrayList<>();
         for (int i = 0; i < mTabModel.getCount(); i++) {