Ensure that pages with non-sticky reasons can enter BFCache twice.

BUG: When a page with a non-sticky reason navigates, it may clear that
reason in `pagehide`. If that page is restored and gets a non-sticky
reason again, clearing the reason in pagehide did not work the second
time.

CAUSE: When a page is navigating, we do a pre-pagehide check to see if
there are blocking reasons. This ignores non-sticky reasons. If there
ar e none, the page enters BFCache and we do a post-entry check to
make sure that there are no blocking reasons of any
kind. `PageLifecycleStateManager::did_receive_back_forward_cache_ack_`
tracks whether the page has fully entered BFCache and is used to
decide which kind of check we perform. This bit was not reset on
leaving BFCache so the pre-pagehide check was using the post-entry
criteria, second time around.

FIX: Reset `did_receive_back_forward_cache_ack_` on leaving BFCache.

This puts the fix behind a flag (default on) so that we can measure
the impact.

Bug: 360183659
Change-Id: Ie7d94e5d64b2b7a483a5d4c9bfb6d56582fe6615
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087162
Auto-Submit: Fergal Daly <fergal@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1395838}

content/browser/back_forward_cache_features_browsertest.cc

@@ -1767,35 +1767,77 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
   ExpectRestored(FROM_HERE);
 }
 
+class BackForwardCacheNonStickyDoubleFixBrowserTest
+    : public BackForwardCacheBrowserTest,
+      public testing::WithParamInterface<bool> {
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    if (IsBackForwardCacheNonStickyDoubleFixEnabled()) {
+      EnableFeatureAndSetParams(kBackForwardCacheNonStickyDoubleFix, "", "");
+    } else {
+      DisableFeature(kBackForwardCacheNonStickyDoubleFix);
+    }
+    BackForwardCacheBrowserTest::SetUpCommandLine(command_line);
+  }
+
+  bool IsBackForwardCacheNonStickyDoubleFixEnabled() { return GetParam(); }
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         BackForwardCacheNonStickyDoubleFixBrowserTest,
+                         testing::Bool());
+
 // If pages released keyboard lock during pagehide, they can enter
-// BackForwardCache.
-IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+// BackForwardCache. This also covers the case of entering BFCache for a
+// second time. KeyboardLock is a good feature to use as it will always
+// block BFCache. See https://crbug.com/360183659
+IN_PROC_BROWSER_TEST_P(BackForwardCacheNonStickyDoubleFixBrowserTest,
                        CacheIfKeyboardLockReleasedInPagehide) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  // 1) Navigate to a page and start using the Keyboard lock.
+  // Navigate to a page and start using the Keyboard lock.
   GURL url(embedded_test_server()->GetURL("/title1.html"));
   EXPECT_TRUE(NavigateToURL(shell(), url));
   RenderFrameHostImplWrapper rfh_a(current_frame_host());
 
   AcquireKeyboardLock(rfh_a.get());
   // Register a pagehide handler to release keyboard lock.
-  EXPECT_TRUE(ExecJs(rfh_a.get(), R"(
+  ASSERT_TRUE(ExecJs(rfh_a.get(), R"(
     window.onpagehide = function(e) {
       new Promise(resolve => {
-      navigator.keyboard.unlock();
-      resolve();
+        navigator.keyboard.unlock();
+        resolve();
       });
     };
   )"));
 
-  // 2) Navigate away.
-  EXPECT_TRUE(NavigateToURL(
+  // Navigate away.
+  ASSERT_TRUE(NavigateToURL(
       shell(), embedded_test_server()->GetURL("b.com", "/title1.html")));
 
-  // 3) Go back and page should be restored from BackForwardCache.
+  // Go back and page should be restored from BackForwardCache.
   ASSERT_TRUE(HistoryGoBack(web_contents()));
   ExpectRestored(FROM_HERE);
+
+  // Acquire the lock again.
+  AcquireKeyboardLock(rfh_a.get());
+
+  // Navigate away again.
+  ASSERT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("b.com", "/title1.html")));
+
+  // Go back again.
+  ASSERT_TRUE(HistoryGoBack(web_contents()));
+  if (IsBackForwardCacheNonStickyDoubleFixEnabled()) {
+    // The page should be restored from BackForwardCache.
+    ExpectRestored(FROM_HERE);
+  } else {
+    // The page should not be restored from BackForwardCache.
+    ExpectNotRestored(
+        {NotRestoredReason::kBlocklistedFeatures},
+        {blink::scheduler::WebSchedulerTrackedFeature::kKeyboardLock}, {}, {},
+        {}, FROM_HERE);
+  }
 }
 
 IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,

content/browser/renderer_host/page_lifecycle_state_manager.cc

@@ -94,6 +94,10 @@ void PageLifecycleStateManager::SetFrameTreeVisibility(
   // automatically resume.
 }
 
+BASE_FEATURE(kBackForwardCacheNonStickyDoubleFix,
+             "BackForwardCacheNonStickyDoubleFix",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 void PageLifecycleStateManager::SetIsInBackForwardCache(
     bool is_in_back_forward_cache,
     blink::mojom::PageRestoreParamsPtr page_restore_params) {
@@ -116,10 +120,14 @@ void PageLifecycleStateManager::SetIsInBackForwardCache(
     pagehide_dispatch_ = blink::mojom::PagehideDispatch::kDispatchedPersisted;
   } else {
     DCHECK(page_restore_params);
-    // When a page is restored from the back-forward cache, we should reset the
-    // |pagehide_dispatch_| state so that we'd dispatch the
-    // events again the next time we navigate away from the page.
+    // When a page is restored from the back-forward cache, we should reset this
+    // state so that it behaves correctly next time navigation occurs.
     pagehide_dispatch_ = blink::mojom::PagehideDispatch::kNotDispatched;
+    // TODO(https://crbug.com/360183659): Make this unconditional after
+    // measuring the impact.
+    if (base::FeatureList::IsEnabled(kBackForwardCacheNonStickyDoubleFix)) {
+      did_receive_back_forward_cache_ack_ = false;
+    }
   }
 
   SendUpdatesToRendererIfNeeded(std::move(page_restore_params),

content/browser/renderer_host/page_lifecycle_state_manager.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_RENDERER_HOST_PAGE_LIFECYCLE_STATE_MANAGER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_PAGE_LIFECYCLE_STATE_MANAGER_H_
 
+#include "base/feature_list.h"
 #include "base/functional/callback_forward.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -19,6 +20,10 @@ namespace content {
 
 class RenderViewHostImpl;
 
+// Enables the fix for a bug that prevents caching of pages for the second time.
+// TODO(https://crbug.com/360183659): Remove this after measuring the impact.
+CONTENT_EXPORT BASE_DECLARE_FEATURE(kBackForwardCacheNonStickyDoubleFix);
+
 // A class responsible for managing the main lifecycle state of the
 // `blink::Page` and communicating in to the `blink::WebView`. 1:1 with
 // `RenderViewHostImpl`.
@@ -103,6 +108,8 @@ class CONTENT_EXPORT PageLifecycleStateManager {
   // |frozen_explicitly_|.
   bool frozen_explicitly_ = false;
 
+  // TODO(https://crrev.com/c/6088660): Combine this and
+  // `did_receive_back_forward_cache_ack_` into a 3-state enum.
   bool is_in_back_forward_cache_ = false;
   bool eviction_enabled_ = false;
 

testing/variations/fieldtrial_testing_config.json

@@ -2926,6 +2926,26 @@
             ]
         }
     ],
+    "BackForwardCacheNonStickyDoubleFix": [
+        {
+            "platforms": [
+                "android",
+                "chromeos",
+                "ios",
+                "linux",
+                "mac",
+                "windows"
+            ],
+            "experiments": [
+                {
+                    "name": "Disabled",
+                    "disable_features": [
+                        "BackForwardCacheNonStickyDoubleFix"
+                    ]
+                }
+            ]
+        }
+    ],
     "BackForwardCacheNotRestoredReasons": [
         {
             "platforms": [