Refactor sudden termination

content/browser/renderer_host/render_view_host_impl.cc

Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index 1626be71108364831440b660f1e222747ea65ca2..a04a2384b9c7b100d4ee38a6fccd9d4ce68ab5f1 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -125,6 +125,19 @@ void DismissVirtualKeyboardTask() {
 }
 #endif
 
+// Enable sudden termination for the current RenderFrameHost of
+// |frame_tree_node| if its SiteInstance is |site_instance|.  Used with
+// FrameTree::ForEach.
+bool EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,

[clamy, 2015/02/03 12:54:03]

Note that since SiteInstance is refcounted, we need to have a scoped_refptr as
an argument of the function to be able to bind it.

[Charlie Reis, 2015/02/03 19:33:15]

I don't think that's true, is it?  It only needs to be a scoped_refptr in cases
that ownership is involved.

There's no chance that the SiteInstance will be deleted in this function, since
it's purely used for comparison and there aren't any actions here that could
cause it to go away.

We should just pass a raw pointer to it from the call sites.

[clamy, 2015/02/04 13:16:19]

There is a static assert in base::Bind preventing the code to compile if trying
to bind a raw pointer of a ref_counted class
(https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&l=100&...).
I agree that the scoped_refptr makes no sense from an ownership point of view (I
had written the raw pointer version before hitting the compile assert). It seems
that SiteInstance has a unique ID, so I went with comparing that instead.
Another option would be to have SiteInstance support weak pointers and bind a
weak pointer but I don't think that's desirable.

[Charlie Reis, 2015/02/04 21:34:57]

Ah, base::Bind...  Sorry for missing that aspect before.

Yes, using the SiteInstance ID seems like a reasonable workaround here.

+                             FrameTreeNode* frame_tree_node) {
+  if (frame_tree_node->current_frame_host()->GetSiteInstance()
+      == site_instance.get()) {
+    frame_tree_node->current_frame_host()
+        ->set_override_sudden_termination_status(true);
+  }
+  return true;
+}
+
 }  // namespace
 
 // static
@@ -183,7 +196,6 @@ RenderViewHostImpl::RenderViewHostImpl(
       run_modal_reply_msg_(NULL),
       run_modal_opener_id_(MSG_ROUTING_NONE),
       is_waiting_for_close_ack_(false),
-      sudden_termination_allowed_(false),
       render_view_termination_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       virtual_keyboard_requested_(false),
       is_focused_element_editable_(false),
@@ -555,7 +567,10 @@ void RenderViewHostImpl::ClosePageIgnoringUnloadEvents() {
   StopHangMonitorTimeout();
   is_waiting_for_close_ack_ = false;
 
-  sudden_termination_allowed_ = true;
+  // Enable sudden termination for all RenderFrameHosts in the FrameTree.
+  FrameTree* frame_tree = static_cast<RenderFrameHostImpl*>(GetMainFrame())
+      ->frame_tree_node()->frame_tree();
+  frame_tree->ForEach(base::Bind(&EnableSuddenTermination, instance_));

[Charlie Reis, 2015/02/03 19:33:15]

instance_.get()

   delegate_->Close(this);
 }
 
@@ -846,11 +861,6 @@ void RenderViewHostImpl::LoadStateChanged(
   delegate_->LoadStateChanged(url, load_state, upload_position, upload_size);
 }
 
-bool RenderViewHostImpl::SuddenTerminationAllowed() const {
-  return sudden_termination_allowed_ ||
-      GetProcess()->SuddenTerminationAllowed();
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 // RenderViewHostImpl, IPC message handlers: