Always swap with a replacement proxy in OnSwapOut.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 29958841fffe598a1847d66da414a6a16353a047..2eb3be8faa668eddb1c8d748c675d406a9dde237 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1610,6 +1610,15 @@ void RenderFrameImpl::OnSwapOut(
 
     if (is_loading)
       proxy->OnDidStartLoading();
+  } else if (!is_main_frame) {

[alexmos, 2016/04/15 17:05:12]

I initially tried to do this for all frames for consistency, but ran into an
issue for main frames: this leaves the view's mainFrame() as null until the view
is closed, which should happen soon afterward.  That actually caused
ExtensionWebRequestApiTest.WebRequestBlocking to fail, where in that time
window, there was a zoom change, and RenderViewZoomer::Visit (which walks
through all RVs in the process) tried to reference this view's
webview->mainFrame() [1].  This zoom code is actually going away in James's
https://codereview.chromium.org/1804023002/, so we could try making that change
for all frames after that, though I don't know if something else might hit this
issue (there are a few other uses of RenderView::ForEach).  Essentially we'd be
trading a time window where a WebLocalFrame has no frameWidget() for a time
window where the webview doesn't have a mainFrame(), and I don't know which
one's worse. :)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[Charlie Reis, 2016/04/15 23:20:05]

On one hand, there are 400 references to WebView::mainFrame(), so having that
return null near shutdown does seem risky.  I suppose it's not a problem in the
case that we have a proxy during OnSwapOut because WebView::mainFrame() can
return a WebRemoteFrame.

On the other hand, could we ever be at risk of the current crash for main
frames?  Suppose we're swapping out the last main frame and not creating a
proxy.  Could we get to RenderWidget::Close() and clear the Web{Frame}Widget of
the main frame, setting up a crash if some code calls GetRenderWidget() from
RenderViewImpl::Close()?  (I realize we don't have a WebFrameWidget for the main
frame yet, but we plan to, and perhaps this applies to the WebWidget for now?)

It does seem unfortunate that we can't uniformly guarantee that the frame is
gone by the end of OnSwapOut-- it seems like it will create corner cases that
lead to more bugs like this in general.

[alexmos, 2016/04/22 05:09:34]

Agree with all your concerns, so I'm super-happy that this went away after
switching to always swap with a proxy.

+    // If there is no proxy, this is the last active frame in this
+    // SiteInstance, and its WebWidget and RenderView will be closed
+    // shortly afterward.  The closing of RenderView would eventually detach
+    // this frame along with any other frames in the frame tree, but if this is
+    // a subframe, delete it right away to prevent it from being used after its
+    // WebFrameWidget has been cleared.  See https://crbug.com/568836#15 for
+    // details.
+    OnDeleteFrame();

[Charlie Reis, 2016/04/15 23:20:05]

I'm assuming that using OnDeleteFrame() rather than frame->detach() is
intentional, so that we don't notify the browser (due to setting
in_browser_initiated_detach_ to true)?

If so, we should update the comments on all the appearances of
in_browser_initiated_detach_ (and possibly the name itself), since they would be
stale after this.

[nasko, 2016/04/18 21:24:26]

This method is called by the browser, so in a sense if we are detaching here, it
is browser initiated.

[alexmos, 2016/04/22 05:09:34]

Likewise, this concern is gone in the latest PS. :)

   }
 
   // Initialize the WebRemoteFrame with the replication state passed by the