Move all new BI swaps to ShouldSwapBrowsingInstancesForNavigation

Currently the decision to create new BrowsingInstances are made in two
functions: ShouldSwapBrowsingInstancesForNavigation and
DetermineSiteInstanceForURL, possibly causing confusion when only
looking at one of the functions, and other issues like not being able
to know whether it is a "forces swap" (requiring different processes)
or a "proactive swap" (may not require different processes). This CL
fixes that by moving all the BI swaps to
ShouldSwapBrowsingInstancesForNavigation.

Bug: 977562
Change-Id: Id5fb8785399584199ac0711811e9ec8b6d6efe03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2081728
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750897}

content/browser/frame_host/render_frame_host_manager.h

@@ -521,6 +521,10 @@ class CONTENT_EXPORT RenderFrameHostManager
     UNRELATED,
     // A SiteInstance in the same browsing instance as the current.
     RELATED,
+    // A pre-existing SiteInstance that might or might not be in the same
+    // browsing instance as the current. Only used when |existing_site_instance|
+    // is specified.
+    PREEXISTING,
   };
 
   enum class AttachToInnerDelegateState {
@@ -540,7 +544,7 @@ class CONTENT_EXPORT RenderFrameHostManager
   struct CONTENT_EXPORT SiteInstanceDescriptor {
     explicit SiteInstanceDescriptor(content::SiteInstance* site_instance)
         : existing_site_instance(site_instance),
-          relation(SiteInstanceRelation::UNRELATED) {}
+          relation(SiteInstanceRelation::PREEXISTING) {}
 
     SiteInstanceDescriptor(BrowserContext* browser_context,
                            GURL dest_url,
@@ -556,8 +560,8 @@ class CONTENT_EXPORT RenderFrameHostManager
     // be used with |dest_url| to resolve the site URL.
     BrowserContext* browser_context;
 
-    // In case |existing_site_instance| is null, specify how the new site is
-    // related to the current BrowsingInstance.
+    // Specifies how the new site is related to the current BrowsingInstance.
+    // This is PREEXISTING iff |existing_site_instance| is defined.
     SiteInstanceRelation relation;
   };
 
@@ -574,7 +578,15 @@ class CONTENT_EXPORT RenderFrameHostManager
   // be created (even if we are in a process model that doesn't usually swap).
   // This forces a process swap and severs script connections with existing
   // tabs.  Cases where this can happen include transitions between WebUI and
-  // regular web pages. |dest_site_instance| may be null.
+  // regular web pages.
+  //
+  // |source_instance| is the SiteInstance of the frame that initiated the
+  // navigation. |current_instance| is the SiteInstance of the frame that is
+  // currently navigating. |destination_instance| is a predetermined
+  // SiteInstance that will be used for |destination_effective_url| if not
+  // null - we will swap BrowsingInstances if it's in a different
+  // BrowsingInstance than the current one.
+  //
   // If there is no current NavigationEntry, then |current_is_view_source_mode|
   // should be the same as |dest_is_view_source_mode|.
   //
@@ -585,12 +597,16 @@ class CONTENT_EXPORT RenderFrameHostManager
   ShouldSwapBrowsingInstance ShouldSwapBrowsingInstancesForNavigation(
       const GURL& current_effective_url,
       bool current_is_view_source_mode,
-      SiteInstance* destination_site_instance,
+      SiteInstanceImpl* source_instance,
+      SiteInstanceImpl* current_instance,
+      SiteInstance* destination_instance,
       const GURL& destination_effective_url,
       bool destination_is_view_source_mode,
+      ui::PageTransition transition,
       bool is_failure,
       bool is_reload,
-      bool cross_origin_opener_policy_mismatch) const;
+      bool cross_origin_opener_policy_mismatch,
+      bool was_server_redirect);
 
   // Returns the SiteInstance to use for the navigation.
   scoped_refptr<SiteInstance> GetSiteInstanceForNavigation(
@@ -653,6 +669,12 @@ class CONTENT_EXPORT RenderFrameHostManager
       ui::PageTransition transition,
       const GURL& dest_url);
 
+  // Returns true if we can use |source_instance| for |dest_url|.
+  bool CanUseSourceSiteInstance(const GURL& dest_url,
+                                SiteInstance* source_instance,
+                                bool was_server_redirect,
+                                bool is_failure);
+
   // Converts a SiteInstanceDescriptor to the actual SiteInstance it describes.
   // If a |candidate_instance| is provided (is not nullptr) and it matches the
   // description, it is returned as is.

content/browser/frame_host/render_frame_host_manager_browsertest.cc

@@ -2389,12 +2389,20 @@ IN_PROC_BROWSER_TEST_P(RenderFrameHostManagerTest,
   shell()->LoadURL(GURL("javascript:document.title='msg'"));
   ASSERT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 
+  scoped_refptr<SiteInstance> orig_site_instance(
+      shell()->web_contents()->GetSiteInstance());
   // Crash the renderer of the view-source page.
   RenderProcessHostWatcher crash_observer(
       shell()->web_contents(),
       RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
   EXPECT_TRUE(NavigateToURLAndExpectNoCommit(shell(), GURL(kChromeUICrashURL)));
   crash_observer.Wait();
+
+  // We should not change SiteInstance and BrowsingInstance on navigations to
+  // RendererDebug URLs.
+  auto* new_site_instance = shell()->web_contents()->GetSiteInstance();
+  EXPECT_EQ(orig_site_instance, new_site_instance);
+  EXPECT_TRUE(orig_site_instance->IsRelatedSiteInstance(new_site_instance));
 }
 
 // Ensure that renderer-side debug URLs don't take effect on crashed renderers.

content/browser/webui/web_ui_browsertest.cc

@@ -154,6 +154,26 @@ IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest, ForceSwapOnDifferenteWebUITypes) {
       web_contents->GetMainFrame()->GetProcess()->GetID()));
 }
 
+// Tests that a WebUI page will use a separate SiteInstance when we navigated to
+// it from the initial blank page.
+IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest,
+                       ForceBrowsingInstanceSwapOnFirstNavigation) {
+  WebContents* web_contents = shell()->web_contents();
+  scoped_refptr<SiteInstance> orig_site_instance(
+      web_contents->GetSiteInstance());
+  // Navigate from the initial blank page to the WebUI URL.
+  const GURL web_ui_url(GetWebUIURL(kChromeUIHistogramHost));
+  EXPECT_TRUE(ContentWebUIControllerFactory::GetInstance()->UseWebUIForURL(
+      web_contents->GetBrowserContext(), web_ui_url));
+  ASSERT_TRUE(NavigateToURL(web_contents, web_ui_url));
+
+  EXPECT_TRUE(ChildProcessSecurityPolicy::GetInstance()->HasWebUIBindings(
+      web_contents->GetMainFrame()->GetProcess()->GetID()));
+  auto* new_site_instance = web_contents->GetSiteInstance();
+  EXPECT_NE(orig_site_instance, new_site_instance);
+  EXPECT_FALSE(orig_site_instance->IsRelatedSiteInstance(new_site_instance));
+}
+
 // Tests that navigating from chrome:// to chrome-untrusted:// results in
 // SiteInstance swap.
 IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest, ForceSwapOnFromChromeToUntrusted) {