Only cancel navigation on user gesture

Only cancel pending navigation on user gesture.

This prevent sites navigating internally to block user from navigating.

BUG=75195
Committed: https://crrev.com/5bec87fd56934448fb237d1798690f2d0a3fc67c
Cr-Commit-Position: refs/heads/master@{#307668}

[wmaslowski, 2014-12-02 09:06:58]

wmaslowski@opera.com changed reviewers:
+ clamy@chromium.org, creis@chromium.org, nasko@chromium.org,
oysteine@chromium.org

[clamy, 2014-12-02 11:04:20]

Thanks. I am not sure though that we want to prevent the canceling of navigation
when the page navigated internally.

How about the following use case: the user typed something in the omnibox, and
the navigation takes time. She then clicks on a link on the current page, which
does an internal navigation that commits faster than the old one. Normally, we
would cancel the previous navigation, but with your CL we don't anymore. I don't
think this is a desirable behavior.

[Charlie Reis, 2014-12-02 18:03:33]

[+dcheng, japhet for context]

On 2014/12/02 11:04:20, clamy wrote:
> Thanks. I am not sure though that we want to prevent the canceling of
navigation
> when the page navigated internally.
> 
> How about the following use case: the user typed something in the omnibox, and
> the navigation takes time. She then clicks on a link on the current page,
which
> does an internal navigation that commits faster than the old one. Normally, we
> would cancel the previous navigation, but with your CL we don't anymore. I
don't
> think this is a desirable behavior.

+1 to concerns about this change.

It might be possible to track whether the navigation was caused by a user
gesture or not, and only keep pending navigations if a non-user-gesture
navigation commits.  This is definitely a non-trivial fix, though, likely
involving changes to both Blink's FrameLoader and the browser process.  It would
probably be best to discuss potential approaches on the bug if you plan to fix
it.

(It's also an issue that would need a test to prevent regression.)

https://codereview.chromium.org/746993003/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/746993003/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:521: render_frame_host,
input_params.was_within_same_page);
nit: params

https://codereview.chromium.org/746993003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/746993003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:440:
!was_within_same_page) {
This wouldn't be the right place to do this.  It only affects cross-process
navigations, so same-site (different page) navigations would still have the bug.

[wmaslowski, 2014-12-03 10:31:10]

I tried the approach with cancelling navigation only if it is caused by user
gesture and it seems to work.

I don't think non-cross-process navigations are problematic here as they aren't
affected by this bug anyway.

[clamy, 2014-12-03 12:30:10]

On 2014/12/03 10:31:10, wmaslowski wrote:
> I tried the approach with cancelling navigation only if it is caused by user
> gesture and it seems to work.
> 
> I don't think non-cross-process navigations are problematic here as they
aren't
> affected by this bug anyway.

If you're trying to navigate to a bookmarked page that is in the same site
instance (eg other page in the same domain), I think the navigation would be
prevented as well, and it would be a same-site navigation. Ending up with
non-user initiated in-page navigations canceling same-site navigations but not
cross-site ones seems like introducing too much of an hedge case.

[wmaslowski, 2014-12-03 12:55:09]

On 2014/12/03 12:30:10, clamy wrote:
> On 2014/12/03 10:31:10, wmaslowski wrote:
> > I tried the approach with cancelling navigation only if it is caused by user
> > gesture and it seems to work.
> > 
> > I don't think non-cross-process navigations are problematic here as they
> aren't
> > affected by this bug anyway.
> 
> If you're trying to navigate to a bookmarked page that is in the same site
> instance (eg other page in the same domain), I think the navigation would be
> prevented as well, and it would be a same-site navigation. Ending up with
> non-user initiated in-page navigations canceling same-site navigations but not
> cross-site ones seems like introducing too much of an hedge case.

Navigating to a bookmarked page in the same site instance works even without
this patch.

Try going to
http://grack.com/blog/2011/03/07/abusing-the-html5-history-api-for-fun-and-ch...
and add http://grack.com/ blog/ to bookmark bar and then click [Try it Out!]
button and then
try to navigate using bookmarks. if the bookmark is in the same domain it
navigates properly,
if in any other then it fails.

[clamy, 2014-12-03 13:19:42]

Ok. Your CL still needs the regression test though. We are doing quite a lot of
refactoring in navigations, so it'd be good to avoid the problem reappearing
when the new architecture lands :).

[wmaslowski, 2014-12-03 14:08:21]

On 2014/12/03 13:19:42, clamy wrote:
> Ok. Your CL still needs the regression test though. We are doing quite a lot
of
> refactoring in navigations, so it'd be good to avoid the problem reappearing
> when the new architecture lands :).

Thanks. I don't really have experience with integrating to chromium. How do I do
the request regression check. Does just clicking Try more bots do it?

[clamy, 2014-12-03 14:17:07]

No you need to write an additional test inside the CL. For this precise case, I
think you could add a unit test for RenderFrameHostManager (in
content/browser/frame_host/render_frame_host_manager_unittests.cc). It could do
the following:
1) Navigate to a page.
2) Start a cross-site navigation (but not commit it).
3) The current RFH receives an OnNavigate IPC for an in-page non user-initiated
navigation. Check that the pending navigation has not been canceled.
4) Commit the cross-site navigation.
5) Repeat 3 first steps above but with a user-initiated in-page navigation. The
pending navigation should have been canceled.

[Charlie Reis, 2014-12-03 21:32:19]

On 2014/12/03 12:55:09, wmaslowski wrote:
> On 2014/12/03 12:30:10, clamy wrote:
> > On 2014/12/03 10:31:10, wmaslowski wrote:
> > > I tried the approach with cancelling navigation only if it is caused by
user
> > > gesture and it seems to work.
> > > 
> > > I don't think non-cross-process navigations are problematic here as they
> > aren't
> > > affected by this bug anyway.
> > 
> > If you're trying to navigate to a bookmarked page that is in the same site
> > instance (eg other page in the same domain), I think the navigation would be
> > prevented as well, and it would be a same-site navigation. Ending up with
> > non-user initiated in-page navigations canceling same-site navigations but
not
> > cross-site ones seems like introducing too much of an hedge case.
> 
> Navigating to a bookmarked page in the same site instance works even without
> this patch.
> 
> Try going to
>
http://grack.com/blog/2011/03/07/abusing-the-html5-history-api-for-fun-and-ch...
> and add http://grack.com/ blog/ to bookmark bar and then click [Try it Out!]
> button and then
> try to navigate using bookmarks. if the bookmark is in the same domain it
> navigates properly,
> if in any other then it fails.

Ah, thanks for confirming that!  I agree that I see the same behavior.  Looks
like the hard work in Blink has already been done, so perhaps the RFHM change
will be sufficient.

A few comments below, and I like @clamy's proposal for how the test could work.

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:521: render_frame_host,
input_params.gesture == NavigationGestureUser);
Please use params (here and below), not input_params.

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

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:442: // one.
Let's update this comment: "Cancel the pending one if it was caused by a user
gesture.  If not, we should not prevent the user from navigating away."

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:447:
DCHECK(!was_caused_by_user_gesture);
This is confusing, since the DCHECK and comment are about which RFH is sending
the message.

Instead, we can put the was_caused_by_user_gesture check in a nested if inside
the block above.

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/746993003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:290: // Called when a
renderer's frame navigates.
Please add: "Navigations in the current RenderFrameHost will only cancel a
pending cross-process navigation if they were caused by a user gesture."

[Charlie Reis, 2014-12-04 00:19:56]

Please update the CL description as well, as it's no longer about "external
navigations."

[wmaslowski, 2014-12-04 11:28:46]

While looking at the tests I found out that
RenderFrameHostManagerTest.PageDoesBackAndReload test that Internal navigation
cancels pending navigation. It seems to be related to a security issue
https://code.google.com/p/chromium/issues/detail?id=51680 . It seems to be
something like <META HTTP-EQUIV="refresh" CONTENT="0;url=index.htm"> doing
internal navigation while navigation backwards happening caused te displayed in
the addressbar to change to previous page but the actual page remained the same,
which allowed spoofing the site. I couldnt reproduce the bug with or without the
patch and the tests seems to be meant to check that the state is sensible so
probably I can just change it to expect pending navigation not to be cancelled,
but I'd rather get your feedback first.

[Charlie Reis, 2014-12-04 20:00:32]

On 2014/12/04 11:28:46, wmaslowski wrote:
> While looking at the tests I found out that
> RenderFrameHostManagerTest.PageDoesBackAndReload test that Internal navigation
> cancels pending navigation. It seems to be related to a security issue
> https://code.google.com/p/chromium/issues/detail?id=51680 . It seems to be
> something like <META HTTP-EQUIV="refresh" CONTENT="0;url=index.htm"> doing
> internal navigation while navigation backwards happening caused te displayed
in
> the addressbar to change to previous page but the actual page remained the
same,
> which allowed spoofing the site. I couldnt reproduce the bug with or without
the
> patch and the tests seems to be meant to check that the state is sensible so
> probably I can just change it to expect pending navigation not to be
cancelled,
> but I'd rather get your feedback first.

Ah, I'm glad we had a test for that!  Changing a security test requires some
careful thought, so I took a closer look.

I've added a more recent defense against URL spoofs that happens in a different
place, so that's probably why you're not seeing the spoof after your change.  As
it turns out, the pending entry still exists, but
NavigationController::GetVisibleEntry() doesn't return it for history
navigations.  Otherwise, an evil page could send you back/forward to a slow page
and then change its own content to look like that page.

Feel free to update that test to expect the pending entry to still exist, but
verify that GetVisibleEntry() returns the last committed entry (url2, not url1).
 I may have some additional changes to suggest there once you have the patch up.

Thanks!

[Charlie Reis, 2014-12-05 19:37:43]

Thanks for updating the tests to cover this behavior.  Are you able to run try
jobs for it?

The CL description still mentions "external navigation;" can you remove that
reference?

A few minor comments below, and it should be ready to go.

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:521: render_frame_host,
input_params.gesture == NavigationGestureUser);
This is the third time I've asked you to use params rather than input_params. 
We use the non-const copy for everything here because we modify some parts of it
along the way, like the transition.  Let's not mix references, which makes bugs
more likely.

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1067: // then
reload. http://crbug.com/51680
Please add:
Also tests that only user-gesture navigations can interrupt cross-process
navigations.  http://crbug.com/75195

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1103: // That
should NOT have cancelled the pending RFH.
Please add: ", because the reload did not have a user gesture.  Thus, the
pending back navigation will still eventually commit."

https://codereview.chromium.org/746993003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1105:
pending_render_view_host() != NULL);
Style nit: 4 space indent on an unfinished line.

git cl format can help catch these things.

[Charlie Reis, 2014-12-08 18:02:35]

Thanks, LGTM.  I sent some try jobs to see if the bots are happy with it.

(For the future, it helps reviewers if you reply "Done" to code review comments
as you complete them.  Not such a big deal here, but it's useful on larger CLs
with more comments.)

[wmaslowski, 2014-12-09 09:06:09]

On 2014/12/08 18:02:35, Charlie Reis wrote:
> Thanks, LGTM.  I sent some try jobs to see if the bots are happy with it.
> 
> (For the future, it helps reviewers if you reply "Done" to code review
comments
> as you complete them.  Not such a big deal here, but it's useful on larger CLs
> with more comments.)

Thanks - I'll try to do it in the future

[clamy, 2014-12-09 10:38:06]

Thanks! Lgtm

[Charlie Reis, 2014-12-09 17:40:55]

@wmaslowski: Are you waiting for anything else, or is this ready to commit?  Let
us know if you need us to check the commit box for you; I'm not sure if it's
available to you or not.

[wmaslowski, 2014-12-10 08:59:12]

The CQ bit was checked by wmaslowski@opera.com

[commit-bot: I haz the power, 2014-12-10 09:00:00]

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

[commit-bot: I haz the power, 2014-12-10 10:07:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-10 10:08:29]

Patchset 4 (id:??) landed as
https://crrev.com/5bec87fd56934448fb237d1798690f2d0a3fc67c
Cr-Commit-Position: refs/heads/master@{#307668}