Ensure RenderFrameHost & NavigationHandle are not destroyed during commit

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 <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 190 matching lines @@
       unload_ack_is_for_navigation_(false),
       is_loading_(false),
       pending_commit_(false),
       nav_entry_id_(0),
       accessibility_reset_token_(0),
       accessibility_reset_count_(0),
       no_create_browser_accessibility_manager_for_testing_(false),
       web_ui_type_(WebUI::kNoWebUI),
       pending_web_ui_type_(WebUI::kNoWebUI),
       should_reuse_web_ui_(false),
+      is_in_commit_(false),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   bool hidden = !!(flags & CREATE_RF_HIDDEN);
   frame_tree_->AddRenderViewHostRef(render_view_host_);
   GetProcess()->AddRoute(routing_id_, this);
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
       this));
   site_instance_->AddObserver(this);
 
@@ skipping 33 matching lines @@
         static_cast<InputRouterImpl*>(render_widget_host_->input_router());
     ir->SetFrameTreeNodeId(frame_tree_node_->frame_tree_node_id());
   }
 }
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
   // Release the WebUI instances before all else as the WebUI may accesses the
   // RenderFrameHost during cleanup.
   ClearAllWebUI();
 
+  CHECK(!is_in_commit_);
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
 
   site_instance_->RemoveObserver(this);
 
   if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   bool is_active = IsRFHStateActive(rfh_state_);
@@ skipping 804 matching lines @@
       // has not been notified about the start of the load yet. Do it now.
       if (!is_loading()) {
         bool was_loading = frame_tree_node()->frame_tree()->IsLoading();
         is_loading_ = true;
         frame_tree_node()->DidStartLoading(true, was_loading);
       }
       pending_commit_ = false;
     }
   }
 
+  // TODO(clamy): Remove this once enough data has been gathered for
+  // crbug.com/589365.
+  is_in_commit_ = true;
+  navigation_handle_->set_is_in_commit(true);
+
   accessibility_reset_count_ = 0;
   frame_tree_node()->navigator()->DidNavigate(this, validated_params);
 
+  // TODO(clamy): Remove this once enough data has been gathered for
+  // crbug.com/589365.
+  is_in_commit_ = false;
+  if (navigation_handle_.get())

[nasko, 2016/03/15 14:10:34]

Shouldn't the handle be destroyed once we've committed?

[clamy, 2016/03/15 14:16:16]

No, hence the TODO in Navigator.

+    navigation_handle_->set_is_in_commit(false);
+
   // For a top-level frame, there are potential security concerns associated
   // with displaying graphics from a previously loaded page after the URL in
   // the omnibar has been changed. It is unappealing to clear the page
   // immediately, but if the renderer is taking a long time to issue any
   // compositor output (possibly because of script deliberately creating this
   // situation) then we clear it after a while anyway.
   // See https://crbug.com/497588.
   if (frame_tree_node_->IsMainFrame() && GetView() &&
       !validated_params.was_within_same_page) {
     RenderWidgetHostImpl::From(GetView()->GetRenderWidgetHost())
@@ skipping 1611 matching lines @@
   FrameTreeNode* focused_frame_tree_node = frame_tree_->GetFocusedFrame();
   if (!focused_frame_tree_node)
     return;
   RenderFrameHostImpl* focused_frame =
       focused_frame_tree_node->current_frame_host();
   DCHECK(focused_frame);
   dst->focused_tree_id = focused_frame->GetAXTreeID();
 }
 
 }  // namespace content