Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

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 166 matching lines @@
         rwh_delegate, GetProcess(), MSG_ROUTING_NONE, hidden));
     render_widget_host_->set_owned_by_render_frame_host(true);
   }
 }
 
 RenderFrameHostImpl::~RenderFrameHostImpl() {
   GetProcess()->RemoveRoute(routing_id_);
   g_routing_id_frame_map.Get().erase(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_));
 
-  if (delegate_)
+  if (delegate_ && render_frame_created_)
     delegate_->RenderFrameDeleted(this);
 
   FrameAccessibility::GetInstance()->OnRenderFrameHostDestroyed(this);
 
   // If this was swapped out, it already decremented the active frame count of
   // the SiteInstance it belongs to.
   if (IsRFHStateActive(rfh_state_))
     GetSiteInstance()->decrement_active_frame_count();
 
   // Notify the FrameTree that this RFH is going away, allowing it to shut down
@@ skipping 414 matching lines @@
       render_view_host_->IsRenderViewLive() :
       GetProcess()->HasConnection() && render_frame_created_;
 
   // Sanity check: the RenderView should always be live if the RenderFrame is.
   DCHECK(!is_live || render_view_host_->IsRenderViewLive());
 
   return is_live;
 }
 
 void RenderFrameHostImpl::SetRenderFrameCreated(bool created) {
+  // If the current status is different than the new status, the delegate
+  // needs to be notified.
+  if (delegate_ && (created ^ render_frame_created_)) {

[Charlie Reis, 2015/02/12 00:29:31]

I feel like != would be clearer than ^.

[nasko, 2015/02/12 17:52:26]

I think so too :)

+    if (created)
+      delegate_->RenderFrameCreated(this);
+    else
+      delegate_->RenderFrameDeleted(this);
+  }
+
   render_frame_created_ = created;
   if (created && render_widget_host_)
     render_widget_host_->InitForFrame();
 }
 
 void RenderFrameHostImpl::Init() {
   GetProcess()->ResumeRequestsForView(routing_id_);
 }
 
 void RenderFrameHostImpl::OnAddMessageToConsole(
@@ skipping 24 matching lines @@
   // frame and it is delivered after this host is swapped out.
   // Ignore such messages, as we know this RenderFrameHost is going away.
   if (rfh_state_ != RenderFrameHostImpl::STATE_DEFAULT)
     return;
 
   RenderFrameHostImpl* new_frame = frame_tree_->AddFrame(
       frame_tree_node_, GetProcess()->GetID(), new_routing_id, frame_name);
   if (!new_frame)
     return;
 
+  new_frame->frame_tree_node()->set_sandbox_flags(sandbox_flags);
+
   // We know that the RenderFrame has been created in this case, immediately
   // after the CreateChildFrame IPC was sent.
   new_frame->SetRenderFrameCreated(true);
-
-  new_frame->frame_tree_node()->set_sandbox_flags(sandbox_flags);
-
-  if (delegate_)
-    delegate_->RenderFrameCreated(new_frame);
 }
 
 void RenderFrameHostImpl::OnDetach() {
   frame_tree_->RemoveFrame(frame_tree_node_);
 }
 
 void RenderFrameHostImpl::OnFrameFocused() {
   frame_tree_->SetFocusedFrame(frame_tree_node_);
 }
 
@@ skipping 352 matching lines @@
 }
 
 void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
   if (frame_tree_node_->IsMainFrame()) {
     // Keep the termination status so we can get at it later when we
     // need to know why it died.
     render_view_host_->render_view_termination_status_ =
         static_cast<base::TerminationStatus>(status);
   }
 
-  SetRenderFrameCreated(false);
-  InvalidateMojoConnection();
-
   // Reset frame tree state associated with this process.  This must happen
   // before RenderViewTerminated because observers expect the subframes of any

[Charlie Reis, 2015/02/12 00:29:31]

I'm not sure why these lines were moved lower.  If it's for a similar reason as
this comment about RenderViewTerminated, let's update the comment.

[nasko, 2015/02/12 17:52:26]

The reason I moved them is that it doesn't make sense to invalidate the node
which we are currently on before invalidating its children. I don't recall if
I've seen an actual problem.

   // affected frames to be cleared first.
   // Note: When a RenderFrameHost is swapped out there is a different one
   // which is the current host. In this case, the FrameTreeNode state must
   // not be reset.
   if (!is_swapped_out())
     frame_tree_node_->ResetForNewProcess();
 
+  SetRenderFrameCreated(false);
+  InvalidateMojoConnection();
+
   if (frame_tree_node_->IsMainFrame()) {
     // RenderViewHost/RenderWidgetHost needs to reset some stuff.
     render_view_host_->RendererExited(
         render_view_host_->render_view_termination_status_, exit_code);
 
     render_view_host_->delegate_->RenderViewTerminated(
         render_view_host_, static_cast<base::TerminationStatus>(status),
         exit_code);
   }
 }
@@ skipping 811 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());
 }
 
 }  // namespace content