Fix 3 crashes related to navigations after a process dies.

Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e73d58efe1a779172c2ca31b97f8aa6256491d5a
Cr-Commit-Position: refs/heads/master@{#383846}

[Charlie Reis, 2016-03-28 04:00:09]

Description was changed from

==========
Fix crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
==========

to

==========
Fix crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-28 19:05:07]

Description was changed from

==========
Fix crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-28 19:05:07]

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

[Charlie Reis, 2016-03-28 19:06:37]

Alex, can you take a look?  This should handle all 3 of the crashes.  I needed
to tweak the original test for RVH's pending deletion (from
https://codereview.chromium.org/1345353003/) as well.

Some clarification comments below.

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:358: if
(root_->render_manager()->IsViewPendingDeletion(iter->second)) {
Computing this on the fly rather than keeping a bit fixes both CreateRenderView
(581912) and RenderViewInit (544755).

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:380:
!root_->render_manager()->IsViewPendingDeletion(iter->second)) {
This fixes 591478.  (It would have been fixed by is_pending_deletion, too, but
we might as well use the new version.)

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1335: OnSwappedOut();
This is a separate fix for CreateRenderView (581912).  Either fix is sufficient
for that bug, but it makes sense to do both.  This doesn't help with the other
bugs.

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4103:
DISABLED_RenderViewHostPendingDeletionIsNotReused
There's a chance I fixed the problem in https://crbug.com/554825.  I'll try
re-enabling it after this CL lands.

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4119: SiteInstanceImpl*
site_instance = rfh->GetSiteInstance();
Have to grab these in advance to avoid a UaF below.

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4124: // TODO(creis): This will
be flaky, since there's still a 1 second timeout.
Oops, I forgot to take this TODO out.  I fixed this on line 4136.

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4145: // navigation. This can be
removed once https://crbug.com/535246 is fixed.
I'd like to remove this as well, but I think this CL tackles enough bugs
already.  :)

Given my fix to clear the pending delete RFHs when the process dies, it means
that rfh is gone after line 4149 and we can't interact with it below.  We can
clean that up as part of the 535246 bug fix.

[Charlie Reis, 2016-03-29 04:19:01]

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4124: // TODO(creis): This will
be flaky, since there's still a 1 second timeout.
On 2016/03/28 19:06:37, Charlie Reis wrote:
> Oops, I forgot to take this TODO out.  I fixed this on line 4136.

Done.

[alexmos, 2016-03-29 18:43:02]

Awesome, I'm so relieved that all of these crashes are going to be fixed!  Just
a couple of minor comments below.

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:358: if
(root_->render_manager()->IsViewPendingDeletion(iter->second)) {
On 2016/03/28 19:06:36, Charlie Reis wrote:
> Computing this on the fly rather than keeping a bit fixes both
CreateRenderView
> (581912) and RenderViewInit (544755).

Acknowledged.

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:380:
!root_->render_manager()->IsViewPendingDeletion(iter->second)) {
On 2016/03/28 19:06:36, Charlie Reis wrote:
> This fixes 591478.  (It would have been fixed by is_pending_deletion, too, but
> we might as well use the new version.)

Acknowledged.

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1335: OnSwappedOut();
On 2016/03/28 19:06:37, Charlie Reis wrote:
> This is a separate fix for CreateRenderView (581912).  Either fix is
sufficient
> for that bug, but it makes sense to do both.  This doesn't help with the other
> bugs.

Acknowledged.

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

https://codereview.chromium.org/1835833002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4103:
DISABLED_RenderViewHostPendingDeletionIsNotReused
On 2016/03/28 19:06:37, Charlie Reis wrote:
> There's a chance I fixed the problem in https://crbug.com/554825.  I'll try
> re-enabling it after this CL lands.

Agreed that you may have fixed it -- and sounds good to try re-enabling
afterward.  Thanks!

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

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2424: if (rvh &&
rvh->IsRenderViewLive() && !IsViewPendingDeletion(rvh) &&
Why is IsViewPendingDeletion necessary here?  it seems rvh comes from
GetRenderViewHost, which already checks for this.

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2426: //
Ensure that we don't create in RenderViewImpl::Init if a proxy is created
nit: word missing after "don't create"?

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2453:
ASSERT_TRUE(root->render_manager()->IsPendingDeletion(rfh_a));
Is it worth checking that the view is pending deletion as well at this point? 
(Similarly, in the test above, it might be worth checking that it's not pending
deletion due to the proxy.)

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2465: if
(SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
Is this the right thing to use? (As opposed to AreAllSitesIsolatedForTesting)

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

https://codereview.chromium.org/1835833002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4188:
EXPECT_TRUE(NavigateToURL(shell(), second_url));
There's a chance that the script will finish before we get to create the
subframe and we won't be testing what we want to test, but I guess this is ok --
it won't cause the test to fail and navigations such as this should take way
less than 1 second on the bots, right?

Another thought is we could compare the new and old RVH routing IDs and make
sure they are different, similarly to the test above.  Probably ok either way,
given that we already have similar coverage above.

[Charlie Reis, 2016-03-29 20:27:51]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 20:28:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1835833002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1835833002/100001

[Charlie Reis, 2016-03-29 20:29:03]

Thanks!  PTAL.

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

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2424: if (rvh &&
rvh->IsRenderViewLive() && !IsViewPendingDeletion(rvh) &&
On 2016/03/29 18:43:01, alexmos wrote:
> Why is IsViewPendingDeletion necessary here?  it seems rvh comes from
> GetRenderViewHost, which already checks for this.

Ha!  Good point.  I put it in to make the test fail (without the fixes), since
it was odd that we weren't accounting for pending delete RVHs here.  But now
that we have the fix for GetRenderViewHost() as well, this is redundant with
that.

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2426: //
Ensure that we don't create in RenderViewImpl::Init if a proxy is created
On 2016/03/29 18:43:01, alexmos wrote:
> nit: word missing after "don't create"? 

Heh, s/create/crash/.  Thanks for catching it.

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2453:
ASSERT_TRUE(root->render_manager()->IsPendingDeletion(rfh_a));
On 2016/03/29 18:43:02, alexmos wrote:
> Is it worth checking that the view is pending deletion as well at this point? 
> (Similarly, in the test above, it might be worth checking that it's not
pending
> deletion due to the proxy.)

Good idea.  Done.  (I made them EXPECT rather than ASSERT, since the RFH version
is the precondition for the test working properly, while the RVH version is what
the test is verifying.)

https://codereview.chromium.org/1835833002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:2465: if
(SiteIsolationPolicy::AreCrossProcessFramesPossible()) {
On 2016/03/29 18:43:02, alexmos wrote:
> Is this the right thing to use? (As opposed to AreAllSitesIsolatedForTesting)

Actually, I don't remember why I needed to guard this.  It's a main frame, so we
create the proxy even in default mode.  I've removed the conditional.

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

https://codereview.chromium.org/1835833002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:4188:
EXPECT_TRUE(NavigateToURL(shell(), second_url));
On 2016/03/29 18:43:02, alexmos wrote:
> There's a chance that the script will finish before we get to create the
> subframe and we won't be testing what we want to test, but I guess this is ok
--
> it won't cause the test to fail and navigations such as this should take way
> less than 1 second on the bots, right?

Yeah, there's a chance it could be a flaky failure if a regression happened, but
it should be pretty likely to fail in that case.  We can't use while(1) since
then the subframe will never commit (given that it ends up in the same process,
but a different RVH).  I think I'm ok with it as is.

> Another thought is we could compare the new and old RVH routing IDs and make
> sure they are different, similarly to the test above.  Probably ok either way,
> given that we already have similar coverage above.

Good idea.  Done.

[alexmos, 2016-03-29 20:35:57]

Great, LGTM!

[Charlie Reis, 2016-03-29 20:54:20]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-03-29 20:54:28]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-03-29 20:55:07]

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

[commit-bot: I haz the power, 2016-03-29 22:12:23]

Description was changed from

==========
Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-29 22:12:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-29 22:14:56]

Description was changed from

==========
Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix 3 crashes related to navigations after a process dies.

Three separate crashes were possible when a RenderViewHost is pending
deletion.  We now compute this state on demand rather than storing it
in a boolean, since it can change between the time of swapout and the
next navigation.  We were also failing to check this state in
FrameTree::GetRenderViewHost.

This also clears the pending delete RenderFrameHost if its process
dies, since we won't hear a swap out ACK from it after that.

BUG=581912, 544755, 591478
TEST=See bugs for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e73d58efe1a779172c2ca31b97f8aa6256491d5a
Cr-Commit-Position: refs/heads/master@{#383846}
==========

[commit-bot: I haz the power, 2016-03-29 22:14:57]

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