Do not change BrowsingInstance on error page commits.

When error page isolation is enabled, error pages commit in their own
SiteInstance. Reloading an error page should stay in the same
SiteInstance it was initially placed in. However, if a navigation is to
an URL that requires a BrowsingInstance swap, forcing the swap causes
the error page to commit in a SiteInstance for the destination URL.
This is not expected nor is it the desired behavior. This CL adds a
check for error pages in ShouldSwapBrowsingInstancesForNavigation
to keep the commits in the same BrowsingInstance. When a successful
navigation/reload happens for the destination URL, the BrowsingInstance
will be correctly swapped.

Bug: 838161
Change-Id: Iaf2e708426b75881dda7065069b618f180fef7d2
Reviewed-on: https://chromium-review.googlesource.com/1113905
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570251}

content/browser/frame_host/render_frame_host_manager.cc

@@ -884,13 +884,23 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     bool current_is_view_source_mode,
     SiteInstance* new_site_instance,
     const GURL& new_effective_url,
-    bool new_is_view_source_mode) const {
+    bool new_is_view_source_mode,
+    bool is_failure) const {
   // A subframe must stay in the same BrowsingInstance as its parent.
   // TODO(nasko): Ensure that SiteInstance swap is still triggered for subframes
   // in the cases covered by the rest of the checks in this method.
   if (!frame_tree_node_->IsMainFrame())
     return false;
 
+  // If the navigation has resulted in 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 (is_failure && SiteIsolationPolicy::IsErrorPageIsolationEnabled(
+                        frame_tree_node_->IsMainFrame())) {
+    return false;
+  }
+
   // 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 (new_site_instance) {
@@ -1029,11 +1039,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
       current_entry->IsViewSourceMode() : dest_is_view_source_mode;
 
   bool force_swap = ShouldSwapBrowsingInstancesForNavigation(
-      current_effective_url,
-      current_is_view_source_mode,
-      dest_instance,
+      current_effective_url, current_is_view_source_mode, dest_instance,
       SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
-      dest_is_view_source_mode);
+      dest_is_view_source_mode, is_failure);
   SiteInstanceDescriptor new_instance_descriptor =
       SiteInstanceDescriptor(current_instance);
   if (ShouldTransitionCrossSite() || force_swap) {

content/browser/frame_host/render_frame_host_manager.h

@@ -565,7 +565,8 @@ class CONTENT_EXPORT RenderFrameHostManager
       bool current_is_view_source_mode,
       SiteInstance* new_site_instance,
       const GURL& new_effective_url,
-      bool new_is_view_source_mode) const;
+      bool new_is_view_source_mode,
+      bool is_failure) const;
 
   // Returns the SiteInstance to use for the navigation.
   scoped_refptr<SiteInstance> GetSiteInstanceForNavigation(

content/browser/frame_host/render_frame_host_manager_browsertest.cc

@@ -54,6 +54,7 @@
 #include "content/public/test/url_loader_interceptor.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
+#include "content/test/test_content_browser_client.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -4349,6 +4350,106 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, ErrorPageNavigationReload) {
       shell()->web_contents()->GetMainFrame()->GetSiteInstance()->GetSiteURL());
 }
 
+// A test ContentBrowserClient implementation which enforces
+// BrowsingInstance swap on every navigation. It is used to verify that
+// reloading of an error page to an URL that requires BrowsingInstance swap
+// works correctly.
+class BrowsingInstanceSwapContentBrowserClient
+    : public TestContentBrowserClient {
+ public:
+  BrowsingInstanceSwapContentBrowserClient() = default;
+
+  bool ShouldIsolateErrorPage(bool in_main_frame) override {
+    return in_main_frame;
+  }
+
+  bool ShouldSwapBrowsingInstancesForNavigation(
+      content::SiteInstance* site_instance,
+      const GURL& current_url,
+      const GURL& new_url) override {
+    return true;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrowsingInstanceSwapContentBrowserClient);
+};
+
+// Test to verify that reloading of an error page which resulted from a
+// navigation to an URL which requires a BrowsingInstance swap, correcly
+// reloads in the same SiteInstance for the error page.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       ErrorPageNavigationReloadBrowsingInstanceSwap) {
+  StartEmbeddedServer();
+  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  GURL error_url(embedded_test_server()->GetURL("/empty.html"));
+  std::unique_ptr<URLLoaderInterceptor> url_interceptor =
+      SetupRequestFailForURL(error_url);
+  NavigationControllerImpl& nav_controller =
+      static_cast<NavigationControllerImpl&>(
+          shell()->web_contents()->GetController());
+
+  // Start with a successful navigation to a document and verify there is
+  // only one entry in session history.
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+  scoped_refptr<SiteInstance> success_site_instance =
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance();
+  EXPECT_EQ(1, nav_controller.GetEntryCount());
+
+  BrowsingInstanceSwapContentBrowserClient content_browser_client;
+  ContentBrowserClient* old_client =
+      SetBrowserClientForTesting(&content_browser_client);
+
+  // Navigate to an url resulting in an error page and ensure a new entry
+  // was added to session history.
+  EXPECT_FALSE(NavigateToURL(shell(), error_url));
+  EXPECT_EQ(2, nav_controller.GetEntryCount());
+
+  scoped_refptr<SiteInstance> initial_instance =
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance();
+  EXPECT_EQ(GURL(kUnreachableWebDataURL), initial_instance->GetSiteURL());
+  EXPECT_TRUE(
+      success_site_instance->IsRelatedSiteInstance(initial_instance.get()));
+
+  // 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.
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
+    reload_observer.Wait();
+    EXPECT_FALSE(reload_observer.last_navigation_succeeded());
+    EXPECT_EQ(2, nav_controller.GetEntryCount());
+    EXPECT_EQ(initial_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+  }
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    EXPECT_TRUE(ExecuteScript(shell(), "location.reload();"));
+    reload_observer.Wait();
+    EXPECT_FALSE(reload_observer.last_navigation_succeeded());
+    EXPECT_EQ(2, nav_controller.GetEntryCount());
+    EXPECT_EQ(initial_instance,
+              shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+  }
+
+  // Allow the navigation to succeed and ensure it swapped to a non-related
+  // SiteInstance.
+  url_interceptor.reset();
+  {
+    TestNavigationObserver reload_observer(shell()->web_contents());
+    EXPECT_TRUE(ExecuteScript(shell(), "location.reload();"));
+    reload_observer.Wait();
+    EXPECT_TRUE(reload_observer.last_navigation_succeeded());
+    EXPECT_EQ(2, nav_controller.GetEntryCount());
+    EXPECT_FALSE(initial_instance->IsRelatedSiteInstance(
+        shell()->web_contents()->GetMainFrame()->GetSiteInstance()));
+    EXPECT_FALSE(success_site_instance->IsRelatedSiteInstance(
+        shell()->web_contents()->GetMainFrame()->GetSiteInstance()));
+  }
+
+  SetBrowserClientForTesting(old_client);
+}
+
 // Helper class to simplify testing of unload handlers.  It allows waiting for
 // particular HTTP requests to be made to the embedded_test_server(); the tests
 // use this to wait for termination pings (e.g., navigator.sendBeacon()) made

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -392,11 +392,10 @@ class RenderFrameHostManagerTest : public RenderViewHostImplTestHarness {
     bool current_is_view_source_mode = current_entry ?
         current_entry->IsViewSourceMode() : new_entry->IsViewSourceMode();
     return manager->ShouldSwapBrowsingInstancesForNavigation(
-        current_effective_url,
-        current_is_view_source_mode,
+        current_effective_url, current_is_view_source_mode,
         new_entry->site_instance(),
         SiteInstanceImpl::GetEffectiveURL(browser_context, new_entry->GetURL()),
-        new_entry->IsViewSourceMode());
+        new_entry->IsViewSourceMode(), false);
   }
 
   // Creates a test RenderViewHost that's swapped out.