WebLayer: Close tabs on launching external intents when instructed

This CL augments WebLayer with closing of tabs when the external
intent launching code asks to do so (namely, when an initial navigation
in a Tab results in the launch of an external intent). To do so we
invoke BrowserImpl#destroyTab(). As clients < 84 are not necessarily
expecting the last tab to be closed by the WebLayer implementation, we
do this only when the client is 84+.

As part of this CL we looked at hardening WebLayer against the case
where the client calls Browser#destroyTab() from within
TabListCallback#onTabRemoved() (which itself is received after the Tab
destruction process has started in the implementation). It turns out
that WebLayer will already fail fast in this case:
Browser#onTabRemoved() sets the removed Tab's Browser to null, and
Browser#destroyTab() raises an exception if the passed-in Tab's Browser
is not equal to |this|.

Bug: 1031465
Change-Id: I87e1464d3be4b0fe138ab7aa14fe700b8b930722
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2144120
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759969}

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

@@ -21,6 +21,7 @@ import org.chromium.base.test.util.CallbackHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.weblayer.Browser;
 import org.chromium.weblayer.Tab;
+import org.chromium.weblayer.TabListCallback;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
 /**
@@ -202,7 +203,8 @@ public class ExternalNavigationTest {
 
     /**
      * Tests that clicking on a link that goes to an external intent in a new tab results in
-     * a new tab being opened whose URL is that of the intent and the intent being launched.
+     * a new tab being opened whose URL is that of the intent and the intent being launched,
+     * followed by the new tab being closed.
      */
     @Test
     @SmallTest
@@ -215,8 +217,26 @@ public class ExternalNavigationTest {
 
         mActivityTestRule.navigateAndWait(url);
 
-        // Grab the existing tab before causing a new one to be opened.
-        Tab tab = mActivityTestRule.getActivity().getTab();
+        // Set up listening for the tab addition and removal that we expect to happen.
+        CallbackHelper onTabAddedCallbackHelper = new CallbackHelper();
+        CallbackHelper onTabRemovedCallbackHelper = new CallbackHelper();
+        TabListCallback tabListCallback = new TabListCallback() {
+            @Override
+            public void onTabAdded(Tab tab) {
+                onTabAddedCallbackHelper.notifyCalled();
+            }
+
+            @Override
+            public void onTabRemoved(Tab tab) {
+                onTabRemovedCallbackHelper.notifyCalled();
+            }
+        };
+        Browser browser = mActivityTestRule.getActivity().getBrowser();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { browser.registerTabListCallback(tabListCallback); });
+
+        // Grab the original tab before it changes.
+        Tab originalTab = mActivityTestRule.getActivity().getTab();
 
         mActivityTestRule.executeScriptSync(
                 "document.onclick = function() {document.getElementById('link').click()}",
@@ -224,23 +244,30 @@ public class ExternalNavigationTest {
         EventUtils.simulateTouchCenterOfView(
                 mActivityTestRule.getActivity().getWindow().getDecorView());
 
-        intentInterceptor.waitForIntent();
+        // (1) A new tab should be created...
+        onTabAddedCallbackHelper.waitForFirst();
 
-        // The current URL should not have changed in the existing tab, and the intent should have
-        // been launched.
-        Assert.assertEquals(url, mActivityTestRule.getLastCommittedUrlInTab(tab));
+        // (2) The intent should be launched in that tab...
+        intentInterceptor.waitForIntent();
         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());
 
-        // A new tab should have been created whose URL is that of the intent.
-        Browser browser = mActivityTestRule.getActivity().getBrowser();
+        // (3) And finally the new tab should be closed.
+        onTabRemovedCallbackHelper.waitForFirst();
+
+        // Now the original tab should be all that's left in the browser, with the display URL being
+        // the original URL.
         int numTabs =
                 TestThreadUtils.runOnUiThreadBlocking(() -> { return browser.getTabs().size(); });
-        Assert.assertEquals(2, numTabs);
-        Assert.assertEquals(INTENT_TO_CHROME_URL, mActivityTestRule.getCurrentDisplayUrl());
+        Assert.assertEquals(1, numTabs);
+        Assert.assertEquals(mActivityTestRule.getActivity().getTab(), originalTab);
+        Assert.assertEquals(url, mActivityTestRule.getCurrentDisplayUrl());
+
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { browser.unregisterTabListCallback(tabListCallback); });
     }
 
     /**

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

@@ -31,7 +31,6 @@ import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.EmbeddedTestServerRule;
 import org.chromium.weblayer.CookieManager;
-import org.chromium.weblayer.NavigationController;
 import org.chromium.weblayer.Tab;
 import org.chromium.weblayer.WebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
@@ -263,17 +262,6 @@ public class InstrumentationActivityTestRule extends ActivityTestRule<Instrument
         return getTestServer().getURL("/weblayer/test/data/" + path);
     }
 
-    // Returns the display URL of the last committed navigation entry in |tab|. Note that this will
-    // return an empty URL if there have been no committed navigations in |tab|.
-    public String getLastCommittedUrlInTab(Tab tab) {
-        return TestThreadUtils.runOnUiThreadBlockingNoException(() -> {
-            NavigationController navController = tab.getNavigationController();
-            return navController
-                    .getNavigationEntryDisplayUri(navController.getNavigationListCurrentIndex())
-                    .toString();
-        });
-    }
-
     // Returns the URL that is currently being displayed to the user.
     public String getCurrentDisplayUrl() {
         return getActivity().getCurrentDisplayUrl();

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

@@ -5,6 +5,7 @@
 package org.chromium.weblayer_private;
 
 import android.app.Activity;
+import android.os.RemoteException;
 import android.os.SystemClock;
 
 import org.chromium.base.ContextUtils;
@@ -118,7 +119,15 @@ public class InterceptNavigationDelegateClientImpl implements InterceptNavigatio
 
     @Override
     public void closeTab() {
-        // TODO(crbug.com/1031465): Bring up tab closing.
+        // Prior to 84 the client was not equipped to handle the case of WebLayer initiating the
+        // last tab being closed, so we simply short-circuit out here in that case.
+        if (WebLayerFactoryImpl.getClientMajorVersion() < 84) return;
+
+        try {
+            mTab.getBrowser().destroyTab(mTab);
+        } catch (RemoteException e) {
+            throw new RuntimeException(e);
+        }
     }
 
     @Override

weblayer/shell/android/shell_apk/src/org/chromium/weblayer/shell/InstrumentationActivity.java

@@ -27,6 +27,7 @@ import androidx.fragment.app.FragmentTransaction;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.weblayer.Browser;
+import org.chromium.weblayer.NavigationController;
 import org.chromium.weblayer.NewTabCallback;
 import org.chromium.weblayer.NewTabType;
 import org.chromium.weblayer.Profile;
@@ -36,6 +37,7 @@ import org.chromium.weblayer.TabListCallback;
 import org.chromium.weblayer.UnsupportedVersionException;
 import org.chromium.weblayer.WebLayer;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -63,6 +65,8 @@ public class InstrumentationActivity extends FragmentActivity {
     private IntentInterceptor mIntentInterceptor;
     private Bundle mSavedInstanceState;
     private TabCallback mTabCallback;
+    private TabListCallback mTabListCallback;
+    private List<Tab> mPreviousTabList = new ArrayList<>();
 
     private static boolean isJaCoCoEnabled() {
         // Nothing is set at runtime indicating jacoco is being used. This looks for the existence
@@ -194,6 +198,9 @@ public class InstrumentationActivity extends FragmentActivity {
             mTab.unregisterTabCallback(mTabCallback);
             mTabCallback = null;
         }
+        if (mTabListCallback != null) {
+            mBrowser.unregisterTabListCallback(mTabListCallback);
+        }
     }
 
     private void createWebLayerAsync() {
@@ -224,26 +231,58 @@ public class InstrumentationActivity extends FragmentActivity {
 
         mBrowser.setTopView(mTopContentsContainer);
 
-        if (mBrowser.getActiveTab() == null) {
-            assert mBrowser.getTabs().size() == 0;
-            // This happens with session restore enabled.
-            mBrowser.registerTabListCallback(new TabListCallback() {
-                @Override
-                public void onTabAdded(Tab tab) {
-                    if (mTab == null) {
-                        mBrowser.unregisterTabListCallback(this);
-                        setTab(tab);
+        mTabListCallback = new TabListCallback() {
+            @Override
+            public void onTabAdded(Tab tab) {
+                // The first tab can be added asynchronously with session restore enabled.
+                if (mTab == null) {
+                    setTab(tab);
+                }
+            }
+
+            @Override
+            public void onTabRemoved(Tab tab) {
+                mPreviousTabList.remove(tab);
+
+                if (mTab == tab) {
+                    Tab prevTab = null;
+                    if (!mPreviousTabList.isEmpty()) {
+                        prevTab = mPreviousTabList.remove(mPreviousTabList.size() - 1);
                     }
+
+                    setTab(prevTab);
                 }
-            });
+            }
+        };
+
+        mBrowser.registerTabListCallback(mTabListCallback);
+
+        if (mBrowser.getActiveTab() == null) {
+            // This happens with session restore enabled.
+            assert mBrowser.getTabs().size() == 0;
         } else {
             setTab(mBrowser.getActiveTab());
         }
     }
 
+    // Clears the state associated with |mTab| and sets |tab|, if non-null, as |mTab| and the
+    // active tab in the browser.
     private void setTab(Tab tab) {
-        assert mTab == null;
+        if (mTab != null) {
+            mTab.unregisterTabCallback(mTabCallback);
+            mTabCallback = null;
+            mTab = null;
+        }
+
         mTab = tab;
+
+        if (mTab == null) return;
+
+        // TODO(crbug.com/1066382): This will not be correct in the case where the initial
+        // navigation in |tab| was a failed navigation and there have been no more navigations since
+        // then.
+        mUrlView.setText(getLastCommittedUrlInTab(mTab));
+
         mTabCallback = new TabCallback() {
             @Override
             public void onVisibleUriChanged(Uri uri) {
@@ -255,22 +294,17 @@ public class InstrumentationActivity extends FragmentActivity {
         mTab.setNewTabCallback(new NewTabCallback() {
             @Override
             public void onNewTab(Tab newTab, @NewTabType int type) {
-                // NOTE: At this time there isn't a need to hang on to the previous tab as this
-                // activity doesn't support closing tabs. If needed that could be added following
-                // the implementation in WebLayerShellActivity.java.
-                mTab.unregisterTabCallback(mTabCallback);
-                mTabCallback = null;
-                mTab = null;
-
+                mPreviousTabList.add(mTab);
                 setTab(newTab);
-                mBrowser.setActiveTab(newTab);
             }
-
             @Override
             public void onCloseTab() {
                 assert false;
             }
         });
+
+        // Will be a no-op if this tab is already the active tab.
+        mBrowser.setActiveTab(mTab);
     }
 
     private Fragment getOrCreateBrowserFragment() {
@@ -307,6 +341,16 @@ public class InstrumentationActivity extends FragmentActivity {
         return fragment;
     }
 
+    // Returns the display URL of the last committed navigation entry in |tab|. This will
+    // return an empty URL if there have been no committed navigations in |tab|.
+    public String getLastCommittedUrlInTab(Tab tab) {
+        NavigationController navController = tab.getNavigationController();
+        int currentIndex = navController.getNavigationListCurrentIndex();
+        return currentIndex == -1
+                ? ""
+                : navController.getNavigationEntryDisplayUri(currentIndex).toString();
+    }
+
     public String getCurrentDisplayUrl() {
         return mUrlView.getText().toString();
     }