Fix RenderFrameHostChanged 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 38f4192c62ab9c49290126814c0db5313c88ade3..21e0ecbd32e2eccb9c1ebd0bdf80e407ea658da9 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -184,6 +184,10 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   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);
 
@@ -194,10 +198,6 @@ RenderFrameHostImpl::~RenderFrameHostImpl() {
   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();