Fix RenderFrameHostChanged 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_));
 
+  // Notify the FrameTree that this RFH is going away, allowing it to shut down
+  // the corresponding RenderViewHost if it is no longer needed.
+  frame_tree_->UnregisterRenderFrameHost(this);

[Charlie Reis, 2015/03/02 18:38:25]

Moving this makes me nervous about UaFs.  Are we sure that no observers below
(e.g., of RenderFrameDeleted) will try to access the RenderFrameHost or
RenderViewHost?

At the least, we should add a comment about why this new ordering is important
if the old way didn't work.

[nasko, 2015/03/02 19:30:53]

I don't know if observers will need to access RenderViewHost, though we are
weaning off of it, so I'd like to understand of there are such cases.
In terms of RenderFrameHost, observers won't see RenderFrameDeleted as part o
fthis call, it is right below it.
I think the main goal of this is to align with how OnRenderProcessGone works,
which does FTN scrub before firing off the RenderFrameDeleted observer.
If it makes you nervous, I can look into not making this change in this CL and
see if causes any issues.

[Charlie Reis, 2015/03/02 22:44:30]

Yes, that's my concern, though.

I just looked more closely, and lots of WebContentsObservers use the
RenderFrameHost that's passed to RenderFrameDeleted.  (I see people calling its
GetParent, GetProcess, GetRoutingID, etc.)  It seems quite dangerous to delete
its RenderViewHost before we call RenderFrameDeleted.

 
I'm not sure I understand the OnRenderProcessGone comparison, but I would feel
better about leaving UnregisterRenderFrameHost after RenderFrameDeleted.

[Charlie Reis, 2015/03/02 22:58:49]

Oops, this didn't come up in the patch diffs, so I didn't realize you'd already
reverted it.  LGTM!

+
   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
-  // the corresponding RenderViewHost if it is no longer needed.
-  frame_tree_->UnregisterRenderFrameHost(this);
-
   // NULL out the swapout timer; in crash dumps this member will be null only if
   // the dtor has run.
   swapout_event_monitor_timeout_.reset();
 
   for (const auto& iter: visual_state_callbacks_) {
     iter.second.Run(false);
   }
 
   if (render_widget_host_)
     render_widget_host_->Cleanup();
@@ skipping 1699 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