Ensure RenderFrameHost & NavigationHandle are not destroyed during commit

This CL adds a boolean to RenderFrameHost and NavigationHandle to check if they
are destroyed during a navigation commit, which would lead to a use-after-free
bug.

BUG=589365
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

Cr-Commit-Position: refs/heads/master@{#381713}

content/browser/frame_host/navigation_handle_impl.cc

@@ -65,7 +65,8 @@ NavigationHandleImpl::NavigationHandleImpl(
       frame_tree_node_(frame_tree_node),
       next_index_(0),
       navigation_start_(navigation_start),
-      pending_nav_entry_id_(pending_nav_entry_id) {
+      pending_nav_entry_id_(pending_nav_entry_id),
+      is_in_commit_(false) {
   DCHECK(!navigation_start.is_null());
   GetDelegate()->DidStartNavigation(this);
 }
@@ -73,6 +74,8 @@ NavigationHandleImpl::NavigationHandleImpl(
 NavigationHandleImpl::~NavigationHandleImpl() {
   GetDelegate()->DidFinishNavigation(this);
 
+  CHECK(!is_in_commit_);
+
   // Cancel the navigation on the IO thread if the NavigationHandle is being
   // destroyed in the middle of the NavigationThrottles checks.
   if (!IsBrowserSideNavigationEnabled() && !complete_callback_.is_null())

content/browser/frame_host/navigation_handle_impl.h

@@ -214,6 +214,10 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
       bool same_page,
       RenderFrameHostImpl* render_frame_host);
 
+  // TODO(clamy): Remove this once enough data has been gathered for
+  // crbug.com/589365.
+  void set_is_in_commit(bool is_in_commit) { is_in_commit_ = is_in_commit; }
+
  private:
   friend class NavigationHandleImplTest;
 
@@ -295,6 +299,12 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   // corresponding ServiceWorkerNetworkProvider is created in the renderer.
   scoped_ptr<ServiceWorkerNavigationHandle> service_worker_handle_;
 
+  // True if the RenderFrameHost that owns the NavigationHandle is in the
+  // process of committing a navigation.  This is temporary to help pinpoint
+  // the cause of crbug.com/589365.
+  // TODO(clamy): Remove once enough data has been gathered.
+  bool is_in_commit_;
+
   DISALLOW_COPY_AND_ASSIGN(NavigationHandleImpl);
 };
 

content/browser/frame_host/navigator_impl.cc

@@ -567,9 +567,16 @@ void NavigatorImpl::DidNavigate(
                                         transition_type);
     render_frame_host->navigation_handle()->DidCommitNavigation(
         params, is_navigation_within_page, render_frame_host);
+
+    // TODO(clamy): Remove this once enough data has been gathered for
+    // crbug.com/589365.
+    render_frame_host->navigation_handle()->set_is_in_commit(false);
+
     render_frame_host->SetNavigationHandle(nullptr);
   }
 
+  // TODO(clamy): The NavigationHandle should always be reset here.
+
   if (!did_navigate)
     return;  // No navigation happened.
 

content/browser/frame_host/render_frame_host_impl.cc

@@ -209,6 +209,7 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       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 hidden = !!(flags & CREATE_RF_HIDDEN);
   frame_tree_->AddRenderViewHostRef(render_view_host_);
@@ -255,6 +256,8 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   // RenderFrameHost during cleanup.
   ClearAllWebUI();
 
+  CHECK(!is_in_commit_);
+
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
@@ -1064,9 +1067,20 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
     }
   }
 
+  // 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())
+    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

content/browser/frame_host/render_frame_host_impl.h

@@ -932,6 +932,11 @@ class CONTENT_EXPORT RenderFrameHostImpl : public RenderFrameHost,
   // called (no pending instance should be set).
   bool should_reuse_web_ui_;
 
+  // True if the RenderFrameHost is in the process of committing a navigation.
+  // This is temporary to help pinpoint the cause of crbug.com/589365.
+  // TODO(clamy): Remove once enough data has been gathered.
+  bool is_in_commit_;
+
   // NOTE: This must be the last member.
   base::WeakPtrFactory<RenderFrameHostImpl> weak_ptr_factory_;