Fix process leak from NavigationHandle::GetStartingSiteInstance.

CL adapted from clamy@'s https://crrev.com/c/1565876.
This is a partial revert of r638122 and r642275, plus a new test.

This CL fixes a process leak that happens in navigations where the
current renderer process is not live (crashed process or NTP). The
mechanism is the following:

1) We want to navigate to a page in a FrameTreeNode where the current
RFH is not live. This RFH has a SiteInstance with id 1, and a process
with id 1.
2) We create a NavigationRequest, which takes as its starting
SiteInstance the SiteInstance with id 1.
3) We call into RenderFrameHostManager::GetFrameHostForNavigation. It
creates a speculative RFH, with a new SiteInstance with id 2, and a
process with id 2. It commits this RFH immediately, since the current
one is not live. This has the side effect of deleting process 1, however
it does not delete SiteInstance 1 since it is referenced by the
NavigationRequest. So SiteInstance 1 ends up associated with no RFH, and
has no process either.
4) We create the NavigationHandle and call DidStartNavigation. One of
the WebContentsObservers accesses NavigationHandle::GetStartingSiteInstance
(which is SiteInstance 1), and asks it for its process
(https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc?type=cs&q=safe_browsing::SafeBrowsingNavigationObserver::DidStartNavigation&g=0&l=185).
Because the SiteInstance no longer has a process, we re-create a process
with id 3 due to this call.
5) We finish the navigation and delete the last reference to
SiteInstance 1. However, this does not delete process 3, which is not
referenced anymore, and we leak the process.

To fix the issue, this CL makes sure that the starting SiteInstance is
captured just before creating the NavigationHandle, which is after a new
RFH has been committed to replace the non-live current RFH. This way,
the SiteInstance is properly linked to a RFH and it has a
RenderProcessHost. Separately, we can ensure that unused processes will
be cleaned up if GetProcess() is called unexpectedly.

TBR=clamy@chromium.org
TBR=jbudorick@chromium.org

Bug: 949977
Change-Id: Ia84e238af0036822dd8c4cd47be5b497d0e7be3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1566643
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650619}

content/browser/frame_host/navigation_handle_impl_browsertest.cc

@@ -1410,6 +1410,71 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
   EXPECT_FALSE(NavigateToURL(shell(), kUrl2));
 }
 
+// Verify that a cross-process navigation in a frame for which the current
+// renderer process is not live will not result in leaking a
+// RenderProcessHost. See https://crbug.com/949977.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
+                       NoLeakFromStartingSiteInstance) {
+  GURL url_a = embedded_test_server()->GetURL("a.com", "/title1.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url_a));
+
+  // Kill the a.com process, to test what happens with the next navigation.
+  scoped_refptr<SiteInstance> site_instance_a =
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance();
+  EXPECT_TRUE(site_instance_a->HasProcess());
+  RenderProcessHost* process_1 = site_instance_a->GetProcess();
+  RenderProcessHostWatcher process_exit_observer_1(
+      process_1, content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  RenderProcessHostWatcher rph_gone_observer_1(
+      process_1, content::RenderProcessHostWatcher::WATCH_FOR_HOST_DESTRUCTION);
+  process_1->Shutdown(RESULT_CODE_KILLED);
+  process_exit_observer_1.Wait();
+
+  // Start to navigate the sad tab to another site.
+  GURL url_b = embedded_test_server()->GetURL("b.com", "/title2.html");
+  TestNavigationManager navigation_b(shell()->web_contents(), url_b);
+  shell()->web_contents()->GetController().LoadURL(
+      url_b, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  EXPECT_TRUE(navigation_b.WaitForRequestStart());
+
+  // The starting SiteInstance should be the SiteInstance of the current
+  // RenderFrameHost.
+  scoped_refptr<SiteInstance> starting_site_instance =
+      navigation_b.GetNavigationHandle()->GetStartingSiteInstance();
+  EXPECT_EQ(shell()->web_contents()->GetMainFrame()->GetSiteInstance(),
+            starting_site_instance);
+  // Because of the sad tab, this is actually the b.com SiteInstance, which
+  // commits immediately after starting the navigation and has a process.
+  EXPECT_EQ(GURL("http://b.com"), starting_site_instance->GetSiteURL());
+  EXPECT_TRUE(starting_site_instance->HasProcess());
+
+  // In https://crbug.com/949977, we used the a.com SiteInstance here and didn't
+  // have a process, and an observer called GetProcess, creating a process. This
+  // process never went away, even after the SiteInstance was gone.
+  RenderProcessHost* rph_2 = starting_site_instance->GetProcess();
+  RenderProcessHostWatcher process_exit_observer_2(
+      rph_2, content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  navigation_b.WaitForNavigationFinished();
+
+  // Ensure RPH 1 is destroyed, which happens at commit time even before the fix
+  // for the bug.
+  rph_gone_observer_1.Wait();
+
+  // Navigate to another process. This isn't necessary to trigger the original
+  // leak (when the starting SiteInstance was a.com), but it lets the test
+  // finish in the case that the starting SiteInstance is b.com, since b.com's
+  // process goes away with this navigation.
+  // TODO(creis): There's still a slight risk that other buggy code could find
+  // site_instance_a and call GetProcess() on it, causing a leak. We'll add a
+  // backup fix and test for that in a followup CL.
+  GURL url_c = embedded_test_server()->GetURL("c.com", "/title1.html");
+  EXPECT_TRUE(NavigateToURL(shell()->web_contents(), url_c));
+
+  // Wait for rph_2 to exit when it's not used. This wouldn't happen when the
+  // bug was present.
+  process_exit_observer_2.Wait();
+}
+
 // Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
 // URL from the very beginning of the navigation.
 class NavigationHandleImplHttpsUpgradeBrowserTest

content/browser/frame_host/navigation_request.cc

@@ -563,17 +563,6 @@ NavigationRequest::NavigationRequest(
     bindings_ = entry->bindings();
   }
 
-  // This is needed to get site URLs and assign the expected RenderProcessHost.
-  // This is not always the same as |source_site_instance|, as it only depends
-  // on the current frame host, and does not depend on |entry|.
-  starting_site_instance_ =
-      frame_tree_node->current_frame_host()->GetSiteInstance();
-
-  // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
-  // be correct for cross-BrowsingInstance redirects.
-  site_url_ = SiteInstanceImpl::GetSiteForURL(
-      starting_site_instance_->GetIsolationContext(), common_params_.url);
-
   // Update the load flags with cache information.
   UpdateLoadFlagsWithCacheFlags(&begin_params_->load_flags,
                                 common_params_.navigation_type,
@@ -811,6 +800,21 @@ void NavigationRequest::CreateNavigationHandle(bool is_for_commit) {
   DCHECK(frame_tree_node_->navigation_request() == this || is_for_commit);
   FrameTreeNode* frame_tree_node = frame_tree_node_;
 
+  // This is needed to get site URLs and assign the expected RenderProcessHost.
+  // This is not always the same as |source_site_instance_|, as it only depends
+  // on the current frame host, and does not depend on |entry|.
+  // The |starting_site_instance_| needs to be set here instead of the
+  // constructor since a navigation can be started after the constructor and
+  // before here, which can set a different RenderFrameHost and a different
+  // starting SiteInstance.
+  starting_site_instance_ =
+      frame_tree_node->current_frame_host()->GetSiteInstance();
+
+  // TODO(alexmos): Using |starting_site_instance_|'s IsolationContext may not
+  // be correct for cross-BrowsingInstance redirects.
+  site_url_ = SiteInstanceImpl::GetSiteForURL(
+      starting_site_instance_->GetIsolationContext(), common_params_.url);
+
   // Compute the redirect chain.
   // TODO(clamy): Try to simplify this and have the redirects be part of
   // CommonNavigationParams.

third_party/blink/web_tests/TestExpectations

@@ -93,6 +93,7 @@ crbug.com/678482 http/tests/devtools/debugger/fetch-breakpoints.js [ Timeout Pas
 crbug.com/678482 virtual/nobinary-for-devtools/http/tests/devtools/debugger/fetch-breakpoints.js [ Timeout Pass ]
 crbug.com/678491 http/tests/misc/webtiming-no-origin.html [ Crash Pass ]
 crbug.com/765779 http/tests/loading/bad-server-subframe.html [ Failure ]
+crbug.com/793127 external/wpt/content-security-policy/securitypolicyviolation/upgrade-insecure-requests-reporting.https.html [ Crash ]
 crbug.com/793127 http/tests/security/upgrade-insecure-requests/iframe-upgrade.https.html [ Crash ]
 crbug.com/793127 virtual/outofblink-cors/http/tests/security/upgrade-insecure-requests/iframe-upgrade.https.html [ Crash ]
 crbug.com/801992 http/tests/misc/iframe-script-modify-attr.html [ Pass Crash ]