Clank: Stop reloading after a new load.

This prevents a reload scheduled after TabWebContentsObserver.renderProcessGone
from racing with a subsequent page load.

Bug: 828400
Change-Id: I5743736aded76a227a1cda0a4fa1824751df9a3f
Reviewed-on: https://chromium-review.googlesource.com/992612Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Matthew Cary <mattcary@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549488}

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java

@@ -62,7 +62,9 @@ import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.prerender.ExternalPrerenderHandler;
 import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabObserver;
 import org.chromium.chrome.browser.util.IntentUtils;
 import org.chromium.chrome.browser.util.UrlUtilities;
 import org.chromium.content.browser.BrowserStartupController;
@@ -213,6 +215,7 @@ public class CustomTabsConnection {
 
         // Only for hidden tab.
         public final Tab tab;
+        public final TabObserver observer;
         @VisibleForTesting
         boolean mDidFinishLoad;
 
@@ -221,21 +224,23 @@ public class CustomTabsConnection {
         public final Bundle extras;
 
         static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) {
-            return new SpeculationParams(session, url, PREFETCH, null, null, null, null);
+            return new SpeculationParams(session, url, PREFETCH, null, null, null, null, null);
         }
 
         static SpeculationParams forPrerender(CustomTabsSessionToken session, String url,
                 WebContents webcontents, String referrer, Bundle extras) {
             return new SpeculationParams(
-                    session, url, PRERENDER, webcontents, referrer, extras, null);
+                    session, url, PRERENDER, webcontents, referrer, extras, null, null);
         }
         static SpeculationParams forHiddenTab(CustomTabsSessionToken session, String url, Tab tab,
-                String referrer, Bundle extras) {
-            return new SpeculationParams(session, url, HIDDEN_TAB, null, referrer, extras, tab);
+                TabObserver observer, String referrer, Bundle extras) {
+            return new SpeculationParams(
+                    session, url, HIDDEN_TAB, null, referrer, extras, tab, observer);
         }
 
         private SpeculationParams(CustomTabsSessionToken session, String url, int speculationMode,
-                WebContents webContents, String referrer, Bundle extras, Tab tab) {
+                WebContents webContents, String referrer, Bundle extras, Tab tab,
+                TabObserver observer) {
             this.session = session;
             this.url = url;
             this.speculationMode = speculationMode;
@@ -243,6 +248,21 @@ public class CustomTabsConnection {
             this.referrer = referrer;
             this.extras = extras;
             this.tab = tab;
+            this.observer = observer;
+        }
+    }
+
+    static class HiddenTabObserver extends EmptyTabObserver {
+        private CustomTabsConnection mCustomTabsConnection;
+
+        HiddenTabObserver(CustomTabsConnection connection) {
+            mCustomTabsConnection = connection;
+        }
+
+        @Override
+        public void onCrash(Tab tab, boolean sadTabShown) {
+            final CustomTabsConnection connection = mCustomTabsConnection;
+            ThreadUtils.postOnUiThread(() -> { connection.cancelSpeculation(null /* session */); });
         }
     }
 
@@ -897,6 +917,7 @@ public class CustomTabsConnection {
             if (mSpeculation == null || session == null) return null;
             if (session.equals(mSpeculation.session) && mSpeculation.tab != null) {
                 Tab tab = mSpeculation.tab;
+                tab.removeObserver(mSpeculation.observer);
                 String speculatedUrl = mSpeculation.url;
                 String speculationReferrer = mSpeculation.referrer;
                 mSpeculation = null;
@@ -1526,6 +1547,8 @@ public class CustomTabsConnection {
         if (IntentHandler.getExtraHeadersFromIntent(extrasIntent) != null) return;
 
         Tab tab = Tab.createDetached(new CustomTabDelegateFactory(false, false, null));
+        HiddenTabObserver observer = new HiddenTabObserver(this);
+        tab.addObserver(observer);
 
         // Updating post message as soon as we have a valid WebContents.
         mClientManager.resetPostMessageHandlerForSession(
@@ -1537,7 +1560,8 @@ public class CustomTabsConnection {
             loadParams.setReferrer(
                     new Referrer(referrer, WebReferrerPolicy.DEFAULT));
         }
-        mSpeculation = SpeculationParams.forHiddenTab(session, url, tab, referrer, extras);
+        mSpeculation =
+                SpeculationParams.forHiddenTab(session, url, tab, observer, referrer, extras);
         mSpeculation.tab.loadUrl(loadParams);
     }
 

chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java

@@ -1693,6 +1693,8 @@ public class Tab
     protected void didStartPageLoad(String validatedUrl, boolean showingErrorPage) {
         updateTitle();
         removeSadTabIfPresent();
+        // A page load means that any pending reload is no longer necessary.
+        mNeedsReload = false;
 
         mDataSavedOnStartPageLoad =
                 DataReductionProxySettings.getInstance().getContentLengthSavedInHistorySummary();

chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java

@@ -111,7 +111,8 @@ public class TabWebContentsObserver extends WebContentsObserver {
                 || activityState == ActivityState.STOPPED
                 || activityState == ActivityState.DESTROYED) {
             // The tab crashed in background or was killed by the OS out-of-memory killer.
-            //setNeedsReload(true);
+            // TODO(crbug.com/829381): why not reuse the native needsReload flag found in
+            // NavigationController?
             mTab.setNeedsReload(true);
             if (applicationRunning) {
                 rendererCrashStatus = TAB_RENDERER_CRASH_STATUS_HIDDEN_IN_FOREGROUND_APP;

chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java

@@ -25,6 +25,7 @@ import org.junit.After;
 import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -43,6 +44,7 @@ import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.content_public.browser.WebContents;
 
 import java.util.ArrayList;
@@ -50,8 +52,6 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.Callable;
 import java.util.concurrent.FutureTask;
-import java.util.concurrent.Semaphore;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
 /** Tests for CustomTabsConnection. */
@@ -63,6 +63,9 @@ public class CustomTabsConnectionTest {
     private static final String INVALID_SCHEME_URL = "intent://www.google.com";
     private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
 
+    @Rule
+    public Features.InstrumentationProcessor mProcessor = new Features.InstrumentationProcessor();
+
     @Before
     public void setUp() throws Exception {
         PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DATA_DIRECTORY_SUFFIX);
@@ -240,8 +243,8 @@ public class CustomTabsConnectionTest {
     @Test
     @SmallTest
     @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE)
-    @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
-            "enable-features=" + ChromeFeatureList.CCT_BACKGROUND_TAB})
+    @CommandLineFlags.Add(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
+    @Features.EnableFeatures(ChromeFeatureList.CCT_BACKGROUND_TAB)
     @RetryOnFailure
     public void testOnlyOneHiddenTab() throws Exception {
         Assert.assertTrue("Failed warmup()", mCustomTabsConnection.warmup(0));
@@ -253,7 +256,7 @@ public class CustomTabsConnectionTest {
         // First hidden tab, add an observer to check that it's destroyed.
         Assert.assertTrue("Failed first mayLaunchUrl()",
                 mCustomTabsConnection.mayLaunchUrl(token, Uri.parse(URL), null, null));
-        final Semaphore destroyed = new Semaphore(0);
+        final CallbackHelper tabDestroyedHelper = new CallbackHelper();
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
@@ -264,7 +267,7 @@ public class CustomTabsConnectionTest {
                 tab.addObserver(new EmptyTabObserver() {
                     @Override
                     public void onDestroyed(Tab destroyedTab) {
-                        destroyed.release();
+                        tabDestroyedHelper.notifyCalled();
                     }
                 });
             }
@@ -283,9 +286,7 @@ public class CustomTabsConnectionTest {
                 Assert.assertEquals(URL2, mCustomTabsConnection.mSpeculation.url);
             }
         });
-
-        Assert.assertTrue(
-                "Only one hidden tab should exist.", destroyed.tryAcquire(10, TimeUnit.SECONDS));
+        tabDestroyedHelper.waitForCallback("The first hidden tab should have been destroyed", 0);
 
         // Clears the second hidden tab.
         mCustomTabsConnection.resetThrottling(Process.myUid());
@@ -293,6 +294,40 @@ public class CustomTabsConnectionTest {
                 mCustomTabsConnection.mayLaunchUrl(token, null, null, null));
     }
 
+    /**
+     * Tests that if the renderer backing a hidden tab is killed, the speculation is
+     * canceled.
+     */
+    @Test
+    @SmallTest
+    @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE)
+    @CommandLineFlags.Add(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
+    @Features.EnableFeatures(ChromeFeatureList.CCT_BACKGROUND_TAB)
+    @RetryOnFailure
+    public void testKillHiddenTabRenderer() throws Exception {
+        Assert.assertTrue("Failed warmup()", mCustomTabsConnection.warmup(0));
+        CustomTabsSessionToken token = CustomTabsSessionToken.createMockSessionTokenForTesting();
+        Assert.assertTrue("Failed newSession()", mCustomTabsConnection.newSession(token));
+        mCustomTabsConnection.setSpeculationModeForSession(
+                token, CustomTabsConnection.SpeculationParams.HIDDEN_TAB);
+        Assert.assertTrue("Failed first mayLaunchUrl()",
+                mCustomTabsConnection.mayLaunchUrl(token, Uri.parse(URL), null, null));
+        final CallbackHelper tabDestroyedHelper = new CallbackHelper();
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            Assert.assertNotNull("Null speculation", mCustomTabsConnection.mSpeculation);
+            Tab speculationTab = mCustomTabsConnection.mSpeculation.tab;
+            Assert.assertNotNull("Null speculation tab", speculationTab);
+            speculationTab.addObserver(new EmptyTabObserver() {
+                @Override
+                public void onDestroyed(Tab tab) {
+                    tabDestroyedHelper.notifyCalled();
+                }
+            });
+            speculationTab.getWebContents().simulateRendererKilledForTesting(false);
+        });
+        tabDestroyedHelper.waitForCallback("The speculated tab was not destroyed", 0);
+    }
+
     @Test
     @SmallTest
     @Restriction(RESTRICTION_TYPE_NON_LOW_END_DEVICE)

chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java

@@ -22,6 +22,7 @@ import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.net.test.util.TestWebServer;
 
 /**
  * Tests related to the sad tab logic.
@@ -68,6 +69,50 @@ public class SadTabTest {
         Assert.assertFalse(tab.isShowingSadTab());
     }
 
+    /**
+     * Verify that a tab navigating to a page that is killed in the background is reloaded.
+     */
+    @Test
+    @SmallTest
+    @Feature({"SadTab"})
+    public void testSadTabReloadAfterKill() throws Throwable {
+        final Tab tab = mActivityTestRule.getActivity().getActivityTab();
+
+        TestWebServer webServer = TestWebServer.start();
+        try {
+            final String url1 = webServer.setEmptyResponse("/page1.html");
+            mActivityTestRule.loadUrl(url1);
+            Assert.assertFalse(tab.needsReload());
+            simulateRendererKilled(tab, false);
+            Assert.assertTrue(tab.needsReload());
+        } finally {
+            webServer.shutdown();
+        }
+    }
+
+    /**
+     * Verify that a tab killed in the background is not reloaded if another load has started.
+     */
+    @Test
+    @SmallTest
+    @Feature({"SadTab"})
+    public void testSadTabNoReloadAfterLoad() throws Throwable {
+        final Tab tab = mActivityTestRule.getActivity().getActivityTab();
+
+        TestWebServer webServer = TestWebServer.start();
+        try {
+            final String url1 = webServer.setEmptyResponse("/page1.html");
+            final String url2 = webServer.setEmptyResponse("/page2.html");
+            mActivityTestRule.loadUrl(url1);
+            Assert.assertFalse(tab.needsReload());
+            simulateRendererKilled(tab, false);
+            mActivityTestRule.loadUrl(url2);
+            Assert.assertFalse(tab.needsReload());
+        } finally {
+            webServer.shutdown();
+        }
+    }
+
     /**
      * Confirm that after a successive refresh of a failed tab that failed to load, change the
      * button from "Reload" to "Send Feedback". If reloaded a third time and it is successful it