Fix RenderFrameHost lifetime and clean up CommitPending.

Fix RenderFrameHost lifetime and clean up CommitPending.

This updates the CommitPending logic now that swap out happens at commit
time, ensuring that the old RFH is kept alive until the SwapOut ACK is
received.

It is now also possible to swap out without creating a proxy.

BUG=407160
TEST=Cross-site iframe is visible in --site-per-process mode.

Committed: https://crrev.com/69d87d46eebbdc761974b3b7e3937406816e1cdb
Cr-Commit-Position: refs/heads/master@{#297902}

[Charlie Reis, 2014-09-09 01:43:26]

Patchset #2 (id:40001) has been deleted

[Charlie Reis, 2014-10-01 22:37:52]

Patchset #10 (id:220001) has been deleted

[Charlie Reis, 2014-10-01 22:38:30]

creis@chromium.org changed reviewers:
+ nasko@chromium.org

[Charlie Reis, 2014-10-01 22:43:14]

I think this is finally ready for review!  I've overhauled CommitPending and
SwapOutOldFrame.

I would have liked cleaner code around pending_delete_hosts_, but I haven't
found a good way to use a set of linked pointers.  Suggestions welcome.

It would also be nice to add some unit tests for this; I'll see how feasible
that is.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (left):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:86: STATE_PENDING_SHUTDOWN,
We didn't really need this state, since the few places that check can infer it
from the RFHM's pending deletion list.

[nasko, 2014-10-02 16:04:02]

This looks great! I'm surprised how much cleaner it is now and it is actually
readable : ).
Just a few comments/suggestions and it should be good to go.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:218: // WasSwappedOut is
called (when it is no longer visible).
nit: the "(when it is no longer visible)" part is stale, isn't it? Since we run
the unload handler asynchronously, we show the new frame at CommitPending, which
doesn't depend on WasSwappedOut anymore.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:531:
scoped_ptr<RenderFrameHostImpl> old_render_frame_host) {
I love that we are using scoped_ptrs more now. It makes it much easier to follow
ownership flow.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1439: // belongs to the
parent frame's SiteInstance.  (We do this after swapping out
nit: I'd put the sentence in () on a new line starting with Note:. It is
important enough to deserve a highlight, which Note: gives.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:450: // deleted or on the
pending delete list after this call.
nit: s/deleted or on/deleted or put on/?

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:543: RFHPendingDeleteList
pending_delete_hosts_;
Why not use ScopedPtrVector? It supports exactly the semantics we need
(hosts_.push_back(param.Pass())) and doesn't have the "complexities" of
linked_ptrs.

[Charlie Reis, 2014-10-02 19:41:20]

Thanks!  New patch up.  At your suggestion, I'll look into creating a
site_per_process_unittests.cc in a separate CL, to avoid blocking this further.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:218: // WasSwappedOut is
called (when it is no longer visible).
On 2014/10/02 16:04:02, nasko wrote:
> nit: the "(when it is no longer visible)" part is stale, isn't it? Since we
run
> the unload handler asynchronously, we show the new frame at CommitPending,
which
> doesn't depend on WasSwappedOut anymore.

Yep, good catch.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1439: // belongs to the
parent frame's SiteInstance.  (We do this after swapping out
On 2014/10/02 16:04:02, nasko wrote:
> nit: I'd put the sentence in () on a new line starting with Note:. It is
> important enough to deserve a highlight, which Note: gives.

Done.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:450: // deleted or on the
pending delete list after this call.
On 2014/10/02 16:04:02, nasko wrote:
> nit: s/deleted or on/deleted or put on/?

Done.  Also s/after/during/.

https://codereview.chromium.org/525583002/diff/240001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:543: RFHPendingDeleteList
pending_delete_hosts_;
On 2014/10/02 16:04:02, nasko wrote:
> Why not use ScopedPtrVector? It supports exactly the semantics we need
> (hosts_.push_back(param.Pass())) and doesn't have the "complexities" of
> linked_ptrs.

It's a good thought, but I don't think it helps in this case.  IsPendingDeletion
and DeleteFromPendingList will end up looking the same (since there's no find()
and erase() still takes an iterator).  Plus it's array based, so it will require
resizing on insert/delete (unlike lists) in order to support indexed access
(which we don't need).

I don't expect this list to get huge (at most 50, if you quickly go back through
all your session history items), but we may as well use lists until we can use
some kind of ownership pointer with sets.

[nasko, 2014-10-02 19:48:57]

LGTM

[Charlie Reis, 2014-10-02 19:50:24]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-10-02 19:50:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/525583002/280001

[commit-bot: I haz the power, 2014-10-02 21:11:07]

Committed patchset #12 (id:280001) as cc4e280a4b6e3daa796f85079d2823f27b7c6ab5

[commit-bot: I haz the power, 2014-10-02 21:11:44]

Patchset 12 (id:??) landed as
https://crrev.com/69d87d46eebbdc761974b3b7e3937406816e1cdb
Cr-Commit-Position: refs/heads/master@{#297902}