Make tests pass with navigation queuing

Update or skip tests to handle navigation queueing, which will cause
newer navigations to wait for a pre-existing pending commit navigation
to finish committing, before getting into the pending commit state
itself. For most tests, it just changes the navigations to not get
into the pending commit stage if it's not necessary for the test.

Bug: 1220337
Change-Id: I227ebcc35a7f7046592f3cde023bb6c5584dff63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4310980
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150190}

components/captive_portal/content/captive_portal_tab_helper_unittest.cc

@@ -279,7 +279,6 @@ TEST_F(CaptivePortalTabHelperTest, UnexpectedProvisionalLoad) {
   std::unique_ptr<NavigationSimulator> same_site_navigation =
       NavigationSimulator::CreateRendererInitiated(same_site_url, main_rfh());
   same_site_navigation->Start();
-  same_site_navigation->ReadyToCommit();
 
   // It's unexpectedly interrupted by a cross-process navigation, which starts
   // navigating before the old navigation cancels.

content/browser/renderer_host/navigation_controller_impl_unittest.cc

@@ -914,10 +914,7 @@ TEST_F(NavigationControllerTest, LoadURL_NewPending) {
   EXPECT_EQ(0U, navigation_entry_changed_counter_);
   EXPECT_EQ(0U, navigation_list_pruned_counter_);
 
-  // After the beforeunload but before it commits...
-  navigation->ReadyToCommit();
-
-  // ... Do a new navigation.
+  // After the navigation starts but before it commits, do a new navigation.
   const GURL kNewURL("http://see");
   NavigationSimulator::NavigateAndCommitFromDocument(kNewURL, main_test_rfh());
 
@@ -1007,7 +1004,7 @@ TEST_F(NavigationControllerTest, LoadURL_PrivilegedPending) {
   // The zeroth entry should be pending.
   auto back_navigation = NavigationSimulator::CreateHistoryNavigation(
       -1, contents(), false /* is_renderer_initiated */);
-  back_navigation->ReadyToCommit();
+  back_navigation->Start();
   EXPECT_EQ(0U, navigation_entry_changed_counter_);
   EXPECT_EQ(0U, navigation_list_pruned_counter_);
   EXPECT_EQ(0, controller.GetPendingEntryIndex());

content/browser/renderer_host/render_frame_host_impl.cc

@@ -1805,10 +1805,7 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   // a speculative RFH.  So, a speculative main frame being deleted will always
   // pass this condition as well.
   if (was_created && render_view_host_->GetMainRenderFrameHost() != this) {
-    CHECK(IsPendingDeletion() || IsInBackForwardCache() ||
-          lifecycle_state() == LifecycleStateImpl::kPrerendering ||
-          lifecycle_state() == LifecycleStateImpl::kSpeculative)
-        << lifecycle_state();
+    CHECK_NE(lifecycle_state(), LifecycleStateImpl::kActive);
   }
 
   GetAgentSchedulingGroup().RemoveRoute(routing_id_);

content/browser/renderer_host/render_frame_host_manager_browsertest.cc

@@ -49,6 +49,7 @@
 #include "content/browser/webui/web_ui_impl.h"
 #include "content/common/content_constants_internal.h"
 #include "content/common/content_navigation_policy.h"
+#include "content/common/features.h"
 #include "content/public/browser/browser_child_process_host.h"
 #include "content/public/browser/child_process_launcher_utils.h"
 #include "content/public/browser/navigation_controller.h"
@@ -1678,16 +1679,11 @@ IN_PROC_BROWSER_TEST_P(
 IN_PROC_BROWSER_TEST_P(
     RenderFrameHostManagerTest,
     DeleteSpeculativeRFHPendingCommitOfPendingEntryOnInterrupted1) {
-  if (ShouldCreateNewHostForAllFrames()) {
-    // This test involves starting a navigation while another navigation is
-    // committing, which might lead to deletion of a pending commit RFH, which
-    // will crash when RenderDocument is enabled. Skip the test if so.
-    // TODO(https://crbug.com/1220337): Update this test to work under
-    // navigation queueing, which will prevent the deletion of the pending
-    // commit RFH but still fails because this test actually expects the pending
-    // commit RFH to get deleted.
-    return;
+  if (!AreAllSitesIsolatedForTesting()) {
+    GTEST_SKIP() << "This test requires speculative RenderFrameHosts, so skip "
+                    "it when site isolation is turned off";
   }
+
   const std::string kOriginalPath = "/original.html";
   const std::string kFirstRedirectPath = "/redirect1.html";
   const std::string kSecondRedirectPath = "/reidrect2.html";
@@ -1755,40 +1751,39 @@ IN_PROC_BROWSER_TEST_P(
       "Content-Type: text/html; charset=utf-8\r\n"
       "\r\n");
   EXPECT_TRUE(first_reload.WaitForResponse());
-  first_reload.ResumeNavigation();
 
-  // The navigation is ready to commit: it has been handed to the speculative
-  // RenderFrameHost for commit if Site Isolation is enabled, otherwise it
-  // commits in the same RenderFrameHost.
-  RenderFrameHostImpl* speculative_rfh =
-      static_cast<WebContentsImpl*>(shell()->web_contents())
-          ->GetPrimaryFrameTree()
-          .root()
-          ->render_manager()
-          ->speculative_frame_host();
-  if (AreAllSitesIsolatedForTesting()) {
-    CHECK(speculative_rfh);
-  } else {
-    CHECK(!speculative_rfh);
-  }
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetPrimaryFrameTree()
+                            .root();
+  RenderFrameHostImplWrapper first_speculative_rfh(
+      root->render_manager()->speculative_frame_host());
+  EXPECT_TRUE(first_speculative_rfh.get());
 
   // The user requests a new reload while the previous reload hasn't committed
-  // yet. The navigation start deletes the speculative RenderFrameHost that was
-  // supposed to commit the browser-initiated navigation, unless Site Isolation
-  // is enabled. This should not crash.
+  // yet. This second reload starts immediately after pausing the commit of the
+  // first reload. It might delete the speculative RenderFrameHost that was
+  // supposed to commit the first reload. This should not crash.
   TestNavigationManager second_reload(shell()->web_contents(), kOriginalURL);
+  CommitNavigationPauser commit_pauser(first_speculative_rfh.get());
+  first_reload.ResumeNavigation();
+  commit_pauser.WaitForCommitAndPause();
   shell()->web_contents()->GetController().Reload(
       ReloadType::ORIGINAL_REQUEST_URL, false);
   EXPECT_TRUE(second_reload.WaitForRequestStart());
-  speculative_rfh = static_cast<WebContentsImpl*>(shell()->web_contents())
-                        ->GetPrimaryFrameTree()
-                        .root()
-                        ->render_manager()
-                        ->speculative_frame_host();
-  if (AreAllSitesIsolatedForTesting()) {
-    EXPECT_TRUE(speculative_rfh);
+
+  RenderFrameHostImplWrapper second_speculative_rfh(
+      root->render_manager()->speculative_frame_host());
+
+  EXPECT_TRUE(second_speculative_rfh.get());
+  if (ShouldQueueNavigationsWhenPendingCommitRFHExists()) {
+    // When navigation queueing is enabled, the first speculative RFH is still
+    // kept around as it is pending commit.
+    EXPECT_TRUE(first_speculative_rfh.get());
+    EXPECT_EQ(first_speculative_rfh.get(), second_speculative_rfh.get());
   } else {
-    EXPECT_FALSE(speculative_rfh);
+    // Otherwise, the first speculative RFH will be deleted and replaced by a
+    // new speculative RFH.
+    EXPECT_FALSE(first_speculative_rfh.get());
   }
 
   // The second reload results in a 204.
@@ -1799,12 +1794,17 @@ IN_PROC_BROWSER_TEST_P(
       "Connection: close\r\n"
       "\r\n");
   ASSERT_TRUE(second_reload.WaitForNavigationFinished());
-  speculative_rfh = static_cast<WebContentsImpl*>(shell()->web_contents())
-                        ->GetPrimaryFrameTree()
-                        .root()
-                        ->render_manager()
-                        ->speculative_frame_host();
-  EXPECT_FALSE(speculative_rfh);
+
+  if (ShouldQueueNavigationsWhenPendingCommitRFHExists()) {
+    // If navigation queuing is enabled, the first reload's speculative RFH
+    // will be kept.
+    EXPECT_TRUE(root->render_manager()->speculative_frame_host());
+  } else {
+    // If navigation queueing is turned off, the second reload will delete the
+    // first reload's speculative RFH, and we end up with no speculative RFH
+    // after the second reload commits.
+    EXPECT_FALSE(root->render_manager()->speculative_frame_host());
+  }
 }
 
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_ANDROID) || \
@@ -1824,6 +1824,12 @@ IN_PROC_BROWSER_TEST_P(
 IN_PROC_BROWSER_TEST_P(
     RenderFrameHostManagerTest,
     MAYBE_DeleteSpeculativeRFHPendingCommitOfPendingEntryOnInterrupted2) {
+  if (ShouldQueueNavigationsWhenPendingCommitRFHExists()) {
+    // When navigation queueing is enabled, starting a new navigation won't
+    // delete an existing pending commit RFH, so this test can't run as
+    // intended.
+    return;
+  }
   const std::string kOriginalPath = "/original.html";
   const std::string kRedirectPath = "/redirect.html";
   net::test_server::ControllableHttpResponse original_response1(

content/browser/renderer_host/render_frame_host_manager_unittest.cc

@@ -416,15 +416,15 @@ class RenderFrameHostManagerTest
 
     // Simulates request creation that triggers the 1st internal call to
     // GetFrameHostForNavigation.
-    manager->DidCreateNavigationRequest(navigation_request.get());
+    frame_tree_node->TakeNavigationRequest(std::move(navigation_request));
 
     // And also simulates the 2nd and final call to GetFrameHostForNavigation
     // that determines the final frame that will commit the navigation.
     BrowsingContextGroupSwap ignored_bcg_swap_info =
         BrowsingContextGroupSwap::CreateDefault();
     TestRenderFrameHost* frame_host = static_cast<TestRenderFrameHost*>(
         manager
-            ->GetFrameHostForNavigation(navigation_request.get(),
+            ->GetFrameHostForNavigation(frame_tree_node->navigation_request(),
                                         &ignored_bcg_swap_info)
             .value());
     CHECK(frame_host);
@@ -3258,7 +3258,7 @@ TEST_P(RenderFrameHostManagerTest, NavigateFromDeadRendererToWebUI) {
           nullptr /* navigation_ui_data */, absl::nullopt /* impression */,
           false /* is_pdf */
       );
-  manager->DidCreateNavigationRequest(navigation_request.get());
+  frame_tree_node->TakeNavigationRequest(std::move(navigation_request));
 
   // As the initial RenderFrame was not live, the new RenderFrameHost should be
   // made as active/current immediately along with its WebUI at request time.
@@ -3274,8 +3274,9 @@ TEST_P(RenderFrameHostManagerTest, NavigateFromDeadRendererToWebUI) {
   BrowsingContextGroupSwap ignored_bcg_swap_info =
       BrowsingContextGroupSwap::CreateDefault();
   EXPECT_EQ(host, manager
-                      ->GetFrameHostForNavigation(navigation_request.get(),
-                                                  &ignored_bcg_swap_info)
+                      ->GetFrameHostForNavigation(
+                          frame_tree_node->navigation_request(),
+                          &ignored_bcg_swap_info)
                       .value());
 
   // No pending RenderFrameHost as the current one should be reused.

content/public/test/test_utils.cc

@@ -232,6 +232,10 @@ bool CanSameSiteMainFrameNavigationsChangeSiteInstances() {
          IsBackForwardCacheEnabled();
 }
 
+bool IsNavigationQueueingEnabled() {
+  return ShouldQueueNavigationsWhenPendingCommitRFHExists();
+}
+
 void DisableProactiveBrowsingInstanceSwapFor(RenderFrameHost* rfh) {
   if (!CanSameSiteMainFrameNavigationsChangeSiteInstances())
     return;
@@ -486,8 +490,9 @@ bool RenderFrameDeletedObserver::WaitUntilDeleted() {
 }
 
 RenderFrameHostWrapper::RenderFrameHostWrapper(RenderFrameHost* rfh)
-    : rfh_id_(rfh->GetGlobalId()),
-      deleted_observer_(std::make_unique<RenderFrameDeletedObserver>(rfh)) {}
+    : rfh_id_(rfh ? rfh->GetGlobalId() : GlobalRenderFrameHostId()),
+      deleted_observer_(rfh ? std::make_unique<RenderFrameDeletedObserver>(rfh)
+                            : nullptr) {}
 
 RenderFrameHostWrapper::RenderFrameHostWrapper(RenderFrameHostWrapper&& rfhft) =
     default;
@@ -504,10 +509,12 @@ bool RenderFrameHostWrapper::IsDestroyed() const {
 // See RenderFrameDeletedObserver for notes on the difference between
 // RenderFrame being deleted and RenderFrameHost being destroyed.
 bool RenderFrameHostWrapper::WaitUntilRenderFrameDeleted() const {
+  CHECK(deleted_observer_);
   return deleted_observer_->WaitUntilDeleted();
 }
 
 bool RenderFrameHostWrapper::IsRenderFrameDeleted() const {
+  CHECK(deleted_observer_);
   return deleted_observer_->deleted();
 }
 

content/public/test/test_utils.h

@@ -133,6 +133,10 @@ bool WillSameSiteNavigationsChangeRenderFrameHosts();
 // above, this will not be true when RenderDocument for main-frame is enabled.
 bool CanSameSiteMainFrameNavigationsChangeSiteInstances();
 
+// Returns true if navigation queueing is fully enabled, where we will queue new
+// navigations that happen when there is an existing pending commit navigation.
+bool IsNavigationQueueingEnabled();
+
 // Makes sure that navigations that start in |rfh| won't result in a proactive
 // BrowsingInstance swap (note they might still result in a normal
 // BrowsingInstance swap, e.g. in the case of cross-site navigations).