Convert main_render_frame_ to raw pointer in RenderViewImpl.

content/renderer/render_frame_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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 712 matching lines @@
 
 RenderFrameImpl::~RenderFrameImpl() {
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, RenderFrameGone());
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, OnDestruct());
 
 #if defined(VIDEO_HOLE)
   if (contains_media_player_)
     render_view_->UnregisterVideoHoleFrame(this);
 #endif
 
-  // RenderFrameProxy is only "owned" by RenderFrameImpl in the case it is the
-  // main frame. Ensure it is deleted along with this object.
-  if (!is_subframe_ && render_frame_proxy_) {
-    // The following method calls back into this object and clears
-    // |render_frame_proxy_|.
-    render_frame_proxy_->frameDetached();
+  if (!is_subframe_) {
+    // RenderFrameProxy is "owned" by RenderFrameImpl in the case it is
+    // the main frame. Ensure it is deleted along with this object.
+    if (render_frame_proxy_) {
+      // The following method calls back into this object and clears
+      // |render_frame_proxy_|.
+      render_frame_proxy_->frameDetached();
+    }
+
+    // Since this is the main frame, the RenderViewImpl pointer to this object
+    // must be cleared, otherwise it will result in use-after-free bugs.

[ncarter (slow), 2015/05/11 18:27:13]

"otherwise it will result in use-after-free bugs" seems a bit meta. It's fine to
say only that we need to eliminate a reference to this object: we're in the
dtor; the motivation is obvious.

A check like the following might make it clearer:

if (render_view->main_render_frame_) {
  CHECK_EQ(render_view->main_render_frame_, this);
  render_view->main_render_frame_ = nullptr;
}

[nasko, 2015/05/11 22:01:11]

Done.

+    render_view_->main_render_frame_ = nullptr;
   }
 
   render_view_->UnregisterRenderFrame(this);
   g_routing_id_frame_map.Get().erase(routing_id_);
   RenderThread::Get()->RemoveRoute(routing_id_);
 }
 
 void RenderFrameImpl::SetWebFrame(blink::WebLocalFrame* web_frame) {
   DCHECK(!frame_);
 
@@ skipping 1475 matching lines @@
     }
     frame->parent()->removeChild(frame);
   }
 
   // |frame| is invalid after here.  Be sure to clear frame_ as well, since this
   // object may not be deleted immediately and other methods may try to access
   // it.
   frame->close();
   frame_ = nullptr;
 
-  if (is_subframe_) {
-    delete this;
-    // Object is invalid after this point.
-  }
+  delete this;
+  // Object is invalid after this point.
 }
 
 void RenderFrameImpl::frameFocused() {
   Send(new FrameHostMsg_FrameFocused(routing_id_));
 }
 
 void RenderFrameImpl::willClose(blink::WebFrame* frame) {
   DCHECK(!frame_ || frame_ == frame);
 
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, FrameWillClose());
@@ skipping 388 matching lines @@
       DocumentState::FromDataSource(frame->dataSource());
   NavigationStateImpl* navigation_state =
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
         RenderFrameProxy::FromRoutingID(proxy_routing_id_);
     CHECK(proxy);
     proxy->web_frame()->swap(frame_);
     proxy_routing_id_ = MSG_ROUTING_NONE;
+
+    // If this is the main frame going from a remote frame to a local frame,
+    // it needs to set RenderViewImpl's pointer for the main frame to itself.
+    if (!is_subframe_) {
+      CHECK(!render_view_->main_render_frame_);

[ncarter (slow), 2015/05/11 18:27:13]

How does this happen?

Seems like RenderViewImpl::Create always returns a render_view_ with a
main_render_frame_ set by Initialize(). I'm not seeing the codepath that would
let us wind up with a RenderView with a null main_render_frame_?

[nasko, 2015/05/11 22:01:11]

Create RenderView and the main RenderFrame. Navigate the frame cross process,
which replaces the RenderFrame with RenderFrameProxy. In the destructor of
RenderFrame, we nullify the main_render_frame_ pointer, while RenderView is
still alive. Later on, we navigate that same frame back to this process,
resulting in replacing the RenderFrameProxy with a new RenderFrame. They all use
the same RenderView, so we need to ensure we update the main_render_frame_
pointer to the new RenderFrame object. It happens in this block of code.

Should I be including more in the comment? It was supposed to described this,
but in less words. It failed though : (.

[ncarter (slow), 2015/05/11 22:44:49]

No, it's fine and this makes sense. I had forgotten that the render_view_ could
keep on living like that after its original main frame went remote.

+      render_view_->main_render_frame_ = this;
+    }
   }
 
   // When we perform a new navigation, we need to update the last committed
   // session history entry with state for the page we are leaving. Do this
   // before updating the HistoryController state.
   render_view_->UpdateSessionHistory(frame);
 
   render_view_->history_controller()->UpdateForCommit(
       this, item, commit_type, navigation_state->WasWithinSamePage());
 
@@ skipping 2278 matching lines @@
 #elif defined(ENABLE_BROWSER_CDMS)
         cdm_manager_,
 #endif
         this);
   }
 
   return cdm_factory_;
 }
 
 }  // namespace content