Fix detection of RenderViewHosts pending deletion in CreateRenderViewHost.

Fix detection of RenderViewHosts pending deletion in CreateRenderViewHost.

Previously, if FrameTree::CreateRenderViewHost found an existing RVH,
it checked whether its main frame is pending deletion to decide that
the RVH shouldn't be reused.  This doesn't work when
IsSwappedOutForbidden is true, because the RVH's main frame might have
been cleared by RFHM::CommitPending.  Consequently, on A->B->A
navigations, if B->A happened before the swapout ACK was received for
A->B, we ended up reusing RVH(A) for the new navigation, and crashed
trying to create RF(A) because creating main frames with an existing
RenderView and without a proxy to replace isn't supported by the
renderer.  In such situations, RVH(A) shouldn't be reused, and this
code introduces a flag on RVH to ensure that this happens even after
the main frame has been cleared.

BUG=515302
Committed: https://crrev.com/b97d6bb6f374d80d432d74ce5c31465fa21209f7
Cr-Commit-Position: refs/heads/master@{#350489}

[alexmos, 2015-09-18 18:32:16]

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

[alexmos, 2015-09-18 18:32:17]

Nasko, can you please take a look?  I went with your suggestion on tracking the
RVH pending deletion state with a bool flag on RVH.  Hopefully it can go away
after swapped-out is gone.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1345353003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:284:
render_view_host_pending_shutdown_map_.insert(
Note that as we discussed, FrameTree::ReleaseRenderViewHostRef will still ensure
that the RVH's refcount drops to 0 before it is deleted.  IIUC, placing the RVH
on this map simply ensures that it won't be reused here.

[nasko, 2015-09-18 19:06:17]

Thanks for taking this on! I think the code itself looks good! Some comments on
the test.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1345353003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:284:
render_view_host_pending_shutdown_map_.insert(
On 2015/09/18 18:32:17, alexmos wrote:
> Note that as we discussed, FrameTree::ReleaseRenderViewHostRef will still
ensure
> that the RVH's refcount drops to 0 before it is deleted.  IIUC, placing the
RVH
> on this map simply ensures that it won't be reused here.

Acknowledged.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3403: RenderFrameHostImpl* rfhi
= root->current_frame_host();
nit: Either rfh or rvhi, for consistency. IMHO, the i isn't really adding much.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3418:
EXPECT_TRUE(root->render_manager()->IsPendingDeletion(rfhi));
nit: I would put those expectations right after the NavigateToURL above, as they
are valid right after that and are an outcome of that navigation, regardless of
whether we navigate next or not.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3420: NavigateToURL(shell(),
a_url);
nit: I wonder if we should make this navigation async, such that we don't wait
for a commit. Sending the swapped out ack after this commit is ... interesting :
). I don't feel strongly though, so your call.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3427: rfhi->OnSwappedOut();
For going the extra mile, should we put a RenderFrameDeleted observer to ensure
it gets deleted?
Also, I wonder if there is any difference of calling the method directly vs
making up a FrameHostMsg_SwapOutACK and calling OnMessageReceived.

[alexmos, 2015-09-18 22:37:05]

Please take another look.  PS3 addresses your comments, and PS4 fixes the UAFs
due to SwapOut timer that we discussed in chat.  

I can't reproduce any timeouts locally when doing sync navigations instead of
async (and killing the swapout timer in that case), so I still don't know how
exactly what fixed the two timeouts from PS2, but the PS3 bots are all green.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3403: RenderFrameHostImpl* rfhi
= root->current_frame_host();
On 2015/09/18 19:06:17, nasko (slow to review) wrote:
> nit: Either rfh or rvhi, for consistency. IMHO, the i isn't really adding
much.

Done.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3418:
EXPECT_TRUE(root->render_manager()->IsPendingDeletion(rfhi));
On 2015/09/18 19:06:17, nasko (slow to review) wrote:
> nit: I would put those expectations right after the NavigateToURL above, as
they
> are valid right after that and are an outcome of that navigation, regardless
of
> whether we navigate next or not.

Done.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3420: NavigateToURL(shell(),
a_url);
On 2015/09/18 19:06:17, nasko (slow to review) wrote:
> nit: I wonder if we should make this navigation async, such that we don't wait
> for a commit. Sending the swapped out ack after this commit is ... interesting
:
> ). I don't feel strongly though, so your call.

Done.

https://codereview.chromium.org/1345353003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3427: rfhi->OnSwappedOut();
On 2015/09/18 19:06:17, nasko (slow to review) wrote:
> For going the extra mile, should we put a RenderFrameDeleted observer to
ensure
> it gets deleted?

Done.

> Also, I wonder if there is any difference of calling the method directly vs
> making up a FrameHostMsg_SwapOutACK and calling OnMessageReceived.

Seems like they would do the same thing, as OnMessageReceived would call
OnSwapOutACK(), which only calls OnSwappedOut.  I tried to use OnMessageReceived
but that caused WebContentsObserverSanityChecker to complain.

[alexmos, 2015-09-18 22:42:46]

On 2015/09/18 22:37:05, alexmos wrote:
> Please take another look.  PS3 addresses your comments, and PS4 fixes the UAFs
> due to SwapOut timer that we discussed in chat.  
> 
> I can't reproduce any timeouts locally when doing sync navigations instead of
> async (and killing the swapout timer in that case), so I still don't know how
> exactly what fixed the two timeouts from PS2, but the PS3 bots are all green.

Err, sorry, my patchset numbers are all screwed up and off by 2.

[nasko, 2015-09-18 23:29:14]

LGTM with one question.

https://codereview.chromium.org/1345353003/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3430: shell()->LoadURL(a_url);
This navigation doesn't wait for anything. Is this ok? Also, its implementation
is almost equivalent to the code on 3417-3419. Why not use it there too?

[alexmos, 2015-09-21 16:25:24]

https://codereview.chromium.org/1345353003/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3430: shell()->LoadURL(a_url);
On 2015/09/18 23:29:14, nasko (slow to review) wrote:
> This navigation doesn't wait for anything. Is this ok? 

The interesting part happens when creating the pending RFH and figuring out
which RVH to use and that's covered by the RVH check below.  But since the
original bug manifested as a renderer crash on this navigation, I do think it's
a good idea to make sure it succeeds, so I added the wait as the last step.

Incidentally, after a fresh look, I found a problem with the RVH check: I was
comparing the current RVH (for B) vs. the pending one.  That's fixed now, and
re-verified that it fails without my fix -- please take a quick look.

> Also, its implementation is almost equivalent to the code on 3417-3419. Why
not use it there too?

Good point!  Done.

[nasko, 2015-09-21 17:07:20]

LGTM with a nit.

https://codereview.chromium.org/1345353003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3428: TestNavigationObserver
observer(shell()->web_contents());
nit: Let's add to the name of this variable what it is observing. You could put
both navigations in nested scopes if you want to reuse the variable name. Or
just call this one navigation_observer or something similar.

[alexmos, 2015-09-21 17:11:14]

Thanks!

https://codereview.chromium.org/1345353003/diff/120001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1345353003/diff/120001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:3428: TestNavigationObserver
observer(shell()->web_contents());
On 2015/09/21 17:07:20, nasko (slow to review) wrote:
> nit: Let's add to the name of this variable what it is observing. You could
put
> both navigations in nested scopes if you want to reuse the variable name. Or
> just call this one navigation_observer or something similar.

Done.

[alexmos, 2015-09-21 17:11:55]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-09-21 17:11:55]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1345353003/#ps140001
(title: "Fix Nasko's nit")

[commit-bot: I haz the power, 2015-09-21 17:12:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1345353003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1345353003/140001

[commit-bot: I haz the power, 2015-09-21 18:49:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-21 18:49:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2015-09-23 21:11:05]

Nasko, can you please take a look at the Mac timeout workaround in PS9?

[nasko, 2015-09-23 21:13:05]

LGTM

[alexmos, 2015-09-24 05:28:54]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-09-24 05:29:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1345353003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1345353003/160001

[commit-bot: I haz the power, 2015-09-24 07:31:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-09-24 07:33:52]

Patchset 9 (id:??) landed as
https://crrev.com/b97d6bb6f374d80d432d74ce5c31465fa21209f7
Cr-Commit-Position: refs/heads/master@{#350489}