[bfache] Disable CredentialManagerBrowserTest tests that expect unload.

As part of this CL, make DisableForTesting part of public interface,
so that tests outside of content/ can disable the BackForwardCache.

TBR=thakis@chromium.org

Change-Id: Iaa6265b37e3406e386b1e002c70d219d45d84629
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1813349
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698865}

chrome/browser/password_manager/credential_manager_browsertest.cc

@@ -19,6 +19,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/test_password_store.h"
+#include "content/public/browser/back_forward_cache.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_features.h"
@@ -102,6 +103,9 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
   // the call to store() triggered from the unload handler.
   void TestStoreInUnloadHandlerForSameSiteNavigation(
       bool preestablish_mojo_pipe) {
+    WebContents()->GetController().GetBackForwardCache().DisableForTesting(
+        content::BackForwardCache::TEST_USES_UNLOAD_EVENT);
+
     // Use URLs that differ on subdomains so we can tell which one was used for
     // saving, but they still belong to the same SiteInstance, so they will be
     // renderered in the same RenderFrame (in the same process).

content/browser/frame_host/back_forward_cache_impl.h

@@ -114,56 +114,6 @@ class CONTENT_EXPORT BackForwardCacheImpl : public BackForwardCache {
     task_runner_for_testing_ = task_runner;
   }
 
-  // List of reasons the BackForwardCache was disabled for a specific test. If a
-  // test needs to be disabled for a reason not covered below, please add to
-  // this enum.
-  enum DisableForTestingReason {
-    // The test has expectations that won't make sense if caching is enabled.
-    //
-    // One alternative to disabling the test is to make the test's logic
-    // conditional, based on whether or not BackForwardCache is enabled.
-    //
-    // You should also consider whether it would make sense to instead
-    // split into two tests, one using a cacheable page, and one using an
-    // uncacheable page.
-    //
-    // Once BackForwardCache is enabled everywhere, any tests still disabled for
-    // this reason should change their expectations to permanently match the
-    // BackForwardCache enabled behavior.
-    TEST_ASSUMES_NO_CACHING,
-
-    // Unload events never fire for documents that are put into the
-    // BackForwardCache. This is by design, as there is never an appropriate
-    // moment to fire unload if the document is cached.
-    // In short, this is because:
-    //
-    // * We can't fire unload when going into the cache, because it may be
-    // destructive, and put the document into an unknown/bad state. Pages can
-    // also be cached and restored multiple times, and we don't want to invoke
-    // unload more than once.
-    //
-    // * We can't fire unload when the document is evicted from the cache,
-    // because at that point we don't want to run javascript for privacy and
-    // security reasons.
-    //
-    // An alternative to disabling the test, is to have the test load a page
-    // that is ineligible for caching (e.g. due to an unsupported feature).
-    TEST_USES_UNLOAD_EVENT,
-  };
-
-  // Disables the BackForwardCache so that no documents will be stored/served.
-  // This allows tests to "force" not using the BackForwardCache, this can be
-  // useful when:
-  // * Tests rely on a new document being loaded.
-  // * Tests want to test this case specifically.
-  // Callers should pass an accurate |reason| to make future triaging of
-  // disabled tests easier.
-  //
-  // Note: It's preferable to make tests BackForwardCache compatible
-  // when feasible, rather than using this method. Also please consider whether
-  // you actually should have 2 tests, one with the document cached
-  // (BackForwardCache enabled), and one without.
-  void DisableForTesting(DisableForTestingReason reason);
 
   // Sets the number of documents that can be stored in the cache. This is meant
   // for use from within tests only.
@@ -176,6 +126,7 @@ class CONTENT_EXPORT BackForwardCacheImpl : public BackForwardCache {
   // BackForwardCache:
   void DisableForRenderFrameHost(GlobalFrameRoutingId id,
                                  std::string_view reason) override;
+  void DisableForTesting(DisableForTestingReason reason) override;
 
  private:
   // Destroys all evicted frames in the BackForwardCache.

content/public/browser/back_forward_cache.h

@@ -45,6 +45,58 @@ class CONTENT_EXPORT BackForwardCache {
   virtual void DisableForRenderFrameHost(GlobalFrameRoutingId id,
                                          std::string_view reason) = 0;
 
+  // List of reasons the BackForwardCache was disabled for a specific test. If a
+  // test needs to be disabled for a reason not covered below, please add to
+  // this enum.
+  enum DisableForTestingReason {
+    // The test has expectations that won't make sense if caching is enabled.
+    //
+    // One alternative to disabling BackForwardCache is to make the test's logic
+    // conditional, based on whether or not BackForwardCache is enabled.
+    //
+    // You should also consider whether it would make sense to instead split
+    // into two tests, one using a cacheable page, and one using an uncacheable
+    // page.
+    //
+    // Once BackForwardCache is enabled everywhere, any tests still disabled for
+    // this reason should change their expectations to permanently match the
+    // BackForwardCache enabled behavior.
+    TEST_ASSUMES_NO_CACHING,
+
+    // Unload events never fire for documents that are put into the
+    // BackForwardCache. This is by design, as there is never an appropriate
+    // moment to fire unload if the document is cached.
+    // In short, this is because:
+    //
+    // * We can't fire unload when going into the cache, because it may be
+    // destructive, and put the document into an unknown/bad state. Pages can
+    // also be cached and restored multiple times, and we don't want to invoke
+    // unload more than once.
+    //
+    // * We can't fire unload when the document is evicted from the cache,
+    // because at that point we don't want to run javascript for privacy and
+    // security reasons.
+    //
+    // An alternative to disabling the BackForwardCache, is to have the test
+    // load a page that is ineligible for caching (e.g. due to an unsupported
+    // feature).
+    TEST_USES_UNLOAD_EVENT,
+  };
+
+  // Disables the BackForwardCache so that no documents will be stored/served.
+  // This allows tests to "force" not using the BackForwardCache, this can be
+  // useful when:
+  // * Tests rely on a new document being loaded.
+  // * Tests want to test this case specifically.
+  // Callers should pass an accurate |reason| to make future triaging of
+  // disabled tests easier.
+  //
+  // Note: It's preferable to make tests BackForwardCache compatible
+  // when feasible, rather than using this method. Also please consider whether
+  // you actually should have 2 tests, one with the document cached
+  // (BackForwardCache enabled), and one without.
+  virtual void DisableForTesting(DisableForTestingReason reason) = 0;
+
  protected:
   BackForwardCache() = default;
 };

testing/buildbot/filters/bfcache.browser_tests.filter

 # These tests currently fail when run with --enable-features=BackForwardCache
 
 # Rely on an unload handler to run. It doesn't run.
--CredentialManagerBrowserTest.StoreInUnloadHandler_CrossSite_OnDemandMojoPipe
--CredentialManagerBrowserTest.StoreInUnloadHandler_CrossSite_PreestablishedPipe
 -IsolatedAppTest.IsolatedAppProcessModel
 
 # No focused view.