Always swap with a replacement proxy in OnSwapOut.

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 <utility>
 #include <vector>
@@ skipping 1592 matching lines @@
                                     base::IntToString(routing_id));
       base::debug::SetCrashKeyValue("swapout_proxy_id",
                                     base::IntToString(proxy->routing_id()));
       base::debug::SetCrashKeyValue(
           "swapout_view_id", base::IntToString(render_view->GetRoutingID()));
       CHECK(!render_view->main_render_frame_);
     }
 
     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
   // process that is now rendering the frame.
   if (proxy)
     proxy->SetReplicatedState(replicated_frame_state);
 
   // Safe to exit if no one else is using the process.
   // TODO(nasko): Remove the dependency on RenderViewImpl here and ref count
   // the process based on the lifetime of this RenderFrameImpl object.
@@ skipping 4414 matching lines @@
                                     int match_count,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
   Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
                                    selection_rect, ordinal,
                                    final_status_update));
 }
 
 }  // namespace content