Stop unfreezing tabs "frozen" for lazy loads

When opening a tab in the background on Svelte devices, tabs are
marked as "frozen" without actually being frozen (no saved state).

* Make CompositorViewHolder load these tabs properly instead of
  trying to unfreeze them.

* Make Tab#unfreezeContents() protected so that nothing can handle
  this.

* Clear out pointers when destroying the objects and clean up
  destruction, in general.

* Stop destroying some TabObservers twice.

* Add a test to catch this.

* Moves process killing to the end of destroy() instead of the middle.

BUG=495877

Review URL: https://codereview.chromium.org/1337113007

Cr-Commit-Position: refs/heads/master@{#348755}

chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java

@@ -761,36 +761,50 @@ public abstract class ChromeActivity extends AsyncInitializationActivity
     @SuppressLint("NewApi")
     @Override
     protected final void onDestroy() {
-        if (mReaderModeActivityDelegate != null) mReaderModeActivityDelegate.destroy();
-        if (mContextualSearchManager != null) mContextualSearchManager.destroy();
-        if (mTabModelSelectorTabObserver != null) mTabModelSelectorTabObserver.destroy();
+        if (mReaderModeActivityDelegate != null) {
+            mReaderModeActivityDelegate.destroy();
+            mReaderModeActivityDelegate = null;
+        }
+
+        if (mContextualSearchManager != null) {
+            mContextualSearchManager.destroy();
+            mContextualSearchManager = null;
+        }
+
+        if (mTabModelSelectorTabObserver != null) {
+            mTabModelSelectorTabObserver.destroy();
+            mTabModelSelectorTabObserver = null;
+        }
+
         if (mCompositorViewHolder != null) {
             if (mCompositorViewHolder.getLayoutManager() != null) {
                 mCompositorViewHolder.getLayoutManager().removeSceneChangeObserver(this);
             }
             mCompositorViewHolder.shutDown();
+            mCompositorViewHolder = null;
         }
+
         onDestroyInternal();
 
+        if (mToolbarManager != null) {
+            mToolbarManager.destroy();
+            mToolbarManager = null;
+        }
+
         TabModelSelector selector = getTabModelSelector();
         if (selector != null) selector.destroy();
 
-        if (mWindowAndroid != null) mWindowAndroid.destroy();
-        if (!Locale.getDefault().equals(mCurrentLocale)) {
-            // This is a hack to relaunch renderer processes. Killing the main process also kills
-            // its dependent (renderer) processes, and Android's activity manager service seems to
-            // still relaunch the activity even when process dies in onDestroy().
-            // This shouldn't be moved to ChromeActivity since it may cause a crash if
-            // you kill the process from EmbedContentViewActivity since Preferences looks up
-            // ChildAccountManager#hasChildAccount() when it is not set.
-            // TODO(changwan): Implement a more generic and safe relaunch mechanism such as
-            // killing dependent processes on onDestroy and launching them at onCreate().
-            Log.w(TAG, "Forcefully killing process...");
-            Process.killProcess(Process.myPid());
+        if (mWindowAndroid != null) {
+            mWindowAndroid.destroy();
+            mWindowAndroid = null;
         }
+
         getChromeApplication().removePolicyChangeListener(this);
-        if (mTabContentManager != null) mTabContentManager.destroy();
-        if (mTabModelSelectorTabObserver != null) mTabModelSelectorTabObserver.destroy();
+
+        if (mTabContentManager != null) {
+            mTabContentManager.destroy();
+            mTabContentManager = null;
+        }
 
         AccessibilityManager manager = (AccessibilityManager)
                 getBaseContext().getSystemService(Context.ACCESSIBILITY_SERVICE);
@@ -798,7 +812,18 @@ public abstract class ChromeActivity extends AsyncInitializationActivity
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
             manager.removeTouchExplorationStateChangeListener(mTouchExplorationStateChangeListener);
         }
+
         super.onDestroy();
+
+        if (!Locale.getDefault().equals(mCurrentLocale)) {
+            // This is a hack to relaunch renderer processes. Killing the main process also kills
+            // its dependent (renderer) processes, and Android's activity manager service seems to
+            // still relaunch the activity even when process dies in onDestroy().
+            // TODO(changwan): Implement a more generic and safe relaunch mechanism, like killing
+            //                 dependent processes in onDestroy() and launching them at onCreate().
+            Log.w(TAG, "Forcefully killing process...");
+            Process.killProcess(Process.myPid());
+        }
     }
 
     /**
@@ -810,7 +835,6 @@ public abstract class ChromeActivity extends AsyncInitializationActivity
      * by the {@link WindowAndroid}.
      */
     protected void onDestroyInternal() {
-        if (mToolbarManager != null) mToolbarManager.destroy();
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java

@@ -1144,13 +1144,23 @@ public class ChromeTabbedActivity extends ChromeActivity implements OverviewMode
     @Override
     public void onDestroyInternal() {
         if (mLayoutManager != null) mLayoutManager.removeOverviewModeObserver(this);
-        if (mTabModelSelectorTabObserver != null) mTabModelSelectorTabObserver.destroy();
+
+        if (mTabModelSelectorTabObserver != null) {
+            mTabModelSelectorTabObserver.destroy();
+            mTabModelSelectorTabObserver = null;
+        }
+
         if (mTabModelObserver != null) {
             for (TabModel model : mTabModelSelectorImpl.getModels()) {
                 model.removeObserver(mTabModelObserver);
             }
         }
-        if (mUndoBarPopupController != null) mUndoBarPopupController.destroy();
+
+        if (mUndoBarPopupController != null) {
+            mUndoBarPopupController.destroy();
+            mUndoBarPopupController = null;
+        }
+
         super.onDestroyInternal();
     }
 

chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java

@@ -870,7 +870,7 @@ public class CompositorViewHolder extends FrameLayout
     }
 
     private void setTab(Tab tab) {
-        if (tab != null && tab.isFrozen()) tab.unfreezeContents();
+        if (tab != null) tab.loadIfNeeded();
 
         View newView = tab != null ? tab.getView() : null;
         if (mView == newView) return;

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

@@ -2021,10 +2021,11 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
     }
 
     /**
-     * Restores the WebContents from its saved state.
+     * Restores the WebContents from its saved state.  This should only be called if the tab is
+     * frozen with a saved TabState, and NOT if it was frozen for a lazy load.
      * @return Whether or not the restoration was successful.
      */
-    public boolean unfreezeContents() {
+    protected boolean unfreezeContents() {
         try {
             TraceEvent.begin("Tab.unfreezeContents");
             assert mFrozenContentsState != null;
@@ -2375,7 +2376,7 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
     /**
      * @return Parameters that should be used for a lazily loaded Tab.  May be null.
      */
-    protected LoadUrlParams getPendingLoadParams() {
+    private LoadUrlParams getPendingLoadParams() {
         return mPendingLoadParams;
     }
 

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelBase.java

@@ -398,7 +398,7 @@ public abstract class TabModelBase extends TabModelJniBridge {
         return currentTab != null ? currentTab.getId() : Tab.INVALID_TAB_ID;
     }
 
-    private boolean hasVaildTab() {
+    private boolean hasValidTab() {
         if (mTabs.size() <= 0) return false;
         for (int i = 0; i < mTabs.size(); i++) {
             if (!mTabs.get(i).isClosing()) return true;
@@ -417,7 +417,7 @@ public abstract class TabModelBase extends TabModelJniBridge {
                 mModelDelegate.selectModel(isIncognito());
             }
 
-            if (!hasVaildTab()) {
+            if (!hasValidTab()) {
                 mIndex = INVALID_TAB_INDEX;
             } else {
                 mIndex = MathUtils.clamp(i, 0, mTabs.size() - 1);

chrome/android/javatests/src/org/chromium/chrome/browser/TabsOpenedFromExternalAppTest.java

@@ -8,16 +8,26 @@ import android.content.Intent;
 import android.net.Uri;
 import android.provider.Browser;
 import android.test.FlakyTest;
+import android.test.suitebuilder.annotation.MediumTest;
 import android.text.TextUtils;
+import android.view.ContextMenu;
 import android.view.KeyEvent;
+import android.view.View;
 
 import junit.framework.Assert;
 
+import org.chromium.base.BaseSwitches;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tabmodel.TabModelUtils;
 import org.chromium.chrome.test.ChromeTabbedActivityTestBase;
+import org.chromium.chrome.test.MultiActivityTestBase;
+import org.chromium.chrome.test.util.ApplicationTestUtils;
 import org.chromium.chrome.test.util.ChromeTabUtils;
 import org.chromium.chrome.test.util.TestHttpServerClient;
 import org.chromium.content.browser.test.util.Criteria;
@@ -25,12 +35,13 @@ import org.chromium.content.browser.test.util.CriteriaHelper;
 import org.chromium.content.browser.test.util.DOMUtils;
 import org.chromium.content.browser.test.util.JavaScriptUtils;
 import org.chromium.content.browser.test.util.KeyUtils;
+import org.chromium.content.browser.test.util.TouchCommon;
 
+import java.util.concurrent.Callable;
 import java.util.concurrent.TimeoutException;
 
 /**
- * Test the behavior of tabs when opening a URL from an external app, more specifically how we reuse
- * tabs.
+ * Test the behavior of tabs when opening a URL from an external app.
  */
 public class TabsOpenedFromExternalAppTest extends ChromeTabbedActivityTestBase {
 
@@ -437,4 +448,99 @@ public class TabsOpenedFromExternalAppTest extends ChromeTabbedActivityTestBase
         assertEquals("Selected tab is not on the right URL.", url2,
                 getActivity().getActivityTab().getUrl());
     }
+
+
+    private static class TestTabObserver extends EmptyTabObserver {
+        private ContextMenu mContextMenu;
+
+        @Override
+        public void onContextMenuShown(Tab tab, ContextMenu menu) {
+            mContextMenu = menu;
+        }
+    }
+
+    /**
+     * Catches regressions for https://crbug.com/495877.
+     */
+    @MediumTest
+    @CommandLineFlags.Add(BaseSwitches.ENABLE_LOW_END_DEVICE_MODE)
+    public void testBackgroundSvelteTabIsSelectedAfterClosingExternalTab() throws Exception {
+        // Start up Chrome and immediately close its tab -- it gets in the way.
+        startMainActivityFromLauncher();
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                TabModelUtils.closeTabByIndex(getActivity().getCurrentTabModel(), 0);
+            }
+        });
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return getActivity().getTabModelSelector().getTotalTabCount() == 0;
+            }
+        }));
+
+        // Open a tab via an external application.
+        final Intent intent = new Intent(
+                Intent.ACTION_VIEW, Uri.parse(MultiActivityTestBase.HREF_LINK));
+        intent.setClassName(getInstrumentation().getTargetContext().getPackageName(),
+                ChromeTabbedActivity.class.getName());
+        intent.putExtra(Browser.EXTRA_APPLICATION_ID, "com.legit.totes");
+        intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
+        getInstrumentation().getTargetContext().startActivity(intent);
+
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return getActivity().getTabModelSelector().getTotalTabCount() == 1;
+            }
+        }));
+        ApplicationTestUtils.assertWaitForPageScaleFactorMatch(getActivity(), 0.5f, false);
+
+        // Long press the center of the page, which should bring up the context menu.
+        final TestTabObserver observer = new TestTabObserver();
+        getActivity().getActivityTab().addObserver(observer);
+        assertNull(observer.mContextMenu);
+        final View view = ThreadUtils.runOnUiThreadBlocking(new Callable<View>() {
+            @Override
+            public View call() throws Exception {
+                return getActivity().getActivityTab().getContentViewCore().getContainerView();
+            }
+        });
+        TouchCommon.longPressView(view, view.getWidth() / 2, view.getHeight() / 2);
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return observer.mContextMenu != null;
+            }
+        }));
+        getActivity().getActivityTab().removeObserver(observer);
+
+        // Select the "open in new tab" option.
+        ThreadUtils.runOnUiThreadBlocking(new Runnable() {
+            @Override
+            public void run() {
+                assertTrue(observer.mContextMenu.performIdentifierAction(
+                        R.id.contextmenu_open_in_new_tab, 0));
+            }
+        });
+
+        // The second tab should open in the background.
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return getActivity().getTabModelSelector().getTotalTabCount() == 2;
+            }
+        }));
+
+        // Hitting "back" should close the tab, minimize Chrome, and select the background tab.
+        // Confirm that the number of tabs is correct and that closing the tab didn't cause a crash.
+        getActivity().onBackPressed();
+        assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
+            @Override
+            public boolean isSatisfied() {
+                return getActivity().getTabModelSelector().getTotalTabCount() == 1;
+            }
+        }));
+    }
 }

chrome/test/android/javatests/src/org/chromium/chrome/test/MultiActivityTestBase.java

@@ -46,7 +46,7 @@ public abstract class MultiActivityTestBase extends InstrumentationTestCase
     private Map<String, BaseParameter> mAvailableParameters;
 
     /** Defines one gigantic link spanning the whole page that creates a new window with URL_4. */
-    protected static final String HREF_LINK = UrlUtils.encodeHtmlDataUri(
+    public static final String HREF_LINK = UrlUtils.encodeHtmlDataUri(
             "<html>"
             + "  <head>"
             + "    <title>href link page</title>"