Don't clear last committed URL on pending delete RenderFrameHosts

Now that last committed URLs are tracked on RFH instead of
FrameTreeNode, there's no harm in keeping this URL around after a RFH
becomes pending deletion.  Keeping it might be useful for security
checks on such RFHs, and not keeping it might actually lead to
correctness issues when last committed URLs are checked for pending
delete RFHs.

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

content/browser/frame_host/frame_tree_node.cc

@@ -262,9 +262,6 @@ void FrameTreeNode::RemoveChild(FrameTreeNode* child) {
 }
 
 void FrameTreeNode::ResetForNewProcess() {
-  current_frame_host()->SetLastCommittedUrl(GURL());
-  blame_context_.TakeSnapshot();
-
   // Remove child nodes from the tree, then delete them. This destruction
   // operation will notify observers.
   std::vector<std::unique_ptr<FrameTreeNode>>().swap(children_);

content/browser/frame_host/frame_tree_node_blame_context_unittest.cc

@@ -208,7 +208,7 @@ TEST_F(FrameTreeNodeBlameContextTest, URLChange) {
   trace_analyzer::Start("*");
   root()->SetCurrentURL(url1);
   root()->SetCurrentURL(url2);
-  root()->ResetForNewProcess();
+  root()->SetCurrentURL(GURL());
   auto analyzer = trace_analyzer::Stop();
 
   trace_analyzer::TraceEventVector events;

content/browser/frame_host/render_frame_host_impl.cc

@@ -1905,6 +1905,7 @@ void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
   SetRenderFrameCreated(false);
   InvalidateMojoConnection();
   document_scoped_interface_provider_binding_.Close();
+  SetLastCommittedUrl(GURL());
 
   // Execute any pending AX tree snapshot callbacks with an empty response,
   // since we're never going to get a response from this renderer.

content/browser/frame_host/render_frame_host_impl.h

@@ -757,6 +757,8 @@ class CONTENT_EXPORT RenderFrameHostImpl
                            ContextMenuAfterCrossProcessNavigation);
   FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
                            ActiveSandboxFlagsRetainedAfterSwapOut);
+  FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
+                           LastCommittedURLRetainedAfterSwapOut);
   FRIEND_TEST_ALL_PREFIXES(SecurityExploitBrowserTest,
                            AttemptDuplicateRenderViewHost);
 

content/browser/site_per_process_browsertest.cc

@@ -11481,4 +11481,36 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   EXPECT_TRUE(success);
 }
 
+// Tests that the last committed URL is preserved on an RFH even after the RFH
+// goes into the pending deletion state.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       LastCommittedURLRetainedAfterSwapOut) {
+  // Navigate to a.com.
+  GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+  RenderFrameHostImpl* rfh = web_contents()->GetMainFrame();
+  EXPECT_EQ(start_url, rfh->GetLastCommittedURL());
+
+  // Disable the swapout ACK and the swapout timer.
+  scoped_refptr<SwapoutACKMessageFilter> filter = new SwapoutACKMessageFilter();
+  rfh->GetProcess()->AddFilter(filter.get());
+  rfh->DisableSwapOutTimerForTesting();
+
+  // Open a popup on a.com to keep the process alive.
+  OpenPopup(shell(), embedded_test_server()->GetURL("a.com", "/title2.html"),
+            "foo");
+
+  // Navigate cross-process to b.com.
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("b.com", "/title3.html")));
+
+  // The old RFH should be pending deletion.
+  EXPECT_FALSE(rfh->is_active());
+  EXPECT_FALSE(rfh->IsCurrent());
+  EXPECT_NE(rfh, web_contents()->GetMainFrame());
+
+  // Check that it still has a valid last committed URL.
+  EXPECT_EQ(start_url, rfh->GetLastCommittedURL());
+}
+
 }  // namespace content