Unstuck StartSurfaceLayoutTest with pending thumbnail readbacks

When a readback cannot be returned because the tab is no longer attached
to the view, all the subsequent readback would be abandoned, causing
blank or stale thumbnails in Grid Tab Switcher. When this happens, the
symptom persists until the tab is reattached to the view or a restart.

This symptom caused StartSurfaceLayoutTest to be flaky. This CL tries to
reduce the flakiness by reattaching the Tab to the view by switching to
it.

Bug: 989348
Change-Id: Ia714ac8f674b5194e91a56cf19b2b9e4ece8b55f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1727520Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683244}

chrome/android/features/start_surface/internal/javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceLayoutTest.java

@@ -42,6 +42,7 @@ import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.compositor.layouts.Layout;
 import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tabmodel.TabSelectionType;
 import org.chromium.chrome.browser.tasks.tab_management.GridTabSwitcher;
 import org.chromium.chrome.browser.util.FeatureUtilities;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
@@ -191,6 +192,7 @@ public class StartSurfaceLayoutTest {
 
     /**
      * Make Chrome have {@code numTabs} or Tabs with {@code url} loaded.
+     * @param numTabs The number of tabs we expect after finishing
      * @param url The URL to load. Skip loading when null, but the thumbnail for the NTP might not
      *            be saved.
      */
@@ -209,14 +211,43 @@ public class StartSurfaceLayoutTest {
                                       .getCurrentModel()
                                       .getTabAt(i);
 
-            // TODO(wychen): missing thumbnail should not happen in web tabs, either, but checking
-            //               it makes the tests too flaky.
-            if (previousTab.isNativePage()) checkThumbnailsExist(previousTab);
+            boolean fixPendingReadbacks = mActivityTestRule.getActivity()
+                                                  .getTabContentManager()
+                                                  .getPendingReadbacksForTesting()
+                    != 0;
+
+            // When there are pending readbacks due to detached Tabs, try to fix it by switching
+            // back to that tab.
+            if (fixPendingReadbacks) {
+                int lastIndex = i;
+                // clang-format off
+                TestThreadUtils.runOnUiThreadBlocking(() ->
+                    mActivityTestRule.getActivity().getCurrentTabModel().setIndex(
+                            lastIndex, TabSelectionType.FROM_USER)
+                );
+                // clang-format on
+            }
+            checkThumbnailsExist(previousTab);
+            if (fixPendingReadbacks) {
+                int currentIndex = i + 1;
+                // clang-format off
+                TestThreadUtils.runOnUiThreadBlocking(() ->
+                    mActivityTestRule.getActivity().getCurrentTabModel().setIndex(
+                            currentIndex, TabSelectionType.FROM_USER)
+                );
+                // clang-format on
+            }
         }
         ChromeTabUtils.waitForTabPageLoaded(mActivityTestRule.getActivity().getActivityTab(), null,
                 null, WAIT_TIMEOUT_SECONDS * 10);
         assertEquals(
                 numTabs, mActivityTestRule.getActivity().getTabModelSelector().getTotalTabCount());
+
+        // clang-format off
+        CriteriaHelper.pollUiThread(Criteria.equals(0, () ->
+            mActivityTestRule.getActivity().getTabContentManager().getPendingReadbacksForTesting()
+        ));
+        // clang-format on
     }
 
     private void testTabToGrid(String fromUrl) throws InterruptedException {

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java

@@ -489,6 +489,11 @@ public class TabContentManager {
         nativeSetCaptureMinRequestTimeForTesting(mNativeTabContentManager, timeMs);
     }
 
+    @VisibleForTesting
+    public int getPendingReadbacksForTesting() {
+        return nativeGetPendingReadbacksForTesting(mNativeTabContentManager);
+    }
+
     @CalledByNative
     protected void notifyListenersOfThumbnailChange(int tabId) {
         for (ThumbnailChangeListener listener : mListeners) {
@@ -520,5 +525,6 @@ public class TabContentManager {
             long nativeTabContentManager, int tabId, Callback<Bitmap> callback);
     private native void nativeSetCaptureMinRequestTimeForTesting(
             long nativeTabContentManager, int timeMs);
+    private native int nativeGetPendingReadbacksForTesting(long nativeTabContentManager);
     private static native void nativeDestroy(long nativeTabContentManager);
 }

chrome/browser/android/compositor/tab_content_manager.cc

@@ -412,6 +412,12 @@ void TabContentManager::SetCaptureMinRequestTimeForTesting(
   thumbnail_cache_->SetCaptureMinRequestTimeForTesting(timeMs);
 }
 
+jint TabContentManager::GetPendingReadbacksForTesting(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj) {
+  return pending_tab_readbacks_.size();
+}
+
 // ----------------------------------------------------------------------------
 // Native JNI methods
 // ----------------------------------------------------------------------------

chrome/browser/android/compositor/tab_content_manager.h

@@ -115,6 +115,9 @@ class TabContentManager : public ThumbnailCacheObserver {
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       jint timeMs);
+  jint GetPendingReadbacksForTesting(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj);
 
   // ThumbnailCacheObserver implementation;
   void OnFinishedThumbnailRead(TabId tab_id) override;