Refactor sudden termination

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
@@ skipping 119 matching lines @@
       frame_tree_(frame_tree),
       frame_tree_node_(frame_tree_node),
       routing_id_(routing_id),
       render_frame_created_(false),
       navigations_suspended_(false),
       is_waiting_for_beforeunload_ack_(false),
       unload_ack_is_for_cross_site_transition_(false),
       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);
   GetProcess()->AddRoute(routing_id_, this);
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
       this));
 
   if (is_swapped_out) {
     rfh_state_ = STATE_SWAPPED_OUT;
   } else {
     rfh_state_ = STATE_DEFAULT;
     GetSiteInstance()->increment_active_frame_count();
   }
 
   SetUpMojoIfNeeded();
   swapout_event_monitor_timeout_.reset(new TimeoutMonitor(base::Bind(
       &RenderFrameHostImpl::OnSwappedOut, weak_ptr_factory_.GetWeakPtr())));
 }
 
 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_));
 
   if (delegate_)
     delegate_->RenderFrameDeleted(this);
 
   FrameAccessibility::GetInstance()->OnRenderFrameHostDestroyed(this);
 
   // If this was swapped out, it already decremented the active frame count of
@@ skipping 158 matching lines @@
     IPC_MESSAGE_HANDLER(FrameHostMsg_BeginNavigation,
                         OnBeginNavigation)
     IPC_MESSAGE_HANDLER(FrameHostMsg_TextSurroundingSelectionResponse,
                         OnTextSurroundingSelectionResponse)
     IPC_MESSAGE_HANDLER(AccessibilityHostMsg_Events, OnAccessibilityEvents)
     IPC_MESSAGE_HANDLER(AccessibilityHostMsg_LocationChanges,
                         OnAccessibilityLocationChanges)
     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)
 #endif
   IPC_END_MESSAGE_MAP()
 
   // No further actions here, since we may have been deleted.
   return handled;
 }
 
@@ skipping 532 matching lines @@
   // If canceled, notify the delegate to cancel its pending navigation entry.
   if (!proceed)
     render_view_host_->GetDelegate()->DidCancelLoading();
 }
 
 bool RenderFrameHostImpl::IsWaitingForUnloadACK() const {
   return render_view_host_->is_waiting_for_close_ack_ ||
       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();
 }
 
 void RenderFrameHostImpl::OnSwappedOut() {
   // Ignore spurious swap out ack.
   if (rfh_state_ != STATE_PENDING_SWAP_OUT)
     return;
 
   TRACE_EVENT_ASYNC_END0("navigation", "RenderFrameHostImpl::SwapOut", this);
@@ skipping 244 matching lines @@
   if (enter_fullscreen)
     delegate_->EnterFullscreenMode(GetLastCommittedURL().GetOrigin());
   else
     delegate_->ExitFullscreenMode();
 
   // The previous call might change the fullscreen state. We need to make sure
   // the renderer is aware of that, which is done via the resize message.
   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) {
   RenderViewHostDelegateView* view =
       render_view_host_->delegate_->GetDelegateView();
   if (view) {
     view->ShowPopupMenu(this,
                         params.bounds,
                         params.item_height,
                         params.item_font_size,
@@ skipping 476 matching lines @@
 
 void RenderFrameHostImpl::DidUseGeolocationPermission() {
   RenderFrameHost* top_frame = frame_tree_node()->frame_tree()->GetMainFrame();
   GetContentClient()->browser()->RegisterPermissionUsage(
       PERMISSION_GEOLOCATION,
       delegate_->GetAsWebContents(),
       GetLastCommittedURL().GetOrigin(),
       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