WebLayer: Use RedirectHandlerImpl

This CL augments WebLayer's external intent launching to use the
RedirectHandler implementation that is used by Chrome. This takes
another step in bringing WebLayer's external intent launching logic up
to par with Chrome's; in particular, see the tests modified in this CL,
which highlight areas that had previously been deficient in WebLayer.

There is one subtlety in this bringup: RedirectHandlerImpl wants to
know the last time that the user interacted with the app as a heuristic
to help determine if a navigation was started by the user. In Chrome
this is determined by the Activity subclass listening for user
interaction. However in WebLayer this information is not available, as
WebLayer's Activity is provided by the client and thus opaque. Instead,
this CL uses a replacement heuristic wherein we pass a user interaction
time to RedirectHandlerImpl that is updated on navigations that are
either from user typing or have a user gesture associated with them. To
date, we have not seen any differences that would result from the usage
of these different heuristics.

Bug: 1031465, 1066291
Change-Id: Ia9b2c94433db61eb743bfdd50caa9f99fc699c46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130790
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757774}

components/external_intents/android/java/src/org/chromium/components/external_intents/RedirectHandlerImpl.java

@@ -27,7 +27,7 @@ public class RedirectHandlerImpl implements RedirectHandler {
      * An invalid entry index.
      */
     public static final int INVALID_ENTRY_INDEX = -1;
-    private static final long INVALID_TIME = -1;
+    public static final long INVALID_TIME = -1;
 
     private static final int NAVIGATION_TYPE_NONE = 0;
     private static final int NAVIGATION_TYPE_FROM_INTENT = 1;

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/ExternalNavigationTest.java

@@ -333,20 +333,14 @@ public class ExternalNavigationTest {
     }
 
     /**
-     * Tests that a navigation that redirects to an external intent that can't be handled but has a
-     * fallback URL that launches an intent that *can* be handled results in the launching of the
-     * second intent.
      * |url| is a URL that redirects to an unhandleable intent but has a fallback URL that redirects
      * to a handleable intent.
-     * Tests that a navigation to |url| launches the handleable intent.
-     * TODO(crbug.com/1031465): Disallow such fallback intent launches by sharing Chrome's
-     * RedirectHandler impl, at which point this should fail and be updated to verify that the
-     * intent is blocked.
+     * Tests that a navigation to |url| blocks the handleable intent by policy on chained redirects.
      */
     @Test
     @SmallTest
     public void
-    testNonHandledExternalIntentWithFallbackUrlThatLaunchesIntentAfterRedirectLaunchesFallbackIntent()
+    testNonHandledExternalIntentWithFallbackUrlThatLaunchesIntentAfterRedirectBlocksFallbackIntent()
             throws Throwable {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(ABOUT_BLANK_URL);
         IntentInterceptor intentInterceptor = new IntentInterceptor();
@@ -360,43 +354,43 @@ public class ExternalNavigationTest {
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { tab.getNavigationController().navigate(Uri.parse(url)); });
 
-        intentInterceptor.waitForIntent();
+        NavigationWaiter waiter = new NavigationWaiter(
+                INTENT_TO_CHROME_URL, tab, /*expectFailure=*/true, /*waitForPaint=*/false);
+        waiter.waitForNavigation();
 
-        // The current URL should not have changed, and the intent should have been launched.
+        Assert.assertNull(intentInterceptor.mLastIntent);
+
+        // The current URL should not have changed.
         Assert.assertEquals(ABOUT_BLANK_URL, mActivityTestRule.getCurrentDisplayUrl());
-        Intent intent = intentInterceptor.mLastIntent;
-        Assert.assertNotNull(intent);
-        Assert.assertEquals(INTENT_TO_CHROME_PACKAGE, intent.getPackage());
-        Assert.assertEquals(INTENT_TO_CHROME_ACTION, intent.getAction());
-        Assert.assertEquals(INTENT_TO_CHROME_DATA_STRING, intent.getDataString());
     }
 
     /**
      * Tests that going to a page that loads an intent that can be handled in onload() results in
-     * the external intent being launched.
-     * TODO(crbug.com/1031465): Disallow such intent launches by sharing Chrome's RedirectHandler
-     * impl, at which point this should fail and be updated to verify that the intent is blocked.
+     * the external intent being blocked by policy on intents without user gestures loading in the
+     * midst of a user-typed navigation.
      */
     @Test
     @SmallTest
-    public void testExternalIntentLaunchedViaOnLoad() throws Throwable {
+    public void testExternalIntentViaOnLoadBlocked() throws Throwable {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(ABOUT_BLANK_URL);
         IntentInterceptor intentInterceptor = new IntentInterceptor();
         activity.setIntentInterceptor(intentInterceptor);
 
         String url = mActivityTestRule.getTestDataURL(PAGE_THAT_INTENTS_TO_CHROME_ON_LOAD_FILE);
 
-        mActivityTestRule.navigateAndWait(url);
+        Tab tab = mActivityTestRule.getActivity().getTab();
 
-        intentInterceptor.waitForIntent();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { tab.getNavigationController().navigate(Uri.parse(url)); });
 
-        // The current URL should not have changed, and the intent should have been launched.
+        NavigationWaiter waiter = new NavigationWaiter(
+                INTENT_TO_CHROME_URL, tab, /*expectFailure=*/true, /*waitForPaint=*/false);
+        waiter.waitForNavigation();
+
+        Assert.assertNull(intentInterceptor.mLastIntent);
+
+        // The current URL should not have changed.
         Assert.assertEquals(url, mActivityTestRule.getCurrentDisplayUrl());
-        Intent intent = intentInterceptor.mLastIntent;
-        Assert.assertNotNull(intent);
-        Assert.assertEquals(INTENT_TO_CHROME_PACKAGE, intent.getPackage());
-        Assert.assertEquals(INTENT_TO_CHROME_ACTION, intent.getAction());
-        Assert.assertEquals(INTENT_TO_CHROME_DATA_STRING, intent.getDataString());
     }
 
     /**

weblayer/browser/java/org/chromium/weblayer_private/InterceptNavigationDelegateImpl.java

@@ -4,10 +4,13 @@
 
 package org.chromium.weblayer_private;
 
+import android.os.SystemClock;
+
 import org.chromium.base.annotations.NativeMethods;
 import org.chromium.components.external_intents.ExternalNavigationHandler;
 import org.chromium.components.external_intents.ExternalNavigationHandler.OverrideUrlLoadingResult;
 import org.chromium.components.external_intents.ExternalNavigationParams;
+import org.chromium.components.external_intents.RedirectHandlerImpl;
 import org.chromium.components.navigation_interception.InterceptNavigationDelegate;
 import org.chromium.components.navigation_interception.NavigationParams;
 import org.chromium.content_public.browser.WebContents;
@@ -18,14 +21,17 @@ import org.chromium.content_public.browser.WebContents;
  */
 public class InterceptNavigationDelegateImpl implements InterceptNavigationDelegate {
     private TabImpl mTab;
+    private RedirectHandlerImpl mRedirectHandler;
     private ExternalNavigationHandler mExternalNavHandler;
     private ExternalNavigationDelegateImpl mExternalNavigationDelegate;
+    private long mLastNavigationWithUserGestureTime = RedirectHandlerImpl.INVALID_TIME;
 
     /**
      * Default constructor of {@link InterceptNavigationDelegateImpl}.
      */
     InterceptNavigationDelegateImpl(TabImpl tab) {
         mTab = tab;
+        mRedirectHandler = RedirectHandlerImpl.create();
         mExternalNavigationDelegate = new ExternalNavigationDelegateImpl(mTab);
         mExternalNavHandler = new ExternalNavigationHandler(mExternalNavigationDelegate);
         InterceptNavigationDelegateImplJni.get().associateWithWebContents(
@@ -41,15 +47,14 @@ public class InterceptNavigationDelegateImpl implements InterceptNavigationDeleg
      * ExternalNavigationHandler#shouldOverrideUrlLoading().
      */
     private ExternalNavigationParams.Builder buildExternalNavigationParams(
-            NavigationParams navigationParams, boolean shouldCloseTab) {
+            NavigationParams navigationParams, RedirectHandlerImpl redirectHandler,
+            boolean shouldCloseTab) {
         return new ExternalNavigationParams
                 .Builder(navigationParams.url, mTab.getProfile().isIncognito(),
                         navigationParams.referrer, navigationParams.pageTransitionType,
                         navigationParams.isRedirect)
                 .setApplicationMustBeInForeground(true)
-                // NOTE: ExternalNavigationHandler.java supports the redirect handler being
-                // null, although WebLayer will likely eventually want one.
-                .setRedirectHandler(null)
+                .setRedirectHandler(redirectHandler)
                 .setOpenInNewTab(shouldCloseTab)
                 // TODO(crbug.com/1031465): See whether this needs to become more refined
                 // (cf. //chrome's setting of this field in its version of this method).
@@ -60,6 +65,11 @@ public class InterceptNavigationDelegateImpl implements InterceptNavigationDeleg
                         shouldCloseTab && navigationParams.isMainFrame);
     }
 
+    private int getLastCommittedEntryIndex() {
+        if (mTab.getWebContents() == null) return -1;
+        return mTab.getWebContents().getNavigationController().getLastCommittedEntryIndex();
+    }
+
     private boolean shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent() {
         // Closing of tabs as part of intent launching is not yet implemented in WebLayer; specify
         // parameters such that this flow is never invoked.
@@ -69,9 +79,48 @@ public class InterceptNavigationDelegateImpl implements InterceptNavigationDeleg
 
     @Override
     public boolean shouldIgnoreNavigation(NavigationParams navigationParams) {
+        if (navigationParams.hasUserGesture || navigationParams.hasUserGestureCarryover) {
+            mLastNavigationWithUserGestureTime = SystemClock.elapsedRealtime();
+        }
+
+        RedirectHandlerImpl redirectHandler = null;
+        if (navigationParams.isMainFrame) {
+            redirectHandler = mRedirectHandler;
+        } else if (navigationParams.isExternalProtocol) {
+            // Only external protocol navigations are intercepted for iframe navigations.  Since
+            // we do not see all previous navigations for the iframe, we can not build a complete
+            // redirect handler for each iframe.  Nor can we use the top level redirect handler as
+            // that has the potential to incorrectly give access to the navigation due to previous
+            // main frame gestures.
+            //
+            // By creating a new redirect handler for each external navigation, we are specifically
+            // not covering the case where a gesture is carried over via a redirect.  This is
+            // currently not feasible because we do not see all navigations for iframes and it is
+            // better to error on the side of caution and require direct user gestures for iframes.
+            redirectHandler = RedirectHandlerImpl.create();
+        } else {
+            assert false;
+            return false;
+        }
+
+        // NOTE: Chrome listens for user interaction with its Activity. However, this depends on
+        // being able to subclass the Activity, which is not possible in WebLayer. As a proxy,
+        // WebLayer uses the time of the last navigation with a user gesture to serve as the last
+        // time of user interaction. Note that the user interacting with the webpage causes the
+        // user gesture bit to be set on any navigation in that page for the next several seconds
+        // (cf. comments on //third_party/blink/public/common/frame/user_activation_state.h). This
+        // fact further increases the fidelity of this already-reasonable heuristic as a proxy. To
+        // date we have not seen any concrete evidence of user-visible differences resulting from
+        // the use of the different heuristic.
+        redirectHandler.updateNewUrlLoading(navigationParams.pageTransitionType,
+                navigationParams.isRedirect,
+                navigationParams.hasUserGesture || navigationParams.hasUserGestureCarryover,
+                mLastNavigationWithUserGestureTime, getLastCommittedEntryIndex());
+
         boolean shouldCloseTab = shouldCloseContentsOnOverrideUrlLoadingAndLaunchIntent();
         ExternalNavigationParams params =
-                buildExternalNavigationParams(navigationParams, shouldCloseTab).build();
+                buildExternalNavigationParams(navigationParams, redirectHandler, shouldCloseTab)
+                        .build();
         return (mExternalNavHandler.shouldOverrideUrlLoading(params)
                 != OverrideUrlLoadingResult.NO_OVERRIDE);
     }