Refactor sudden termination

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index f284e5765ae46ad8fe3d6ec78e6a8c38ec56c3af..06dadb87b81d61891669e876c0d2da520fefc625 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -137,6 +137,9 @@ RenderFrameHostImpl::RenderFrameHostImpl(RenderViewHostImpl* render_view_host,
       accessibility_reset_token_(0),
       accessibility_reset_count_(0),
       no_create_browser_accessibility_manager_for_testing_(false),
+      has_beforeunload_handlers_(false),
+      has_unload_handlers_(false),
+      override_sudden_termination_status_(false),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   frame_tree_->RegisterRenderFrameHost(this);
@@ -158,6 +161,8 @@ RenderFrameHostImpl::RenderFrameHostImpl(RenderViewHostImpl* render_view_host,
 }
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
+  if (!SuddenTerminationAllowed())
+    GetProcess()->SuddenTerminationChangedForFrame(true);

[Charlie Reis, 2015/01/23 06:01:32]

I don't think RPH should be tracking this state.  See my comment in RPHI.

[clamy, 2015/01/26 16:38:39]

Done.

   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -336,6 +341,10 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) {
     IPC_MESSAGE_HANDLER(AccessibilityHostMsg_FindInPageResult,
                         OnAccessibilityFindInPageResult)
     IPC_MESSAGE_HANDLER(FrameHostMsg_ToggleFullscreen, OnToggleFullscreen)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_BeforeUnloadHandlersPresenceChanged,

[Charlie Reis, 2015/01/23 06:01:32]

nit: Put these just before OnBeforeUnloadACK, since they're conceptually
related.

[clamy, 2015/01/26 16:38:39]

Done.

+                        OnBeforeUnloadHandlersPresenceChanged)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_UnloadHandlersPresenceChanged,
+                        OnUnloadHandlersPresenceChanged)
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
     IPC_MESSAGE_HANDLER(FrameHostMsg_ShowPopup, OnShowPopup)
     IPC_MESSAGE_HANDLER(FrameHostMsg_HidePopup, OnHidePopup)
@@ -888,6 +897,11 @@ bool RenderFrameHostImpl::IsWaitingForUnloadACK() const {
       rfh_state_ == STATE_PENDING_SWAP_OUT;
 }
 
+bool RenderFrameHostImpl::SuddenTerminationAllowed() const {
+  return override_sudden_termination_status_ ||
+      (!has_beforeunload_handlers_ && !has_unload_handlers_);
+}
+
 void RenderFrameHostImpl::OnSwapOutACK() {
   OnSwappedOut();
 }
@@ -1152,6 +1166,16 @@ void RenderFrameHostImpl::OnToggleFullscreen(bool enter_fullscreen) {
   render_view_host_->WasResized();
 }
 
+void RenderFrameHostImpl::OnBeforeUnloadHandlersPresenceChanged(bool present) {
+  HandleSuddenTerminationDisablerPresenceChange(
+      &has_beforeunload_handlers_, present);
+}
+
+void RenderFrameHostImpl::OnUnloadHandlersPresenceChanged(bool present) {
+  HandleSuddenTerminationDisablerPresenceChange(
+      &has_unload_handlers_, present);
+}
+
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
 void RenderFrameHostImpl::OnShowPopup(
     const FrameHostMsg_ShowPopup_Params& params) {
@@ -1648,4 +1672,15 @@ void RenderFrameHostImpl::DidUseGeolocationPermission() {
       top_frame->GetLastCommittedURL().GetOrigin());
 }
 
+void RenderFrameHostImpl::HandleSuddenTerminationDisablerPresenceChange(

[Charlie Reis, 2015/01/23 06:01:32]

Hmm, this method seems awkward to me.
 - The method name is a bit unwieldy.
 - Taking in a pointer to a bool to modify seems unnecessarily risky.
 - The parameter names are not intuitive.

Can we find a better way to handle this?  It's small enough that I'd be ok
duplicating the code in the two call sites instead, rather than making the
reader try to follow this.

[clamy, 2015/01/26 16:38:39]

Done. If we don't signal the RPHI then only the boolean values need to be
updated.

+    bool* disabler_presence,
+    bool new_value) {
+  bool sudden_termination_before = SuddenTerminationAllowed();
+  *disabler_presence = new_value;
+  if (sudden_termination_before != SuddenTerminationAllowed()) {
+    GetProcess()->SuddenTerminationChangedForFrame(
+        SuddenTerminationAllowed());
+  }
+}
+
 }  // namespace content