[navigation] Do not swap BrowsingInstances when reloading error pages

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1929033.

Force reloads of error pages to always stay in the same browsing
instance.

The reason is the autoreload logic relies on the RenderFrame staying
the same (see crbug.com/1045524).

R=alexmos@chromium.org,nasko@chromium.org
CC=​mmenke@chromium.org
BUG=1043888

Change-Id: Ifd3ee23325c88a82a9008b80784ea5b0c7976b22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2011924
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735564}

content/browser/frame_host/render_frame_host_manager.cc

@@ -1124,11 +1124,26 @@ RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     SiteInstance* destination_site_instance,
     const GURL& destination_effective_url,
     bool destination_is_view_source_mode,
-    bool is_failure) const {
+    bool is_failure,
+    bool is_reload) const {
   // A subframe must stay in the same BrowsingInstance as its parent.
   if (!frame_tree_node_->IsMainFrame())
     return ShouldSwapBrowsingInstance::kNo_NotMainFrame;
 
+  // If this navigation is reloading an error page, do not swap BrowsingInstance
+  // and keep the error page in a related SiteInstance. If later a reload of
+  // this navigation is successful, it will correctly create a new
+  // BrowsingInstance if needed.
+  // TODO(https://crbug.com/1045524): We want to remove this, but it is kept for
+  // now as a workaround for the fact that autoreload is not working properly
+  // when we are changing RenderFrames. Remove this when autoreload logic is
+  // updated to handle different RenderFrames correctly.
+  if (is_failure && is_reload &&
+      SiteIsolationPolicy::IsErrorPageIsolationEnabled(
+          frame_tree_node_->IsMainFrame())) {
+    return ShouldSwapBrowsingInstance::kNo_ReloadingErrorPage;
+  }
+
   // If new_entry already has a SiteInstance, assume it is correct.  We only
   // need to force a swap if it is in a different BrowsingInstance.
   if (destination_site_instance) {
@@ -1238,6 +1253,7 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
     SiteInstanceImpl* candidate_instance,
     ui::PageTransition transition,
     bool is_failure,
+    bool is_reload,
     bool dest_is_restore,
     bool dest_is_view_source_mode,
     bool was_server_redirect) {
@@ -1291,7 +1307,7 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
       ShouldSwapBrowsingInstancesForNavigation(
           current_effective_url, current_is_view_source_mode, dest_instance,
           SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
-          dest_is_view_source_mode, is_failure);
+          dest_is_view_source_mode, is_failure, is_reload);
   bool force_swap = force_swap_result == ShouldSwapBrowsingInstance::kYes;
   if (!force_swap) {
     render_frame_host_->set_browsing_instance_not_swapped_reason(
@@ -2177,11 +2193,21 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
           ? speculative_render_frame_host_->GetSiteInstance()
           : nullptr;
 
+  // Account for renderer-initiated reload as well.
+  // Needed as a workaround for https://crbug.com/1045524, remove it when it is
+  // fixed.
+  bool is_reload = request->common_params().navigation_type ==
+                       mojom::NavigationType::RELOAD ||
+                   request->common_params().navigation_type ==
+                       mojom::NavigationType::RELOAD_BYPASSING_CACHE ||
+                   request->common_params().navigation_type ==
+                       mojom::NavigationType::RELOAD_ORIGINAL_REQUEST_URL;
+
   scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
       request->common_params().url, request->GetSourceSiteInstance(),
       request->dest_site_instance(), candidate_site_instance,
       request->common_params().transition,
-      request->state() >= NavigationRequest::CANCELING,
+      request->state() >= NavigationRequest::CANCELING, is_reload,
       request->GetRestoreType() != RestoreType::NONE, request->is_view_source(),
       request->WasServerRedirect());
 

content/browser/frame_host/render_frame_host_manager.h

@@ -585,7 +585,8 @@ class CONTENT_EXPORT RenderFrameHostManager
       SiteInstance* destination_site_instance,
       const GURL& destination_effective_url,
       bool destination_is_view_source_mode,
-      bool is_failure) const;
+      bool is_failure,
+      bool is_reload) const;
 
   // Returns the SiteInstance to use for the navigation.
   scoped_refptr<SiteInstance> GetSiteInstanceForNavigation(
@@ -595,6 +596,7 @@ class CONTENT_EXPORT RenderFrameHostManager
       SiteInstanceImpl* candidate_instance,
       ui::PageTransition transition,
       bool is_failure,
+      bool is_reload,
       bool dest_is_restore,
       bool dest_is_view_source_mode,
       bool was_server_redirect);

content/browser/frame_host/render_frame_host_manager_browsertest.cc

@@ -5366,6 +5366,15 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
         success_site_instance->IsRelatedSiteInstance(initial_instance.get()));
   }
 
+  // Install a client forcing every navigation to swap BrowsingInstances.
+  // TODO(https://crbug.com/1045524): Ensure for now that we are not swapping
+  // pages on reloads for now even when content client tells us to.
+  // Move client initialisation to a later point in the time after the
+  // problem is fixed.
+  BrowsingInstanceSwapContentBrowserClient content_browser_client;
+  ContentBrowserClient* old_client =
+      SetBrowserClientForTesting(&content_browser_client);
+
   // Reload of the error page that still results in an error should stay in
   // the same SiteInstance. Ensure this works for both browser-initiated
   // reloads and renderer-initiated ones.
@@ -5375,10 +5384,8 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
     reload_observer.Wait();
     EXPECT_FALSE(reload_observer.last_navigation_succeeded());
     EXPECT_EQ(2, nav_controller.GetEntryCount());
-    if (!IsProactivelySwapBrowsingInstanceEnabled()) {
-      EXPECT_EQ(initial_instance,
-                shell()->web_contents()->GetMainFrame()->GetSiteInstance());
-    }
+    EXPECT_EQ(initial_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
     EXPECT_TRUE(
         IsMainFrameOriginOpaqueAndCompatibleWithURL(shell(), error_url));
   }
@@ -5388,21 +5395,12 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
     reload_observer.Wait();
     EXPECT_FALSE(reload_observer.last_navigation_succeeded());
     EXPECT_EQ(2, nav_controller.GetEntryCount());
-    if (!IsProactivelySwapBrowsingInstanceEnabled()) {
-      EXPECT_EQ(initial_instance,
-                shell()->web_contents()->GetMainFrame()->GetSiteInstance());
-    }
+    EXPECT_EQ(initial_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
     EXPECT_TRUE(
         IsMainFrameOriginOpaqueAndCompatibleWithURL(shell(), error_url));
   }
 
-  // Install a client forcing every navigation to swap BrowsingInstances.
-  // Do not do it earlier to ensure that we don't force BrowsingInstance swap
-  // for the reloads above.
-  BrowsingInstanceSwapContentBrowserClient content_browser_client;
-  ContentBrowserClient* old_client =
-      SetBrowserClientForTesting(&content_browser_client);
-
   // Allow the navigation to succeed and ensure it swapped to a non-related
   // SiteInstance.
   url_interceptor.reset();

content/browser/frame_host/should_swap_browsing_instance.h

@@ -20,7 +20,7 @@ enum class ShouldSwapBrowsingInstance {
   kNo_SourceURLSchemeIsNotHTTPOrHTTPS = 5,
   kNo_DestinationURLSchemeIsNotHTTPOrHTTPS = 6,
   kNo_SameSiteNavigation = 7,
-  kNo_ErrorPage = 8,
+  kNo_ReloadingErrorPage = 8,
   kNo_AlreadyHasMatchingBrowsingInstance = 9,
   kNo_RendererDebugURL = 10,
   kNo_NotNeededForBackForwardCache = 11,