Always swap with a replacement proxy in OnSwapOut.

content/browser/frame_host/frame_tree.h

 // 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.
 
 #ifndef CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_H_
 #define CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_H_
 
 #include <stdint.h>
 
 #include <iterator>
@@ skipping 193 matching lines @@
   void ReplicatePageFocus(bool is_focused);
 
   // Updates page-level focus for this FrameTree in the subframe renderer
   // identified by |instance|.
   void SetPageFocus(SiteInstance* instance, bool is_focused);
 
  private:
   friend class FrameTreeTest;
   FRIEND_TEST_ALL_PREFIXES(RenderFrameHostImplBrowserTest, RemoveFocusedFrame);
   typedef base::hash_map<int, RenderViewHostImpl*> RenderViewHostMap;
-  typedef std::multimap<int, RenderViewHostImpl*> RenderViewHostMultiMap;
 
   // Returns a range to iterate over all FrameTreeNodes in the frame tree in
   // breadth-first traversal order, skipping the subtree rooted at
   // |node_to_skip|.
   NodeRange NodesExcept(FrameTreeNode* node_to_skip);
 
   // These delegates are installed into all the RenderViewHosts and
   // RenderFrameHosts that we create.
   RenderFrameHostDelegate* render_frame_delegate_;
   RenderViewHostDelegate* render_view_delegate_;
   RenderWidgetHostDelegate* render_widget_delegate_;
   RenderFrameHostManager::Delegate* manager_delegate_;
 
   // Map of SiteInstance ID to a RenderViewHost.  This allows us to look up the
   // RenderViewHost for a given SiteInstance when creating RenderFrameHosts.
   // Combined with the refcount on RenderViewHost, this allows us to call
   // Shutdown on the RenderViewHost and remove it from the map when no more
   // RenderFrameHosts are using it.
   //
   // Must be declared before |root_| so that it is deleted afterward.  Otherwise
   // the map will be cleared before we delete the RenderFrameHosts in the tree.
   RenderViewHostMap render_view_host_map_;
 
-  // Map of SiteInstance ID to RenderViewHosts that are pending shutdown. The
-  // renderers of these RVH are currently executing the unload event in
-  // background. When the SwapOutACK is received, they will be deleted. In the
-  // meantime, they are kept in this map, as they should not be reused (part of
-  // their state is already gone away).

[Charlie Reis, 2016/04/22 19:11:33]

Sanity check: Is there state that has gone away that we need to be concerned
about, or is this stale?

For example, what about main_frame_routing_id_?  That gets set in the RVH
constructor, and updated at commit time.  If we didn't reuse a pending delete
RVH and created a new one instead, it would have a main_frame_routing_id_ at
construction.  That impacts RenderViewImpl::Initialize.  Now we might have a
stale main_frame_routing_id_ until commit time, which could do something wrong
in RenderViewImpl::Initialize?  (Not sure, since your tests are green.)

Do we need to update this value sooner?  (I ask because it's related to many of
the other crashes we've seen in this area.)

[alexmos, 2016/04/25 18:17:08]

Good question, and the answer is complicated. :)  I actually think this might
not be specific to pending deletion RVHs -- if we have an A-embed-B and we
navigate to B, today that will reuse the old RVH (which was previously swapped
out) even without these changes.  (I wonder if that's ok and intentional?  Maybe
we shouldn't reuse a RV for a new main frame in such cases.)

For the above scenario, the common case is that we reuse the RVH that is live,
so we skip the call to create it, and thus we never get to RVI::Initialize on
this path.  Instead, we go through RFI::CreateFrame (sent for the main frame). 
That creates the RF and leaves the RV's main_render_frame_ as null until commit,
at which point RenderFrameImpl::didCommitProvisionalLoad updates it to the
committed frame.  This actually makes sense, since the RV's main frame is
technically a proxy until that commit happens, so wouldn't be right to update it
earlier.  Following the same reasoning, updating main_frame_routing_id_ on the
browser process side earlier (before commit) might not be right, since the RVH
is still technically swapped out?  The alternative to all this is to update
main_frame_routing_id_ in FrameTree::CreateRenderViewHost, in the case where
that decides to reuse an existing one, but that seems like it won't be
consistent with the renderer (I haven't tried this yet, so not sure what other
side effects that might have).

If the RVH is not live when we decide to reuse it, that seems like it would get
us into trouble here.  But it actually seems to also work: we get into
CreateRenderView with a main_frame_routing_id_ of NONE and proceed to create a
new RV which is swapped out, then we make a separate CreateRenderFrame call to
create the main frame (this is done because the main frame isn't live yet after
creating the view [1]) and then we update the view's main frame at commit time
as before.  This whole scenario seems fragile though.

I'm wondering if Nasko remembers anything about the "part of their state is
already gone away" comment.  It first appeared in clamy@'s unload handler CL,
https://codereview.chromium.org/88503002, which you both reviewed, and I'm not
sure what it referred to originally.  There are other things I'm curious about
that I don't know much about and that might become stale after RVH reuse:
enabled_bindings_, sudden_termination_allowed_, is_waiting_for_close_ack_. 
Since this kind of reuse is already possible today, I wonder if this is the
first time we've noticed this might be problem, or if we've already thought
about it?

Another scenario I'm trying to think through with similar concerns is A1 with a
long unload handler navigating to B, and then going to A2.  That might now reuse
A1's RV (with its RFH pending deletion), whereas it wouldn't reuse it before
this CL.  But it seems in this case the RV's main_render_frame_id_ will be
cleared once B commits, and then we'll go through the flow above when
initializing A2.  I've tried this manually and it seems to work.

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

[Charlie Reis, 2016/04/26 05:06:36]

Agreed-- updating main_frame_routing_id_ does seem like the right thing here.
Yeah, I don't think we've thought closely about it.  I'm inclined to say that we
should leave that for a separate CL/bug and proceed with this one, since there's
already a lot of benefit to this one and the issues we're considering are
already possible.
Sounds good.

-  RenderViewHostMultiMap render_view_host_pending_shutdown_map_;
-
   // This is an owned ptr to the root FrameTreeNode, which never changes over
   // the lifetime of the FrameTree. It is not a scoped_ptr because we need the
   // pointer to remain valid even while the FrameTreeNode is being destroyed,
   // since it's common for a node to test whether it's the root node.
   FrameTreeNode* root_;
 
   int focused_frame_tree_node_id_;
 
   base::Callback<void(RenderFrameHost*)> on_frame_removed_;
 
   // Overall load progress.
   double load_progress_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameTree);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FRAME_HOST_FRAME_TREE_H_