PlzNavigate: don't stop all loaders when renderer-initiated nav fails

PlzNavigate: don't stop all loaders when renderer-initiated nav fails

When a renderer-initiated navigation is aborted by the browser process
in PlzNavigate, we currently send a FrameMsg_Stop IPC to the renderer
process, in order for the renderer process to know that there is no
longer a pending navigation being processed by the browser. This is
wrong, as it also cancels any ongoing load in the current page. Instead,
this CL introduces a new IPC that will only cancel the dummy provisional
DocumentLoader that was created when the navigation was sent to the
browser.

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

[clamy, 2017-03-22 14:54:48]

Description was changed from

==========
PlzNavigate: don't stop all loaders when renderer-initiated nav fails

When a renderer-initiated navigation is aborted by the browser process
in PlzNavigate, we currently send a FrameMsg_Stop IPC to the renderer
process, in order for the renderer process to know that there is no
longer a pending navigation being processed by the browser. This is
wrong, as it also cancels any ongoing load in the current page. Instead,
this CL introduces a new IPC that will only cancel the dummy provisional
DocumentLoader that was created when the navigation was sent to the
browser.

BUG=576261
==========

to

==========
PlzNavigate: don't stop all loaders when renderer-initiated nav fails

When a renderer-initiated navigation is aborted by the browser process
in PlzNavigate, we currently send a FrameMsg_Stop IPC to the renderer
process, in order for the renderer process to know that there is no
longer a pending navigation being processed by the browser. This is
wrong, as it also cancels any ongoing load in the current page. Instead,
this CL introduces a new IPC that will only cancel the dummy provisional
DocumentLoader that was created when the navigation was sent to the
browser.

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

[clamy, 2017-03-22 14:54:58]

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

[commit-bot: I haz the power, 2017-03-22 14:55:23]

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

[clamy, 2017-03-22 14:56:44]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, japhet@chromium.org, nasko@chromium.org

[clamy, 2017-03-22 14:56:44]

@nasko, creis, japhet: PTAL.

This is a spin-off from https://codereview.chromium.org/2735183003/.

Regardless of whether we do end up with an id or not, I think we still need to
go on with changing the IPC to something that will only cancel the provisional
DocumentLoader.

[commit-bot: I haz the power, 2017-03-22 16:27:10]

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

[commit-bot: I haz the power, 2017-03-22 16:27:11]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-22 18:44:46]

blink LGTM

[dcheng, 2017-03-22 19:00:49]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-03-22 19:00:50]

https://codereview.chromium.org/2768813002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2768813002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2168: if (frame())
Nit: is it possible to DCHECK() this?

There are only two times this would be null:
- The frame is not completely initialized, but PlzNavigate should not be active
for that frame then.
- The frame is detached, but then IPCs shouldn't be routed to the RenderFrame.

[Charlie Reis, 2017-03-23 00:09:07]

I'm starting to concede that you might be right about the need for an ID, though
I think we'll want to more clearly document what this is and/or move it into
something like NavigationStateImpl (if that's safe, which I'm not sure about
yet).  (The goal there would be avoiding a bunch of navigation state building up
on RenderFrameImpl.)

Before we dive into that, I thought I'd check if you can land part of this
without the ID, given that we presumably already have the race problem with the
FrameMsg_Stop IPC.  Moving from Stop to DroppedNavigation would fix the "don't
stop ongoing load in current page" problem, and we can separately fix the race
with a longer discussion about the ID.  Does that make sense, or do we need both
in this CL?

[clamy, 2017-03-23 16:00:45]

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

[commit-bot: I haz the power, 2017-03-23 16:01:23]

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

[clamy, 2017-03-23 16:02:51]

On 2017/03/23 00:09:07, Charlie Reis (OOO soon) wrote:
> I'm starting to concede that you might be right about the need for an ID,
though
> I think we'll want to more clearly document what this is and/or move it into
> something like NavigationStateImpl (if that's safe, which I'm not sure about
> yet).  (The goal there would be avoiding a bunch of navigation state building
up
> on RenderFrameImpl.)
> 
> Before we dive into that, I thought I'd check if you can land part of this
> without the ID, given that we presumably already have the race problem with
the
> FrameMsg_Stop IPC.  Moving from Stop to DroppedNavigation would fix the "don't
> stop ongoing load in current page" problem, and we can separately fix the race
> with a longer discussion about the ID.  Does that make sense, or do we need
both
> in this CL?

I've uploaded a patch set that adds the IPC without the id, which should already
be an improvement. For the ID part, I really had in mind something more akin to
a token the renderer sends along with the navigation request, and that the
browser can send back if it cancels it so that we can compare whether it matches
the current token. Maybe calling it token would make it clearer what it is meant
to be used for?

[commit-bot: I haz the power, 2017-03-23 16:20:23]

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

[commit-bot: I haz the power, 2017-03-23 16:20:24]

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

[Charlie Reis, 2017-03-23 16:52:53]

Thanks!  Looking better.

On 2017/03/23 16:02:51, clamy (ooo until Apr 3) wrote:
> On 2017/03/23 00:09:07, Charlie Reis (OOO soon) wrote:
> > I'm starting to concede that you might be right about the need for an ID,
> though
> > I think we'll want to more clearly document what this is and/or move it into
> > something like NavigationStateImpl (if that's safe, which I'm not sure about
> > yet).  (The goal there would be avoiding a bunch of navigation state
building
> up
> > on RenderFrameImpl.)
> > 
> > Before we dive into that, I thought I'd check if you can land part of this
> > without the ID, given that we presumably already have the race problem with
> the
> > FrameMsg_Stop IPC.  Moving from Stop to DroppedNavigation would fix the
"don't
> > stop ongoing load in current page" problem, and we can separately fix the
race
> > with a longer discussion about the ID.  Does that make sense, or do we need
> both
> > in this CL?
> 
> I've uploaded a patch set that adds the IPC without the id, which should
already
> be an improvement. For the ID part, I really had in mind something more akin
to
> a token the renderer sends along with the navigation request, and that the
> browser can send back if it cancels it so that we can compare whether it
matches
> the current token. Maybe calling it token would make it clearer what it is
meant
> to be used for?

Yes, that's a good idea.  Something that doesn't suggest ordering or affiliation
with session history, but that just identifies whether we're talking about the
same navigation.  I'd also like to include comments discouraging people using it
more broadly or exposing it to other code, when we get to that CL.

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:408: inform_renderer &&
!navigation_request_->may_transfer();
Hmm, I'm not sure I understand this change.  Why was !browser_initiated() wrong?
 It was more clearly about the case that the renderer would have some state that
needs clearing.  Using may_transfer() instead is subtle and indirect-- without
looking into the code, I don't know why it's equivalent.  (That also means it's
at risk of changing for unrelated reasons, which could break this check.)

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

https://codereview.chromium.org/2768813002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:919: // navigation.
Please clarify how this differs from FrameMsg_Stop (e.g., doesn't stop ongoing
loads in the current page).

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

https://codereview.chromium.org/2768813002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:5316: void
RenderFrameImpl::OnDroppedNavigation() {
Maybe we should put this alongside OnStop() to make it clear that they're
related?

Looking at that one, I'm also wondering if we should notify observers in this
case.  Do you know if the two current listeners to RFO::OnStop would want to
hear this as well?

(Also, feel free to include a TODO to address the race.  I'm glad we're landing
this first, but we should acknowledge that it's worth coming back to.)

[nasko, 2017-04-19 18:12:18]

Is there next step for this CL? Or is it going to be closed?

[clamy, 2017-04-24 13:53:44]

I thought I had answered comments but hadn't. I'll try to work on another
iteration of the patch later this week as well.

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:408: inform_renderer &&
!navigation_request_->may_transfer();
On 2017/03/23 16:52:53, Charlie Reis wrote:
> Hmm, I'm not sure I understand this change.  Why was !browser_initiated()
wrong?
>  It was more clearly about the case that the renderer would have some state
that
> needs clearing.  Using may_transfer() instead is subtle and indirect-- without
> looking into the code, I don't know why it's equivalent.  (That also means
it's
> at risk of changing for unrelated reasons, which could break this check.)

We should only inform the renderer if the navigation was started through the
BeginNavigation path because that;s the only case where it is expecting the
browser process to be doing a navigation (which corresponds to
!navigation_request_->may_transfer()). browser_initiated may be false for
navigations started via RequestNavigation, if they ultimately come from OpenURL.

[Charlie Reis, 2017-04-24 20:13:38]

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:408: inform_renderer &&
!navigation_request_->may_transfer();
On 2017/04/24 13:53:44, clamy wrote:
> On 2017/03/23 16:52:53, Charlie Reis wrote:
> > Hmm, I'm not sure I understand this change.  Why was !browser_initiated()
> wrong?
> >  It was more clearly about the case that the renderer would have some state
> that
> > needs clearing.  Using may_transfer() instead is subtle and indirect--
without
> > looking into the code, I don't know why it's equivalent.  (That also means
> it's
> > at risk of changing for unrelated reasons, which could break this check.)
> 
> We should only inform the renderer if the navigation was started through the
> BeginNavigation path because that;s the only case where it is expecting the
> browser process to be doing a navigation (which corresponds to
> !navigation_request_->may_transfer()). browser_initiated may be false for
> navigations started via RequestNavigation, if they ultimately come from
OpenURL.

Ok, so the OpenURL case means the renderer doesn't need to be notified, and thus
!browser_initiated() isn't the right thing to use.  (I guess the renderer
doesn't go into a loading state in that case?)

But how is "may_transfer" connected to BeginNavigation?  A renderer-initiated
navigation via BeginNavigation seems like it could still transfer (e.g., for a
redirect to the Chrome Web Store).  Maybe I'm just having trouble with the
name-- the comment on NavigationRequest::may_transfer_ is similarly confusing to
me, since it says that a transfer may happen even when may_transfer_ is false. 
Maybe we can rename that to be clearer, to help this case make sense as well.

Is may_transfer() actually about whether we send the navigation through
RenderFrameHostManager to do a process swap for cross-site navigations (and thus
preserve the current process model in PlzNavigate)?

If so, there's two ways we might try to address this:

1) Make the name (and comment) closer to that case, with something like
should_swap_if_cross_site().  That reduces some of the ambiguity around the word
"transfer," since we do transfers already in non-PlzNavigate on
renderer-initiated navigations (like the CWS case).  However, even this name is
a bit odd in the current context, since "whether the renderer has navigation
state" and "whether we should swap processes for cross-site navigations" are two
different concepts that just happen to be the same at the moment.

2) Make this member of NavigationRequest explicitly be about whether it started
with a BeginNavigation (e.g., from_begin_navigation()?).  Then we can build
policies on top of that state, including whether to do a process swap for
cross-site navigations and whether the renderer needs to be notified when a
navigation is reset.

I'm leaning towards the latter.  What do you think?

[clamy, 2017-04-25 12:24:09]

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2768813002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:408: inform_renderer &&
!navigation_request_->may_transfer();
On 2017/04/24 20:13:38, Charlie Reis wrote:
> On 2017/04/24 13:53:44, clamy wrote:
> > On 2017/03/23 16:52:53, Charlie Reis wrote:
> > > Hmm, I'm not sure I understand this change.  Why was !browser_initiated()
> > wrong?
> > >  It was more clearly about the case that the renderer would have some
state
> > that
> > > needs clearing.  Using may_transfer() instead is subtle and indirect--
> without
> > > looking into the code, I don't know why it's equivalent.  (That also means
> > it's
> > > at risk of changing for unrelated reasons, which could break this check.)
> > 
> > We should only inform the renderer if the navigation was started through the
> > BeginNavigation path because that;s the only case where it is expecting the
> > browser process to be doing a navigation (which corresponds to
> > !navigation_request_->may_transfer()). browser_initiated may be false for
> > navigations started via RequestNavigation, if they ultimately come from
> OpenURL.
> 
> Ok, so the OpenURL case means the renderer doesn't need to be notified, and
thus
> !browser_initiated() isn't the right thing to use.  (I guess the renderer
> doesn't go into a loading state in that case?)
> 
> But how is "may_transfer" connected to BeginNavigation?  A renderer-initiated
> navigation via BeginNavigation seems like it could still transfer (e.g., for a
> redirect to the Chrome Web Store).  Maybe I'm just having trouble with the
> name-- the comment on NavigationRequest::may_transfer_ is similarly confusing
to
> me, since it says that a transfer may happen even when may_transfer_ is false.

> Maybe we can rename that to be clearer, to help this case make sense as well.
> 
> Is may_transfer() actually about whether we send the navigation through
> RenderFrameHostManager to do a process swap for cross-site navigations (and
thus
> preserve the current process model in PlzNavigate)?
> 
> If so, there's two ways we might try to address this:
> 
> 1) Make the name (and comment) closer to that case, with something like
> should_swap_if_cross_site().  That reduces some of the ambiguity around the
word
> "transfer," since we do transfers already in non-PlzNavigate on
> renderer-initiated navigations (like the CWS case).  However, even this name
is
> a bit odd in the current context, since "whether the renderer has navigation
> state" and "whether we should swap processes for cross-site navigations" are
two
> different concepts that just happen to be the same at the moment.
> 
> 2) Make this member of NavigationRequest explicitly be about whether it
started
> with a BeginNavigation (e.g., from_begin_navigation()?).  Then we can build
> policies on top of that state, including whether to do a process swap for
> cross-site navigations and whether the renderer needs to be notified when a
> navigation is reset.
> 
> I'm leaning towards the latter.  What do you think?

I agree that the naming is confusing. I think we named in that way because it
was only used in one particular place for which the naming made more sense (and
I wanted to keep it short).

Since we're using it in other contexts than RenderFrameHostmanager, I like
option 2 better as well.