Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

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 641793ea6e1c8ad720f71c07612c5f853d77365b..700379f04ed1e626c44b8c645f5664345082b8a9 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -184,7 +184,7 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   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);
@@ -619,6 +619,15 @@ bool RenderFrameHostImpl::IsRenderFrameLive() {
 }
 
 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();
@@ -663,14 +672,11 @@ void RenderFrameHostImpl::OnCreateChildFrame(int new_routing_id,
   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() {
@@ -1043,9 +1049,6 @@ void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
         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.
@@ -1055,6 +1058,9 @@ void RenderFrameHostImpl::OnRenderProcessGone(int status, int exit_code) {
   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(