From 2a80258ed798263eeff7ca49f3c13d389eb87df8 Mon Sep 17 00:00:00 2001
From: Sky Malice <[email protected]>
Date: Tue, 26 Nov 2024 22:13:00 +0000
Subject: [PATCH] Rework locking controls from navigations.

Page load and navigation events are not guaranteed to be delivered for one navigation at a time. Instead they can be interleaved. This is causing a problem when a slow navigation starts before the previous navigation is completed. After the 3 second wait, we erroneously unlocked the browser controls when they should not be.

To fix this, all page load events were switched to navigation events. And we now track navigation ids in a set, and only unlock when the set is empty. Special casing for same document navigations was added to handle the difference between navigation events and page load events. This approach has the potential danger of leaving browser controls locked forever if we miss an event.

To mitigate this risk, this CL adds a kill switch and a histogram that counts the number of outstanding navigations when a navigation completes.

Bug: 379652406
Change-Id: I6a837b7f7b2103c6f811e130bf582ac38c66c763
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6040200
Commit-Queue: Sky Malice <[email protected]>
Reviewed-by: Patrick Noland <[email protected]>
Reviewed-by: Sinan Sahin <[email protected]>
Reviewed-by: Yaron Friedman <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1388487}
---

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java
index ba6186e..68a99aa 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegate.java
@@ -10,8 +10,10 @@
 
 import androidx.annotation.Nullable;
 
+import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.cc.input.BrowserControlsState;
 import org.chromium.chrome.browser.device.DeviceClassManager;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.util.ChromeAccessibilityUtil;
 import org.chromium.components.browser_ui.util.BrowserControlsVisibilityDelegate;
 import org.chromium.components.dom_distiller.core.DomDistillerUrlUtils;
@@ -24,6 +26,9 @@
 import org.chromium.ui.base.WindowAndroid;
 import org.chromium.url.GURL;
 
+import java.util.HashSet;
+import java.util.Set;
+
 /**
  * Determines the desired visibility of the browser controls based on the current state of a given
  * tab.
@@ -43,8 +48,11 @@
     private boolean mIsFullscreenWaitingForLoad;
     private boolean mIsFocusedNodeEditable;
 
+    private final Set<Long> mOutstandingNavigations = new HashSet<>();
+
     /**
      * Basic constructor.
+     *
      * @param tab The associated {@link Tab}.
      */
     public TabStateBrowserControlsVisibilityDelegate(Tab tab) {
@@ -100,25 +108,65 @@
                     }
 
                     @Override
+                    public void onDidStartNavigationInPrimaryMainFrame(
+                            Tab tab, NavigationHandle navigation) {
+                        if (!ChromeFeatureList.sControlsVisibilityFromNavigations.isEnabled()) {
+                            return;
+                        }
+
+                        if (navigation.isSameDocument()) return;
+
+                        boolean changed = mOutstandingNavigations.add(navigation.getNavigationId());
+                        RecordHistogram.recordBooleanHistogram(
+                                "Android.BrowserControls.OutstandingChangedOnStart", changed);
+
+                        mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
+                        boolean safe = DomDistillerUrlUtils.isDistilledPage(navigation.getUrl());
+                        updateWaitingForLoad(!safe);
+                    }
+
+                    @Override
                     public void onDidFinishNavigationInPrimaryMainFrame(
                             Tab tab, NavigationHandle navigation) {
-                        if (!navigation.hasCommitted()) return;
-                        mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
-                        mHandler.sendEmptyMessageDelayed(
-                                MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD, getLoadDelayMs());
+                        if (ChromeFeatureList.sControlsVisibilityFromNavigations.isEnabled()) {
+                            if (navigation.isSameDocument()) return;
+
+                            boolean changed =
+                                    mOutstandingNavigations.remove(navigation.getNavigationId());
+                            RecordHistogram.recordBooleanHistogram(
+                                    "Android.BrowserControls.OutstandingChangedOnFinish", changed);
+
+                            if (mOutstandingNavigations.isEmpty()) {
+                                mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
+                                mHandler.sendEmptyMessageDelayed(
+                                        MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD, getLoadDelayMs());
+                            }
+                            RecordHistogram.recordCount100Histogram(
+                                    "Android.BrowserControls.OutstandingNavigationsOnFinish",
+                                    mOutstandingNavigations.size());
+                        } else {
+                            if (!navigation.hasCommitted()) return;
+                            mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
+                            mHandler.sendEmptyMessageDelayed(
+                                    MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD, getLoadDelayMs());
+                        }
                     }
 
                     @Override
                     public void onPageLoadStarted(Tab tab, GURL url) {
-                        mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
-                        updateWaitingForLoad(!DomDistillerUrlUtils.isDistilledPage(url));
+                        if (!ChromeFeatureList.sControlsVisibilityFromNavigations.isEnabled()) {
+                            mHandler.removeMessages(MSG_ID_ENABLE_FULLSCREEN_AFTER_LOAD);
+                            updateWaitingForLoad(!DomDistillerUrlUtils.isDistilledPage(url));
+                        }
                     }
 
                     @Override
                     public void onPageLoadFinished(Tab tab, GURL url) {
                         // Handle the case where a commit or prerender swap notification failed to
                         // arrive and the enable fullscreen message was never enqueued.
-                        scheduleEnableFullscreenLoadDelayIfNecessary();
+                        if (!ChromeFeatureList.sControlsVisibilityFromNavigations.isEnabled()) {
+                            scheduleEnableFullscreenLoadDelayIfNecessary();
+                        }
                     }
 
                     @Override
@@ -127,7 +175,9 @@
                         // urls, so that we can fully unlock controls here possible here.
                         // May have already received the start of a different navigation. Do not
                         // cancel the outstanding delay. See https://crbug.com/1447237.
-                        scheduleEnableFullscreenLoadDelayIfNecessary();
+                        if (!ChromeFeatureList.sControlsVisibilityFromNavigations.isEnabled()) {
+                            scheduleEnableFullscreenLoadDelayIfNecessary();
+                        }
                     }
 
                     @Override
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegateTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegateTest.java
index 761cc43..83aa649e 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegateTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/tab/TabStateBrowserControlsVisibilityDelegateTest.java
@@ -8,8 +8,6 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-import androidx.test.filters.SmallTest;
-
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -26,6 +24,7 @@
 import org.robolectric.shadows.ShadowSystemClock;
 
 import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.base.test.util.Features.DisableFeatures;
 import org.chromium.cc.input.BrowserControlsState;
 import org.chromium.components.security_state.SecurityStateModel;
 import org.chromium.components.security_state.SecurityStateModelJni;
@@ -61,13 +60,20 @@
     }
 
     @Test
-    @SmallTest
+    @DisableFeatures("ControlsVisibilityFromNavigations")
     public void testOnPageLoadFailedDuringNavigation() {
         // Inspired by https://crbug.com/1447237.
         GURL blueGurl = JUnitTestGURLs.BLUE_1;
         GURL redGurl = JUnitTestGURLs.RED_1;
         when(mTabImpl.getUrl()).thenReturn(blueGurl);
 
+        when(mNavigationHandle1.getNavigationId()).thenReturn(1L);
+        when(mNavigationHandle1.getUrl()).thenReturn(blueGurl);
+        when(mNavigationHandle2.getNavigationId()).thenReturn(2L);
+        when(mNavigationHandle2.getUrl()).thenReturn(blueGurl);
+        when(mNavigationHandle3.getNavigationId()).thenReturn(3L);
+        when(mNavigationHandle3.getUrl()).thenReturn(redGurl);
+
         TabStateBrowserControlsVisibilityDelegate controlsVisibilityDelegate =
                 new TabStateBrowserControlsVisibilityDelegate(mTabImpl);
         verify(mTabImpl).addObserver(mTabObserverCaptor.capture());
@@ -102,4 +108,93 @@
                 BrowserControlsState.BOTH,
                 controlsVisibilityDelegate.calculateVisibilityConstraints());
     }
+
+    @Test
+    public void sameDocumentNavigationsIgnored() {
+        when(mTabImpl.getUrl()).thenReturn(JUnitTestGURLs.BLUE_1);
+        when(mNavigationHandle1.getNavigationId()).thenReturn(1L);
+        when(mNavigationHandle1.getUrl()).thenReturn(JUnitTestGURLs.BLUE_1);
+        when(mNavigationHandle1.isSameDocument()).thenReturn(true);
+
+        TabStateBrowserControlsVisibilityDelegate controlsVisibilityDelegate =
+                new TabStateBrowserControlsVisibilityDelegate(mTabImpl);
+        verify(mTabImpl).addObserver(mTabObserverCaptor.capture());