weblayer: reorder tab calls

This patch changes the order of calls to the client when a Tab is
destroyed. Prior to this patch the order was onTabDestroyed then
onTabRemoved. This patch makes the order onTabRemoved() then
onTabDestroyed(). The order was like this at one point, but
without tests it regressed. This adds a test to ensure that doesn't
happen again.

Calling onTabDestroyed() first is problematic as it means client code
can't call any methods on Tab as it's been destroyed, which is error
prone and surprising.

This also ensures calling Tab.destroy() removes the Tab from the
internal map in the client library. This previously would not happen
if Browser.destroyTab() was called.

BUG=none
TEST=covered by tests

Change-Id: Ic3e38cee0eee70ef9b2cfced83deca71729fdd5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411161Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807098}

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

@@ -11,10 +11,12 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+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.WebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
 import java.util.ArrayList;
@@ -137,7 +139,7 @@ public class TabListCallbackTest {
 
     @Test
     @SmallTest
-    public void testDispose() {
+    public void testDestroyTab() {
         initialize("new_browser.html");
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
@@ -146,6 +148,7 @@ public class TabListCallbackTest {
             browser.registerTabListCallback(callback);
             browser.destroyTab(mActivity.getBrowser().getActiveTab());
             Assert.assertTrue(callback.getObservedValues().contains(TabListCallbackImpl.ACTIVE));
+            Assert.assertTrue(callback.getObservedValues().contains(TabListCallbackImpl.REMOVED));
             Assert.assertEquals(1, browser.getTabs().size());
         });
     }
@@ -166,4 +169,28 @@ public class TabListCallbackTest {
         EventUtils.simulateTouchCenterOfView(mActivity.getWindow().getDecorView());
         closeTabCallback.waitForCloseTab();
     }
+
+    @Test
+    @SmallTest
+    public void testOnTabRemoved() throws Exception {
+        mActivity = mActivityTestRule.launchShellWithUrl("about:blank");
+        CallbackHelper callbackHelper = new CallbackHelper();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            Browser browser = mActivity.getBrowser();
+            browser.registerTabListCallback(new TabListCallback() {
+                @Override
+                public void onTabRemoved(Tab tab) {
+                    if (WebLayer.getSupportedMajorVersion(mActivity) >= 87) {
+                        // |tab| should not be destroyed at this point. getGuid() is a good proxy
+                        // for verifying the tab hasn't been destroyed. Prior to 87 the tab was
+                        // destroyed at this point.
+                        tab.getGuid();
+                    }
+                    callbackHelper.notifyCalled();
+                }
+            });
+            mActivity.getBrowser().destroyTab(mActivity.getBrowser().createTab());
+        });
+        callbackHelper.waitForFirst();
+    }
 }

weblayer/browser/browser_impl.cc

@@ -145,12 +145,6 @@ void BrowserImpl::AddTab(JNIEnv* env,
   AddTab(reinterpret_cast<TabImpl*>(native_tab));
 }
 
-void BrowserImpl::RemoveTab(JNIEnv* env,
-                            long native_tab) {
-  // The Java side owns the Tab.
-  RemoveTab(reinterpret_cast<TabImpl*>(native_tab)).release();
-}
-
 ScopedJavaLocalRef<jobjectArray> BrowserImpl::GetTabs(JNIEnv* env) {
   ScopedJavaLocalRef<jclass> clazz =
       base::android::GetClass(env, "org/chromium/weblayer_private/TabImpl");
@@ -272,8 +266,10 @@ void BrowserImpl::SetWebPreferences(blink::web_pref::WebPreferences* prefs) {
 }
 
 #if defined(OS_ANDROID)
-void BrowserImpl::DestroyTabFromJava(Tab* tab) {
-  RemoveTab(tab);
+void BrowserImpl::RemoveTabBeforeDestroyingFromJava(Tab* tab) {
+  // The java side owns the Tab, and is going to delete it shortly. See
+  // JNI_TabImpl_DeleteTab.
+  RemoveTab(tab).release();
 }
 #endif
 
@@ -290,6 +286,7 @@ void BrowserImpl::AddTab(Tab* tab) {
 
 void BrowserImpl::DestroyTab(Tab* tab) {
 #if defined(OS_ANDROID)
+  // Route destruction through the java side.
   Java_BrowserImpl_destroyTabImpl(AttachCurrentThread(), java_impl_,
                                   static_cast<TabImpl*>(tab)->GetJavaTab());
 #else

weblayer/browser/browser_impl.h

@@ -62,8 +62,6 @@ class BrowserImpl : public Browser {
 
   void AddTab(JNIEnv* env,
               long native_tab);
-  void RemoveTab(JNIEnv* env,
-                 long native_tab);
   base::android::ScopedJavaLocalRef<jobjectArray> GetTabs(JNIEnv* env);
   void SetActiveTab(JNIEnv* env,
                     long native_tab);
@@ -105,9 +103,9 @@ class BrowserImpl : public Browser {
 #if defined(OS_ANDROID)
   // On Android the Java Tab class owns the C++ Tab. DestroyTab() calls to the
   // Java Tab class to initiate deletion. This function is called from the Java
-  // side, and must not call DestroyTab(), otherwise we get stuck in infinite
-  // recursion.
-  void DestroyTabFromJava(Tab* tab);
+  // side to remove the tab from the browser and shortly followed by deleting
+  // the tab.
+  void RemoveTabBeforeDestroyingFromJava(Tab* tab);
 #endif
 
   // Browser:

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

@@ -572,7 +572,6 @@ public class BrowserImpl extends IBrowser.Stub implements View.OnAttachStateChan
         long createBrowser(long profile, BrowserImpl caller);
         void deleteBrowser(long browser);
         void addTab(long nativeBrowserImpl, long nativeTab);
-        void removeTab(long nativeBrowserImpl, long nativeTab);
         TabImpl[] getTabs(long nativeBrowserImpl);
         void setActiveTab(long nativeBrowserImpl, long nativeTab);
         TabImpl getActiveTab(long nativeBrowserImpl);

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

@@ -854,6 +854,8 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         // Ensure that this method isn't called twice.
         assert mInterceptNavigationDelegate != null;
 
+        TabImplJni.get().removeTabFromBrowserBeforeDestroying(mNativeTab);
+
         if (WebLayerFactoryImpl.getClientMajorVersion() >= 84) {
             // Notify the client that this instance is being destroyed to prevent it from calling
             // back into this object if the embedder mistakenly tries to do so.
@@ -1061,13 +1063,14 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
     @NativeMethods
     interface Natives {
         TabImpl fromWebContents(WebContents webContents);
-        long createTab(long profile, TabImpl caller);
+        long createTab(long tab, TabImpl caller);
+        void removeTabFromBrowserBeforeDestroying(long nativeTabImpl);
+        void deleteTab(long tab);
         void setJavaImpl(long nativeTabImpl, TabImpl impl);
         void onAutofillProviderChanged(long nativeTabImpl, AutofillProvider autofillProvider);
         void setBrowserControlsContainerViews(long nativeTabImpl,
                 long nativeTopBrowserControlsContainerView,
                 long nativeBottomBrowserControlsContainerView);
-        void deleteTab(long tab);
         WebContents getWebContents(long nativeTabImpl);
         void executeScript(long nativeTabImpl, String script, boolean useSeparateIsolate,
                 Callback<String> callback);

weblayer/browser/tab_impl.cc

@@ -580,9 +580,10 @@ static jlong JNI_TabImpl_CreateTab(JNIEnv* env,
 static void JNI_TabImpl_DeleteTab(JNIEnv* env, jlong tab) {
   TabImpl* tab_impl = reinterpret_cast<TabImpl*>(tab);
   DCHECK(tab_impl);
-  DCHECK(tab_impl->browser());
-  // Don't call Browser::DestroyTab() as it calls back to the java side.
-  tab_impl->browser()->DestroyTabFromJava(tab_impl);
+  // RemoveTabBeforeDestroyingFromJava() should have been called before this,
+  // which sets browser to null.
+  DCHECK(!tab_impl->browser());
+  delete tab_impl;
 }
 
 ScopedJavaLocalRef<jobject> TabImpl::GetWebContents(JNIEnv* env) {
@@ -808,6 +809,11 @@ void TabImpl::ShowTranslateUi(JNIEnv* env) {
       ->ManualTranslateWhenReady();
 }
 
+void TabImpl::RemoveTabFromBrowserBeforeDestroying(JNIEnv* env) {
+  DCHECK(browser_);
+  browser_->RemoveTabBeforeDestroyingFromJava(this);
+}
+
 void TabImpl::SetTranslateTargetLanguage(
     JNIEnv* env,
     const base::android::JavaParamRef<jstring>& translate_target_lang) {

weblayer/browser/tab_impl.h

@@ -198,6 +198,7 @@ class TabImpl : public Tab,
       const base::android::JavaParamRef<jstring>& js_object_name);
   jboolean CanTranslate(JNIEnv* env);
   void ShowTranslateUi(JNIEnv* env);
+  void RemoveTabFromBrowserBeforeDestroying(JNIEnv* env);
   void SetTranslateTargetLanguage(
       JNIEnv* env,
       const base::android::JavaParamRef<jstring>& translate_target_lang);

weblayer/public/java/org/chromium/weblayer/Browser.java

@@ -85,6 +85,9 @@ public class Browser {
 
     // Called prior to notifying IBrowser of destroy().
     void prepareForDestroy() {
+        // See comment in Tab$TabClientImpl.onTabDestroyed for details on this.
+        if (WebLayer.getSupportedMajorVersionInternal() >= 87) return;
+
         for (Tab tab : getTabs()) {
             Tab.unregisterTab(tab);
         }

weblayer/public/java/org/chromium/weblayer/Tab.java

@@ -740,6 +740,13 @@ public class Tab {
 
         @Override
         public void onTabDestroyed() {
+            // Prior to 87 this was called *before* onTabRemoved(). Post 87 this is called after
+            // onTabRemoved(). onTabRemoved() needs the Tab to be registered, so unregisterTab()
+            // should only be called in >= 87 (in < 87 it is called from
+            // Browser.prepareForDestroy()).
+            if (WebLayer.getSupportedMajorVersionInternal() >= 87) {
+                unregisterTab(Tab.this);
+            }
             // Ensure that the app will fail fast if the embedder mistakenly tries to call back
             // into the implementation via this Tab.
             mImpl = null;