Fix leaking of RenderFrames.

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 6bb654a15b58f2bf84fac0ded993a0b6cd45f212..2f37ca326999886e4447044226f2be669eb3944b 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -28,6 +28,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test_utils.h"
@@ -3626,4 +3627,77 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, OpenerSetLocation) {
   EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(), cross_url);
 }
 
+// This test ensures that RenderFrame isn't leaked in the renderer process if

[Charlie Reis, 2015/10/29 17:28:58]

nit: that the

[nasko, 2015/10/29 19:09:45]

Done.

+// a pending cross-process navigation is cancelled. The test works by by trying
+// to create a new RenderFrame with the same routing id. If there is an
+// entry with the same routing ID, a CHECK is hit and the process crashes.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       SubframePendingAndBackToSameSiteInstance) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  NavigateToURL(shell(), main_url);
+
+  // Capture the FrameTreeNode this test will be navigating.
+  FrameTreeNode* node = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root()
+                            ->child_at(0);
+  EXPECT_TRUE(node);
+  EXPECT_NE(node->current_frame_host()->GetSiteInstance(),
+            node->parent()->current_frame_host()->GetSiteInstance());
+
+  // Navigate to the site of the parent, but to a page that will not commit.
+  GURL same_site_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  NavigationStallDelegate stall_delegate(same_site_url);
+  ResourceDispatcherHost::Get()->SetDelegate(&stall_delegate);
+  {
+    NavigationController::LoadURLParams params(same_site_url);
+    params.transition_type = ui::PAGE_TRANSITION_LINK;
+    params.frame_tree_node_id = node->frame_tree_node_id();
+    node->navigator()->GetController()->LoadURLWithParams(params);
+  }
+
+  // Grab the routing id of the pending RenderFrameHost and setup a process

[Charlie Reis, 2015/10/29 17:28:58]

nit: set up

[nasko, 2015/10/29 19:09:45]

Done.

+  // observer to ensure there is no crash when a new RenderFrame creation is
+  // attempted.
+  RenderProcessHost* process =
+      node->render_manager()->pending_frame_host()->GetProcess();
+  RenderProcessHostWatcher watcher(
+      process, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  int frame_routing_id =
+      node->render_manager()->pending_frame_host()->GetRoutingID();
+  int proxy_routing_id =
+      node->render_manager()->GetProxyToParent()->GetRoutingID();
+
+  // Now go to c.com so the navigation to a.com is cancelled and send an IPC
+  // to create a new RenderFrame with the routing id of the previously pending
+  // one.
+  NavigateFrameToURL(node,
+                     embedded_test_server()->GetURL("c.com", "/title2.html"));
+  {
+    FrameMsg_NewFrame_Params params;
+    params.routing_id = frame_routing_id;
+    params.proxy_routing_id = proxy_routing_id;
+    params.opener_routing_id = MSG_ROUTING_NONE;
+    params.parent_routing_id =
+        shell()->web_contents()->GetMainFrame()->GetRoutingID();
+    params.previous_sibling_routing_id = MSG_ROUTING_NONE;
+    params.widget_params.routing_id = MSG_ROUTING_NONE;
+    params.widget_params.hidden = true;
+
+    process->Send(new FrameMsg_NewFrame(params));

[Charlie Reis, 2015/10/29 17:28:58]

I'm a bit surprised this works cleanly.  I guess we don't have sanity checks
that routing IDs won't be reused.  Would that cause problems for this test if
someone added such a sanity check?

Then again, I'm not sure of a better way to test that a RenderFrame doesn't
exist, since I don't see an IPC it sends during deletion.  Perhaps this is fine
for the time being.

[nasko, 2015/10/29 19:09:45]

Yes, we don't have any checks for possible reuse. This would require keeping a
set of all already seen routing ids and there is no such thing today. If someone
were to add this check, yes, it will cause this test to fail.
There are no IPCs during deletion. I also am not eager to add one just to allow
us to test memory leak. I'd rather investigate why our memory bots aren't
catching this case. Or maybe they are and we didn't have a test that
successfully exercised the leak.

+  }
+
+  // The test must wait for a process to exit, but if there is no leak, the
+  // RenderFrame will be properly created and there will be no crash.
+  // Therefore, navigate the main frame to completely different site, which
+  // will cause the original process to exit cleanly.
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL("d.com", "/title3.html")));
+  watcher.Wait();
+  EXPECT_TRUE(watcher.did_exit_normally());
+
+  ResourceDispatcherHost::Get()->SetDelegate(nullptr);
+}
+
 }  // namespace content