Do not limit concurrent readbacks in TabContentManager

The maximum number of concurrent readbacks was capped at 1. 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. The tab can be reattached to the view by switching to it,
but there is no way for a user to know which tab to switch to.

This CL removes the limitation. The maximum number of concurrent
readbacks is still limited by the number of tabs, since a tab can only
have one readback. Removing the limitation should not increase the
system loading because the extra concurrent readbacks would be in
pending state waiting for their tabs to be reattached.

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

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

@@ -5,6 +5,7 @@
 package org.chromium.chrome.features.start_surface;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
 import static org.chromium.base.test.util.CallbackHelper.WAIT_TIMEOUT_SECONDS;
@@ -190,13 +191,23 @@ public class StartSurfaceLayoutTest {
         assertThumbnailsAreReleased();
     }
 
+    /**
+     * Make Chrome have {@code numTabs} or Tabs with {@code url} loaded.
+     * @see #prepareTabs(int, String, boolean)
+     */
+    private void prepareTabs(int numTabs, @Nullable String url) throws InterruptedException {
+        prepareTabs(numTabs, url, true);
+    }
+
     /**
      * 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.
+     * @param waitForLoading Whether wait for URL loading.
      */
-    private void prepareTabs(int numTabs, @Nullable String url) throws InterruptedException {
+    private void prepareTabs(int numTabs, @Nullable String url, boolean waitForLoading)
+            throws InterruptedException {
         assertTrue(numTabs >= 1);
         assertEquals(1, mActivityTestRule.getActivity().getTabModelSelector().getTotalTabCount());
 
@@ -205,6 +216,7 @@ public class StartSurfaceLayoutTest {
             MenuUtils.invokeCustomMenuActionSync(InstrumentationRegistry.getInstrumentation(),
                     mActivityTestRule.getActivity(), org.chromium.chrome.R.id.new_tab_menu_id);
             if (url != null) mActivityTestRule.loadUrl(url);
+            if (!waitForLoading) continue;
 
             Tab previousTab = mActivityTestRule.getActivity()
                                       .getTabModelSelector()
@@ -243,12 +255,16 @@ public class StartSurfaceLayoutTest {
         assertEquals(
                 numTabs, mActivityTestRule.getActivity().getTabModelSelector().getTotalTabCount());
 
+        if (waitForLoading) {
             // clang-format off
             CriteriaHelper.pollUiThread(Criteria.equals(0, () ->
-            mActivityTestRule.getActivity().getTabContentManager().getPendingReadbacksForTesting()
+                mActivityTestRule.getActivity()
+                        .getTabContentManager()
+                        .getPendingReadbacksForTesting()
             ));
             // clang-format on
         }
+    }
 
     private void testTabToGrid(String fromUrl) throws InterruptedException {
         mActivityTestRule.loadUrl(fromUrl);
@@ -292,6 +308,39 @@ public class StartSurfaceLayoutTest {
         testGridToTab(false, false);
     }
 
+    @Test
+    @MediumTest
+    @DisabledTest(message = "crbug.com/986047. This works on emulators but not on real devices.")
+    public void testGridToTabToCurrentLiveDetached() throws Exception {
+        for (int i = 0; i < 10; i++) {
+            // Quickly create some tabs, navigate to web pages, and don't wait for thumbnail
+            // capturing.
+            prepareTabs(2, mUrl, false);
+            // Hopefully we are in a state where some pending readbacks are stuck because their tab
+            // is not attached to the view.
+            if (mActivityTestRule.getActivity()
+                            .getTabContentManager()
+                            .getPendingReadbacksForTesting()
+                    > 0)
+                break;
+
+            // Restart Chrome.
+            // Although we're destroying the activity, the Application will still live on since its
+            // in the same process as this test.
+            TestThreadUtils.runOnUiThreadBlocking(
+                    () -> mActivityTestRule.getActivity().getTabModelSelector().closeAllTabs());
+            ApplicationTestUtils.finishActivity(mActivityTestRule.getActivity());
+            mActivityTestRule.startMainActivityOnBlankPage();
+            Assert.assertEquals(1, mActivityTestRule.tabsCount(false));
+        }
+        assertNotEquals(0,
+                mActivityTestRule.getActivity()
+                        .getTabContentManager()
+                        .getPendingReadbacksForTesting());
+        // The last tab should still get thumbnail even though readbacks for other tabs are stuck.
+        testGridToTab(false, false);
+    }
+
     @Test
     @MediumTest
     @Features.EnableFeatures(ChromeFeatureList.TAB_TO_GTS_ANIMATION + "<Study")

chrome/browser/android/compositor/tab_content_manager.cc

@@ -41,7 +41,6 @@ using base::android::JavaRef;
 
 namespace {
 
-const size_t kMaxReadbacks = 1;
 using TabReadbackCallback = base::OnceCallback<void(float, const SkBitmap&)>;
 
 }  // namespace
@@ -226,8 +225,7 @@ content::RenderWidgetHostView* TabContentManager::GetRwhvForTab(
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DCHECK(tab_android);
   const int tab_id = tab_android->GetAndroidId();
-  if (pending_tab_readbacks_.find(tab_id) != pending_tab_readbacks_.end() ||
-      pending_tab_readbacks_.size() >= kMaxReadbacks) {
+  if (pending_tab_readbacks_.find(tab_id) != pending_tab_readbacks_.end()) {
     return nullptr;
   }