Fix remote-to-local navigations in crashed subframes.

Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by sending a new IPC message,
FrameMsg_SwapIn, to the provisional frame when doing the early
CommitPending call in step (2).  This ensures that the renderer also
swaps the provisional frame into the tree, replacing its associated
proxy.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2619123002
Cr-Commit-Position: refs/heads/master@{#443378}
Committed: https://chromium.googlesource.com/chromium/src/+/f65a795a9bdbb809993025c4f37260ccd38302ef

[alexmos, 2017-01-07 02:22:27]

Description was changed from

==========
Fix remote-to-local navigations in crashed subframes.

BUG=487872
==========

to

==========
Fix remote-to-local navigations in crashed subframes.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2017-01-07 02:26:49]

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

[commit-bot: I haz the power, 2017-01-07 02:26:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:33:17]

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

[commit-bot: I haz the power, 2017-01-07 02:33:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexmos, 2017-01-09 18:29:53]

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

[commit-bot: I haz the power, 2017-01-09 18:30:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-01-09 18:37:58]

Description was changed from

==========
Fix remote-to-local navigations in crashed subframes.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by skipping the early CommitPending call
in step (2) for subframes.  Main frames don't suffer from this bug,
because FrameMsg_DeleteProxy is sent only for subframes in
~RenderFrameProxyHost.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2017-01-09 19:44:57]

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

[alexmos, 2017-01-09 19:44:58]

Hey Charlie, can you please take a look?  See my analysis for this in
https://crbug.com/487872#c10.

It's unfortunate to skip the early CommitPending optimization for sad subframes,
but I haven't yet found a way that would allow keeping it.  In practice it means
that we'll get to stare at a sad subframe for a big longer (navigation commit
vs. navigation start), which doesn't seem that bad to me.  Maybe you have some
other ideas?

Since we create the RenderFrame for this while the proxy is still around, we
must keep the proxy alive until commit, as otherwise we won't be able to swap
the RenderFrame into the tree in didCommitProvisionalLoad when the navigation
commits.  So clearing the proxy_routing_id_ from the RenderFrame after we delete
the proxy as part of early CommitPending doesn't work.  Creating the provisional
RenderFrame without the proxy at the very start also seems wrong (we don't know
whether it will commit or not).  Basically, doing the equivalent of the early
CommitPending on the renderer side seems really hard, and if we keep the early
CommitPending, we're bound to have an inconsistent state for the proxy between
the browser and renderer until the real navigation commit.

Finding a condition to prevent the FrameMsg_DeleteProxy from being sent in ~RFPH
(or the whole RFPH being deleted) as part of the early CommitPending seems bad
as well, as it will create a period of time where the browser has both a current
RFH and a proxy in the same SiteInstance, which violates one of our core
invariants.

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

https://codereview.chromium.org/2619123002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:225:
frame_tree_node_->IsMainFrame()) {
If we skip the early CommitPending for subframes, we'll next hit this path,
which dates back to a bug you fixed in 2008. :)  I'm actually not sure this is
still needed -- seems like the condition on the early CommitPending in
UpdateStateForNavigate would prevent us from ever getting here, and I don't see
why we'd want to recreate the old RenderView like this.  If you agree, I can try
ripping this out in a separate cleanup
(https://codereview.chromium.org/2622673002).  Regardless, calling this for
subframes with a proxy routing ID of NONE is wrong and would hit a DCHECK in
CreateRenderView that either the main frame or proxy routing ID must not be
NONE.

[commit-bot: I haz the power, 2017-01-09 20:33:28]

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

[commit-bot: I haz the power, 2017-01-09 20:33:28]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-10 00:18:09]

On 2017/01/09 19:44:58, alexmos wrote:
> Hey Charlie, can you please take a look?  See my analysis for this in
> https://crbug.com/487872#c10.
> 
> It's unfortunate to skip the early CommitPending optimization for sad
subframes,
> but I haven't yet found a way that would allow keeping it.  In practice it
means
> that we'll get to stare at a sad subframe for a big longer (navigation commit
> vs. navigation start), which doesn't seem that bad to me.  Maybe you have some
> other ideas?

I'm not sure I have an answer yet, but I'm worried about skipping the early
commit for subframes.  For one thing, this bug could later affect main frames as
well, since ~RenderFrameProxyHost has a TODO to remove the !IsMainFrame check. 
It's also less than ideal to leave the sad frame there during navigation, even
for subframes.

Some thoughts below.

> Since we create the RenderFrame for this while the proxy is still around, we
> must keep the proxy alive until commit, as otherwise we won't be able to swap
> the RenderFrame into the tree in didCommitProvisionalLoad when the navigation
> commits.  So clearing the proxy_routing_id_ from the RenderFrame after we
delete
> the proxy as part of early CommitPending doesn't work.

I feel like this idea is on the right track, though.  The early commit in the
browser process really needs to cause an early swap in the renderer process. 
Could we have a way to tell the renderer to swap the RenderFrameProxy and
provisional RenderFrame before didCommitProvisionalLoad, thus clearing
proxy_routing_id_?  Then the navigation won't need to swap when it gets to
didCommitProvisionalLoad.

(Not sure how this plays out in practice, but I'm imagining something like an
alternative to the DeleteProxy IPC that triggers a swap with a provisional
frame.)

>  Creating the provisional
> RenderFrame without the proxy at the very start also seems wrong (we don't
know
> whether it will commit or not).  Basically, doing the equivalent of the early
> CommitPending on the renderer side seems really hard, and if we keep the early
> CommitPending, we're bound to have an inconsistent state for the proxy between
> the browser and renderer until the real navigation commit.
> 
> Finding a condition to prevent the FrameMsg_DeleteProxy from being sent in
~RFPH
> (or the whole RFPH being deleted) as part of the early CommitPending seems bad
> as well, as it will create a period of time where the browser has both a
current
> RFH and a proxy in the same SiteInstance, which violates one of our core
> invariants.
> 

Conversely, I'm wondering if we should be looking for an alternate fix to
https://crbug.com/526304 and https://crbug.com/568676 (in OnNavigate and
didCommitProvisionalLoad).  Those bugs were about having an in-progress
navigation in a provisional RenderFrame at the time the RenderFrameProxy gets
detached from the tree, and dealing with the fact that the provisional
RenderFrame is left floating around after the proxy goes away.

Straw man: maybe the DeleteProxy IPC could optionally contain a routing ID of a
provisional RenderFrame, and specify whether that should be deleted as well (vs
swapping it in in the case above)?  Or the proxy should keep the provisional
RenderFrame's routing ID as state and either delete it or swap it in at
destruction time, depending on which case we're in?  That might let us remove
the early returns from OnNavigate and didCommitProvisionalLoad, which seem like
they could continue to cause bugs like this.

Anyway, happy to chat more in person tomorrow.

[alexmos, 2017-01-11 00:06:20]

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

[commit-bot: I haz the power, 2017-01-11 00:06:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-01-11 00:22:51]

Description was changed from

==========
Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by skipping the early CommitPending call
in step (2) for subframes.  Main frames don't suffer from this bug,
because FrameMsg_DeleteProxy is sent only for subframes in
~RenderFrameProxyHost.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by sending a new IPC message,
FrameMsg_SwapIn, to the provisional frame when doing the early
CommitPending call in step (2).  This ensures that the renderer also
swaps the provisional frame into the tree, replacing its associated
proxy.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2017-01-11 00:38:18]

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

[commit-bot: I haz the power, 2017-01-11 00:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 02:08:02]

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

[commit-bot: I haz the power, 2017-01-11 02:08:02]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[alexmos, 2017-01-11 02:19:33]

On 2017/01/10 00:18:09, Charlie Reis wrote:
> On 2017/01/09 19:44:58, alexmos wrote:
> > Hey Charlie, can you please take a look?  See my analysis for this in
> > https://crbug.com/487872#c10.
> > 
> > It's unfortunate to skip the early CommitPending optimization for sad
> subframes,
> > but I haven't yet found a way that would allow keeping it.  In practice it
> means
> > that we'll get to stare at a sad subframe for a big longer (navigation
commit
> > vs. navigation start), which doesn't seem that bad to me.  Maybe you have
some
> > other ideas?
> 
> I'm not sure I have an answer yet, but I'm worried about skipping the early
> commit for subframes.  For one thing, this bug could later affect main frames
as
> well, since ~RenderFrameProxyHost has a TODO to remove the !IsMainFrame check.

> It's also less than ideal to leave the sad frame there during navigation, even
> for subframes.
> 
> Some thoughts below.
> 
> > Since we create the RenderFrame for this while the proxy is still around, we
> > must keep the proxy alive until commit, as otherwise we won't be able to
swap
> > the RenderFrame into the tree in didCommitProvisionalLoad when the
navigation
> > commits.  So clearing the proxy_routing_id_ from the RenderFrame after we
> delete
> > the proxy as part of early CommitPending doesn't work.
> 
> I feel like this idea is on the right track, though.  The early commit in the
> browser process really needs to cause an early swap in the renderer process. 
> Could we have a way to tell the renderer to swap the RenderFrameProxy and
> provisional RenderFrame before didCommitProvisionalLoad, thus clearing
> proxy_routing_id_?  Then the navigation won't need to swap when it gets to
> didCommitProvisionalLoad.
> 
> (Not sure how this plays out in practice, but I'm imagining something like an
> alternative to the DeleteProxy IPC that triggers a swap with a provisional
> frame.)

Thanks!  I gave that a try in the latest PS.  WDYT?

I needed to factor out the swap part into a new helper, but I actually like how
that simplified didCommitProvisionalLoad.  I thought a separate IPC might be
less confusing than overloading FrameMsg_DeleteProxy (which would also have
required to make the helper public in RenderFrameImpl), and since we already
track the proxy in the provisional RenderFrame, I could just send the message
directly to that, without passing any additional routing IDs.

> 
> >  Creating the provisional
> > RenderFrame without the proxy at the very start also seems wrong (we don't
> know
> > whether it will commit or not).  Basically, doing the equivalent of the
early
> > CommitPending on the renderer side seems really hard, and if we keep the
early
> > CommitPending, we're bound to have an inconsistent state for the proxy
between
> > the browser and renderer until the real navigation commit.
> > 
> > Finding a condition to prevent the FrameMsg_DeleteProxy from being sent in
> ~RFPH
> > (or the whole RFPH being deleted) as part of the early CommitPending seems
bad
> > as well, as it will create a period of time where the browser has both a
> current
> > RFH and a proxy in the same SiteInstance, which violates one of our core
> > invariants.
> > 
> 
> Conversely, I'm wondering if we should be looking for an alternate fix to
> https://crbug.com/526304 and https://crbug.com/568676 (in OnNavigate and
> didCommitProvisionalLoad).  Those bugs were about having an in-progress
> navigation in a provisional RenderFrame at the time the RenderFrameProxy gets
> detached from the tree, and dealing with the fact that the provisional
> RenderFrame is left floating around after the proxy goes away.
> 
> Straw man: maybe the DeleteProxy IPC could optionally contain a routing ID of
a
> provisional RenderFrame, and specify whether that should be deleted as well
(vs
> swapping it in in the case above)?  Or the proxy should keep the provisional
> RenderFrame's routing ID as state and either delete it or swap it in at
> destruction time, depending on which case we're in?  That might let us remove
> the early returns from OnNavigate and didCommitProvisionalLoad, which seem
like
> they could continue to cause bugs like this.
> 
> Anyway, happy to chat more in person tomorrow.

I do like deleting any provisional frame associated with the proxy immediately
when the latter gets detached.  The second option might work, though I'm a bit
wary of keeping additional state in the proxy, since that might get stale if we
aren't careful (given our history of similar problems).  I can give that a try
in a separate CL if you're ok using a new IPC for the early swap case.  The
first option feels like it won't help in some of those races: from what I
remember, some of them detached the remote frame directly in JS (without the
DeleteProxy IPC) in the same process where we were at various stages of creating
and navigating the provisional local frame, so adding a frame routing ID to
FrameMsg_DeleteProxy won't help there.  And if the detach is initiated from a
different process, we might as well clean up the provisional frame using
FrameMsg_Delete (which we should be doing already, hopefully?), though I guess
we could still have a race between the two IPCs arriving.

[Charlie Reis, 2017-01-12 00:29:05]

On 2017/01/11 02:19:33, alexmos wrote:
> On 2017/01/10 00:18:09, Charlie Reis wrote:
> > On 2017/01/09 19:44:58, alexmos wrote:
> > > Hey Charlie, can you please take a look?  See my analysis for this in
> > > https://crbug.com/487872#c10.
> > > 
> > > It's unfortunate to skip the early CommitPending optimization for sad
> > subframes,
> > > but I haven't yet found a way that would allow keeping it.  In practice it
> > means
> > > that we'll get to stare at a sad subframe for a big longer (navigation
> commit
> > > vs. navigation start), which doesn't seem that bad to me.  Maybe you have
> some
> > > other ideas?
> > 
> > I'm not sure I have an answer yet, but I'm worried about skipping the early
> > commit for subframes.  For one thing, this bug could later affect main
frames
> as
> > well, since ~RenderFrameProxyHost has a TODO to remove the !IsMainFrame
check.
> 
> > It's also less than ideal to leave the sad frame there during navigation,
even
> > for subframes.
> > 
> > Some thoughts below.
> > 
> > > Since we create the RenderFrame for this while the proxy is still around,
we
> > > must keep the proxy alive until commit, as otherwise we won't be able to
> swap
> > > the RenderFrame into the tree in didCommitProvisionalLoad when the
> navigation
> > > commits.  So clearing the proxy_routing_id_ from the RenderFrame after we
> > delete
> > > the proxy as part of early CommitPending doesn't work.
> > 
> > I feel like this idea is on the right track, though.  The early commit in
the
> > browser process really needs to cause an early swap in the renderer process.

> > Could we have a way to tell the renderer to swap the RenderFrameProxy and
> > provisional RenderFrame before didCommitProvisionalLoad, thus clearing
> > proxy_routing_id_?  Then the navigation won't need to swap when it gets to
> > didCommitProvisionalLoad.
> > 
> > (Not sure how this plays out in practice, but I'm imagining something like
an
> > alternative to the DeleteProxy IPC that triggers a swap with a provisional
> > frame.)
> 
> Thanks!  I gave that a try in the latest PS.  WDYT?
> 
> I needed to factor out the swap part into a new helper, but I actually like
how
> that simplified didCommitProvisionalLoad.  I thought a separate IPC might be
> less confusing than overloading FrameMsg_DeleteProxy (which would also have
> required to make the helper public in RenderFrameImpl), and since we already
> track the proxy in the provisional RenderFrame, I could just send the message
> directly to that, without passing any additional routing IDs.

I like this.  I agree that it's nice to abstract out the SwapIn code from
didCommit.  I suppose there's a bit of a race where we might start showing the
blank provisional RenderFrame in the browser process before the renderer has
handled the SwapIn request, but I don't think that should be a problem.  It'll
be blank either way, and the SwapIn will definitely occur before the navigation
IPC arrives.

LGTM.


> 
> > 
> > >  Creating the provisional
> > > RenderFrame without the proxy at the very start also seems wrong (we don't
> > know
> > > whether it will commit or not).  Basically, doing the equivalent of the
> early
> > > CommitPending on the renderer side seems really hard, and if we keep the
> early
> > > CommitPending, we're bound to have an inconsistent state for the proxy
> between
> > > the browser and renderer until the real navigation commit.
> > > 
> > > Finding a condition to prevent the FrameMsg_DeleteProxy from being sent in
> > ~RFPH
> > > (or the whole RFPH being deleted) as part of the early CommitPending seems
> bad
> > > as well, as it will create a period of time where the browser has both a
> > current
> > > RFH and a proxy in the same SiteInstance, which violates one of our core
> > > invariants.
> > > 
> > 
> > Conversely, I'm wondering if we should be looking for an alternate fix to
> > https://crbug.com/526304 and https://crbug.com/568676 (in OnNavigate and
> > didCommitProvisionalLoad).  Those bugs were about having an in-progress
> > navigation in a provisional RenderFrame at the time the RenderFrameProxy
gets
> > detached from the tree, and dealing with the fact that the provisional
> > RenderFrame is left floating around after the proxy goes away.
> > 
> > Straw man: maybe the DeleteProxy IPC could optionally contain a routing ID
of
> a
> > provisional RenderFrame, and specify whether that should be deleted as well
> (vs
> > swapping it in in the case above)?  Or the proxy should keep the provisional
> > RenderFrame's routing ID as state and either delete it or swap it in at
> > destruction time, depending on which case we're in?  That might let us
remove
> > the early returns from OnNavigate and didCommitProvisionalLoad, which seem
> like
> > they could continue to cause bugs like this.
> > 
> > Anyway, happy to chat more in person tomorrow.
> 
> I do like deleting any provisional frame associated with the proxy immediately
> when the latter gets detached.  The second option might work, though I'm a bit
> wary of keeping additional state in the proxy, since that might get stale if
we
> aren't careful (given our history of similar problems).  I can give that a try
> in a separate CL if you're ok using a new IPC for the early swap case.  The
> first option feels like it won't help in some of those races: from what I
> remember, some of them detached the remote frame directly in JS (without the
> DeleteProxy IPC) in the same process where we were at various stages of
creating
> and navigating the provisional local frame, so adding a frame routing ID to
> FrameMsg_DeleteProxy won't help there.  And if the detach is initiated from a
> different process, we might as well clean up the provisional frame using
> FrameMsg_Delete (which we should be doing already, hopefully?), though I guess
> we could still have a race between the two IPCs arriving.

Agreed.  Let's try it out in a followup CL, since it would be nice to clean that
case up (from OnNavigate and SwapIn) by proactively deleting the frame.

https://codereview.chromium.org/2619123002/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2619123002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:666: // Requests that a provisional RenderFrame
swaps itself into the frame tree,
nit: s/swaps/swap/

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5004: // to clean this frame up after
https://crbug.com/548275 is fixed.
Note that this bug is marked fixed.  Does that mean we already send an IPC to
clean it up?  (I do think we should try out the idea of proactively deleting the
provisional frame when a proxy is detached, in a separate CL.)

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:796: // tree.
Might want to mention what happens if the proxy isn't found, since it could be
detached right before this IPC arrives.

[alexmos, 2017-01-12 01:34:25]

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

[commit-bot: I haz the power, 2017-01-12 01:35:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-01-12 01:38:11]

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

[alexmos, 2017-01-12 01:38:12]

Thanks, Charlie!  I'll experiment with proactively deleting the provisional
frame when a proxy is detached in a separate CL.

Nasko, can you please review frame_messages.h?

https://codereview.chromium.org/2619123002/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2619123002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:666: // Requests that a provisional RenderFrame
swaps itself into the frame tree,
On 2017/01/12 00:29:05, Charlie Reis wrote:
> nit: s/swaps/swap/

Done.

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5004: // to clean this frame up after
https://crbug.com/548275 is fixed.
On 2017/01/12 00:29:05, Charlie Reis wrote:
> Note that this bug is marked fixed.  Does that mean we already send an IPC to
> clean it up?  (I do think we should try out the idea of proactively deleting
the
> provisional frame when a proxy is detached, in a separate CL.)

I think we do send the IPC for subframes to clean up dead provisional frames
(this is FrameMsg_Delete sent in ~RenderFrameHostImpl, added by Nasko in
https://codereview.chromium.org/1409693009).  But we could still end up in a
race where we detach the proxy and then get to didCommit right before receiving
FrameMsg_Delete, so we still need to abort the navigation in that case (and the
FrameMsg_Delete will then destroy it).  Destroying the provisional frame right
away when its proxy goes away might allow us to remove this early return, as you
say, but for now I've removed this TODO.

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2619123002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.h:796: // tree.
On 2017/01/12 00:29:05, Charlie Reis wrote:
> Might want to mention what happens if the proxy isn't found, since it could be
> detached right before this IPC arrives.

Done.

[commit-bot: I haz the power, 2017-01-12 01:46:49]

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

[commit-bot: I haz the power, 2017-01-12 01:46:51]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[alexmos, 2017-01-12 17:50:08]

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

[commit-bot: I haz the power, 2017-01-12 17:50:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 18:35:57]

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

[commit-bot: I haz the power, 2017-01-12 18:35:58]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-01-12 19:14:46]

IPC LGTM

[alexmos, 2017-01-12 21:56:25]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2017-01-12 21:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2619123002/#ps100001
(title: "Charlie's comments")

[commit-bot: I haz the power, 2017-01-12 21:57:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 22:04:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484258185085890,
"parent_rev": "e4203db424afef97fd382185990f0308383803d6", "commit_rev":
"f65a795a9bdbb809993025c4f37260ccd38302ef"}

[commit-bot: I haz the power, 2017-01-12 22:04:41]

Description was changed from

==========
Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by sending a new IPC message,
FrameMsg_SwapIn, to the provisional frame when doing the early
CommitPending call in step (2).  This ensures that the renderer also
swaps the provisional frame into the tree, replacing its associated
proxy.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix remote-to-local navigations in crashed subframes.

Previously, these navigations were aborted due to this unfortunate
sequence of events:

1. A new provisional RenderFrame is created with a proxy_routing_id_
of the proxy that it would replace once it commits.

2. RFHM::UpdateStateForNavigate calls CommitPending early, because the
current RFH was not live (crashed).  This destroys the
RenderFrameProxy to which the RenderFrame's proxy_routing_id_ in step
(1) referred.

3. Next, when navigating the frame, RenderFrameImpl::OnNavigate
checks whether the proxy it's supposed to replace still exists, and
aborts the navigation because it doesn't.

This CL avoids this problem by sending a new IPC message,
FrameMsg_SwapIn, to the provisional frame when doing the early
CommitPending call in step (2).  This ensures that the renderer also
swaps the provisional frame into the tree, replacing its associated
proxy.

BUG=487872
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2619123002
Cr-Commit-Position: refs/heads/master@{#443378}
Committed:
https://chromium.googlesource.com/chromium/src/+/f65a795a9bdbb809993025c4f372...
==========

[commit-bot: I haz the power, 2017-01-12 22:04:43]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f65a795a9bdbb809993025c4f372...