Revert 251090 "Revert 250823 "With --site-per-process, avoid a c..."

Relanding, this wasn't the cause of the failures.

> Revert 250823 "With --site-per-process, avoid a crash when a sub..."
> 
> Speculative revert to see if the navigation changes here are causing
> sync_integration_tests to fail on:
> 
> http://build.chromium.org/p/chromium.win/builders/Win7%20Sync%20x64/builds/11217
> 
> > With --site-per-process, avoid a crash when a subframe process goes away.
> > 
> > We need to clear out the children of any nodes that are affected by the
> > crash, not the entire FrameTree.
> > 
> > BUG=338508
> > TEST=Killing an iframe process with --site-per-process shows a green rectangle.
> > R=ajwong@chromium.org, nasko@chromium.org
> > 
> > Review URL: https://codereview.chromium.org/156303004
> 
> TBR=creis@chromium.org
> 
> Review URL: https://codereview.chromium.org/163703004

TBR=asvitkine@chromium.org

Review URL: https://codereview.chromium.org/163183005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251110 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/frame_host/frame_tree.cc

@@ -43,6 +43,15 @@ bool FrameTreeNodeForFrameId(int64 frame_id,
   return true;
 }
 
+// Iterate over the FrameTree to reset any node affected by the loss of the
+// given RenderViewHost's process.
+bool ResetNodesForNewProcess(RenderViewHost* render_view_host,
+                             FrameTreeNode* node) {
+  if (render_view_host == node->current_frame_host()->render_view_host())
+    node->ResetForNewProcess();
+  return true;
+}
+
 }  // namespace
 
 FrameTree::FrameTree(Navigator* navigator,
@@ -153,7 +162,17 @@ void FrameTree::SetFrameUrl(int64 frame_id, const GURL& url) {
 }
 
 void FrameTree::ResetForMainFrameSwap() {
-  return root_->ResetForMainFrameSwap();
+  root_->ResetForNewProcess();
+}
+
+void FrameTree::RenderProcessGone(RenderViewHost* render_view_host) {
+  // Walk the full tree looking for nodes that may be affected.  Once a frame
+  // crashes, all of its child FrameTreeNodes go away.
+  // Note that the helper function may call ResetForNewProcess on a node, which
+  // clears its children before we iterate over them.  That's ok, because
+  // ForEach does not add a node's children to the queue until after visiting
+  // the node itself.
+  ForEach(base::Bind(&ResetNodesForNewProcess, render_view_host));
 }
 
 RenderFrameHostImpl* FrameTree::GetMainFrame() const {

content/browser/frame_host/frame_tree.h

@@ -60,7 +60,9 @@ class CONTENT_EXPORT FrameTree {
 
   // Executes |on_node| on each node in the frame tree.  If |on_node| returns
   // false, terminates the iteration immediately. Returning false is useful
-  // if |on_node| is just doing a search over the tree.
+  // if |on_node| is just doing a search over the tree.  The iteration proceeds
+  // top-down and visits a node before adding its children to the queue, making
+  // it safe to remove children during the callback.
   void ForEach(const base::Callback<bool(FrameTreeNode*)>& on_node) const;
 
   // After the FrameTree is created, or after SwapMainFrame() has been called,
@@ -92,6 +94,13 @@ class CONTENT_EXPORT FrameTree {
   // TODO(creis): Look into how we can remove the need for this method.
   void ResetForMainFrameSwap();
 
+  // Update the frame tree after a process exits.  Any nodes currently using the
+  // given |render_view_host| will lose all their children.
+  // TODO(creis): This should take a RenderProcessHost once RenderFrameHost
+  // knows its process.  Until then, we would just be asking the RenderViewHost
+  // for its process, so we'll skip that step.
+  void RenderProcessGone(RenderViewHost* render_view_host);
+
   // Convenience accessor for the main frame's RenderFrameHostImpl.
   RenderFrameHostImpl* GetMainFrame() const;
 

content/browser/frame_host/frame_tree_node.cc

@@ -72,13 +72,13 @@ void FrameTreeNode::RemoveChild(FrameTreeNode* child) {
   }
 }
 
-void FrameTreeNode::ResetForMainFrameSwap() {
+void FrameTreeNode::ResetForNewProcess() {
   frame_id_ = kInvalidFrameId;
   current_url_ = GURL();
 
   // The children may not have been cleared if a cross-process navigation
   // commits before the old process cleans everything up.  Make sure the child
-  // nodes get deleted.
+  // nodes get deleted before swapping to a new process.
   children_.clear();
 }
 

content/browser/frame_host/frame_tree_node.h

@@ -46,9 +46,8 @@ class CONTENT_EXPORT FrameTreeNode {
   void AddChild(scoped_ptr<FrameTreeNode> child, int frame_routing_id);
   void RemoveChild(FrameTreeNode* child);
 
-  // Clears process specific-state after a main frame process swap.
-  // TODO(creis): Look into how we can remove the need for this method.
-  void ResetForMainFrameSwap();
+  // Clears process specific-state in this node to prepare for a new process.
+  void ResetForNewProcess();
 
   FrameTree* frame_tree() const {
     return frame_tree_;

content/browser/renderer_host/render_view_host_impl.cc

@@ -1431,11 +1431,9 @@ void RenderViewHostImpl::OnRenderProcessGone(int status, int exit_code) {
   render_view_termination_status_ =
       static_cast<base::TerminationStatus>(status);
 
-  // Reset frame tree state.
-  // TODO(creis): Once subframes can be in different processes, we'll need to
-  // clear just the FrameTreeNodes affected by the crash (and their subtrees).
+  // Reset frame tree state associated with this process.
   main_frame_id_ = -1;
-  delegate_->GetFrameTree()->ResetForMainFrameSwap();
+  delegate_->GetFrameTree()->RenderProcessGone(this);
 
   // Our base class RenderWidgetHost needs to reset some stuff.
   RendererExited(render_view_termination_status_, exit_code);

content/browser/site_per_process_browsertest.cc

@@ -258,6 +258,68 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CrossSiteIframe) {
             child->current_frame_host()->GetProcess());
 }
 
+// Crash a subframe and ensures its children are cleared from the FrameTree.
+// See http://crbug.com/338508.
+// TODO(creis): Enable this on Android when we can kill the process there.
+#if defined(OS_ANDROID)
+#define MAYBE_CrashSubframe DISABLED_CrashSubframe
+#else
+#define MAYBE_CrashSubframe CrashSubframe
+#endif
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, MAYBE_CrashSubframe) {
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(test_server()->Start());
+  GURL main_url(test_server()->GetURL("files/site_per_process_main.html"));
+  NavigateToURL(shell(), main_url);
+
+  StartFrameAtDataURL();
+
+  // These must stay in scope with replace_host.
+  GURL::Replacements replace_host;
+  std::string foo_com("foo.com");
+
+  // Load cross-site page into iframe.
+  GURL cross_site_url(test_server()->GetURL("files/title2.html"));
+  replace_host.SetHostStr(foo_com);
+  cross_site_url = cross_site_url.ReplaceComponents(replace_host);
+  EXPECT_TRUE(NavigateIframeToURL(shell(), cross_site_url, "test"));
+
+  // Check the subframe process.
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+  ASSERT_EQ(1U, root->child_count());
+  FrameTreeNode* child = root->child_at(0);
+  EXPECT_NE(FrameTreeNode::kInvalidFrameId, root->frame_id());
+  EXPECT_NE(FrameTreeNode::kInvalidFrameId, root->child_at(0)->frame_id());
+
+  // Crash the subframe process.
+  RenderProcessHost* root_process = root->current_frame_host()->GetProcess();
+  RenderProcessHost* child_process = child->current_frame_host()->GetProcess();
+  {
+    RenderProcessHostWatcher crash_observer(
+        child_process,
+        RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+    base::KillProcess(child_process->GetHandle(), 0, false);
+    crash_observer.Wait();
+  }
+
+  // Ensure that the child frame still exists but has been cleared.
+  EXPECT_EQ(1U, root->child_count());
+  EXPECT_EQ(FrameTreeNode::kInvalidFrameId, root->child_at(0)->frame_id());
+
+  // Now crash the top-level page to clear the child frame.
+  {
+    RenderProcessHostWatcher crash_observer(
+        root_process,
+        RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+    base::KillProcess(root_process->GetHandle(), 0, false);
+    crash_observer.Wait();
+  }
+  EXPECT_EQ(0U, root->child_count());
+  EXPECT_EQ(FrameTreeNode::kInvalidFrameId, root->frame_id());
+}
+
 // TODO(nasko): Disable this test until out-of-process iframes is ready and the
 // security checks are back in place.
 // TODO(creis): Replace SpawnedTestServer with host_resolver to get test to run