Prevent crash when opening incognito link in bottom sheet

This change adds a TabModelSelectorObserver for the bottom sheet to
observe tab model changed. If the tab model changes, the sheet is
hidden. The sheet also no longer tries to peek when loading a URL
since 'peek' is no longer the minimum state. This patch also
includes a change to the open/close events which are now driven off
of the sheet's height rather than its state. This prevents a crash
where a URL is loaded and the sheet closes and immediately tries to
re-peek.

Bug: 832814
Change-Id: I41c88599b55a2b763dbaa94adca2117594b88f2a
Reviewed-on: https://chromium-review.googlesource.com/1014712
Commit-Queue: Theresa <twellington@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551786}

chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java

@@ -636,7 +636,7 @@ public class BottomSheet extends FrameLayout
                 if (getCurrentOffsetPx() > getSheetHeightForState(SHEET_STATE_PEEK)) return;
 
                 // Updating the offset will automatically account for the browser controls.
-                setSheetOffsetFromBottom(getCurrentOffsetPx());
+                setSheetOffsetFromBottom(getCurrentOffsetPx(), StateChangeReason.SWIPE);
             }
 
             @Override
@@ -673,9 +673,6 @@ public class BottomSheet extends FrameLayout
                     params, TabModel.TabLaunchType.FROM_CHROME_UI, getActiveTab(), incognito);
         }
 
-        // In all non-native cases, minimize the sheet.
-        setSheetState(SHEET_STATE_PEEK, true, StateChangeReason.NAVIGATION);
-
         return tabLoadStatus;
     }
 
@@ -1018,7 +1015,7 @@ public class BottomSheet extends FrameLayout
         mSettleAnimator.addUpdateListener(new ValueAnimator.AnimatorUpdateListener() {
             @Override
             public void onAnimationUpdate(ValueAnimator animator) {
-                setSheetOffsetFromBottom((Float) animator.getAnimatedValue());
+                setSheetOffsetFromBottom((Float) animator.getAnimatedValue(), reason);
             }
         });
 
@@ -1040,7 +1037,7 @@ public class BottomSheet extends FrameLayout
      * Sets the sheet's offset relative to the bottom of the screen.
      * @param offset The offset that the sheet should be.
      */
-    private void setSheetOffsetFromBottom(float offset) {
+    private void setSheetOffsetFromBottom(float offset, @StateChangeReason int reason) {
         mCurrentOffsetPx = offset;
 
         // The browser controls offset is added here so that the sheet's toolbar behaves like the
@@ -1050,6 +1047,15 @@ public class BottomSheet extends FrameLayout
         if (MathUtils.areFloatsEqual(translationY, getTranslationY())) return;
 
         setTranslationY(translationY);
+
+        float peekHeight = getSheetHeightForState(SHEET_STATE_PEEK);
+        boolean isAtPeekingHeight = MathUtils.areFloatsEqual(getCurrentOffsetPx(), peekHeight);
+        if (isSheetOpen() && (getCurrentOffsetPx() < peekHeight || isAtPeekingHeight)) {
+            onSheetClosed(reason);
+        } else if (!isSheetOpen() && getCurrentOffsetPx() > peekHeight) {
+            onSheetOpened(reason);
+        }
+
         sendOffsetChangeEvents();
     }
 
@@ -1069,7 +1075,7 @@ public class BottomSheet extends FrameLayout
         } else {
             setInternalCurrentState(
                     BottomSheet.SHEET_STATE_SCROLLING, BottomSheet.StateChangeReason.SWIPE);
-            setSheetOffsetFromBottom(offset);
+            setSheetOffsetFromBottom(offset, BottomSheet.StateChangeReason.SWIPE);
         }
     }
 
@@ -1086,12 +1092,13 @@ public class BottomSheet extends FrameLayout
     }
 
     /**
-     * This is the same as {@link #setSheetOffsetFromBottom(float)} but exclusively for testing.
+     * This is the same as {@link #setSheetOffsetFromBottom(float, int)} but exclusively for
+     * testing.
      * @param offset The offset to set the sheet to.
      */
     @VisibleForTesting
     public void setSheetOffsetFromBottomForTesting(float offset) {
-        setSheetOffsetFromBottom(offset);
+        setSheetOffsetFromBottom(offset, StateChangeReason.NONE);
     }
 
     /**
@@ -1212,7 +1219,7 @@ public class BottomSheet extends FrameLayout
         if (animate && state != mCurrentState) {
             createSettleAnimation(state, reason);
         } else {
-            setSheetOffsetFromBottom(getSheetHeightForState(state));
+            setSheetOffsetFromBottom(getSheetHeightForState(state), reason);
             setInternalCurrentState(mTargetState, reason);
             mTargetState = SHEET_STATE_NONE;
         }
@@ -1259,8 +1266,6 @@ public class BottomSheet extends FrameLayout
             return;
         }
 
-        @SheetState
-        final int previousState = mCurrentState;
         mCurrentState = state;
 
         if (mCurrentState == SHEET_STATE_HALF || mCurrentState == SHEET_STATE_FULL) {
@@ -1282,30 +1287,6 @@ public class BottomSheet extends FrameLayout
         for (BottomSheetObserver o : mObservers) {
             o.onSheetStateChanged(mCurrentState);
         }
-
-        if (isSheetOpen() && isClosedState(getSheetState())) {
-            onSheetClosed(reason);
-        } else if (!isSheetOpen() && isClosedState(previousState)
-                && isOpenedState(getSheetState())) {
-            onSheetOpened(reason);
-        }
-    }
-
-    /**
-     * @param state A state of the {@link BottomSheet}.
-     * @return Whether the provided state is considered open.
-     */
-    private boolean isOpenedState(@SheetState int state) {
-        return state == SHEET_STATE_HALF || state == SHEET_STATE_FULL
-                || state == SHEET_STATE_SCROLLING;
-    }
-
-    /**
-     * @param state A state of the {@link BottomSheet}.
-     * @return Whether the provided state is considered closed.
-     */
-    private boolean isClosedState(@SheetState int state) {
-        return state == SHEET_STATE_PEEK || state == SHEET_STATE_HIDDEN;
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java

@@ -20,7 +20,9 @@ import org.chromium.chrome.browser.contextualsearch.ContextualSearchObserver;
 import org.chromium.chrome.browser.gsa.GSAContextDisplaySelection;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tabmodel.TabModel;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
+import org.chromium.chrome.browser.tabmodel.TabModelSelectorObserver;
 import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabObserver;
 import org.chromium.chrome.browser.widget.FadingBackgroundView;
 import org.chromium.chrome.browser.widget.FadingBackgroundView.FadingViewObserver;
@@ -109,6 +111,22 @@ public class BottomSheetController implements ApplicationStatus.ActivityStateLis
             }
         };
 
+        tabModelSelector.addObserver(new TabModelSelectorObserver() {
+            @Override
+            public void onChange() {}
+
+            @Override
+            public void onNewTabCreated(Tab tab) {}
+
+            @Override
+            public void onTabModelSelected(TabModel newModel, TabModel oldModel) {
+                clearRequestsAndHide();
+            }
+
+            @Override
+            public void onTabStateInitialized() {}
+        });
+
         // If the layout changes (to tab switcher, toolbar swipe, etc.) hide the sheet.
         mLayoutManager.addSceneChangeObserver(new SceneChangeObserver() {
             @Override

chrome/android/javatests/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsTest.java

@@ -38,10 +38,12 @@ import org.chromium.chrome.test.util.browser.ChromeModernDesign;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 import org.chromium.chrome.test.util.browser.RecyclerViewTestUtils;
+import org.chromium.content.browser.test.util.TestWebContentsObserver;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.ui.test.util.UiRestriction;
 
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeoutException;
 
 /**
  * Tests related to displaying contextual suggestions in a bottom sheet.
@@ -175,18 +177,26 @@ public class ContextualSuggestionsTest {
     @Test
     @MediumTest
     @Feature({"ContextualSuggestions"})
-    public void testOpenSuggestion() {
+    public void testOpenSuggestion() throws InterruptedException, TimeoutException {
         forceShowSuggestions();
         openSheet();
 
         SnippetArticleViewHolder holder = getFirstSuggestionViewHolder();
         String expectedUrl = holder.getUrl();
 
+        TestWebContentsObserver webContentsObserver = new TestWebContentsObserver(
+                mActivityTestRule.getActivity().getActivityTab().getWebContents());
+
+        int callCount = webContentsObserver.getOnPageStartedHelper().getCallCount();
+
         ThreadUtils.runOnUiThreadBlocking(() -> {
             holder.itemView.performClick();
-            mBottomSheet.endAnimations();
         });
 
+        webContentsObserver.getOnPageStartedHelper().waitForCallback(callCount);
+
+        ThreadUtils.runOnUiThreadBlocking(() -> mBottomSheet.endAnimations());
+
         assertEquals("Tab URL should match snippet URL", expectedUrl,
                 mActivityTestRule.getActivity().getActivityTab().getUrl());
         assertFalse("Sheet should be closed.", mBottomSheet.isSheetOpen());