Add a way to cancel deferred navigations in NavigationHandle

Add a way to cancel deferred navigations in NavigationHandle

This CL introduces a way to cancel deferred navigations in NavigationHandle. It
also adds a CANCEL value for the NavigationThrottles that indicates that the
navigation should be cancelled without the issuer making it like it was never
issued.

BUG=537634
Committed: https://crrev.com/e8853384ca0c7ed581bc38c2e4b2a23475d81eb9
Cr-Commit-Position: refs/heads/master@{#360325}

[clamy, 2015-11-04 15:39:26]

Description was changed from

==========
Add a way to cancel deferred navigations in NavigationHandle

This CL introduces a way to cancel deferred navigations in NavigationHandle. It
also adds a CANCEL value for the NavigationThrottles that indicates that the
navigation should be cancelled without teh issuer making it like it was never
issued.

BUG=537634
==========

to

==========
Add a way to cancel deferred navigations in NavigationHandle

This CL introduces a way to cancel deferred navigations in NavigationHandle. It
also adds a CANCEL value for the NavigationThrottles that indicates that the
navigation should be cancelled without teh issuer making it like it was never
issued.

BUG=537634
==========

[clamy, 2015-11-04 15:39:27]

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

[clamy, 2015-11-04 15:41:53]

@nasko: PTAL. This is part of a series of patch to convert the
DataReductionproxyResourceThrottle in a NavigationThrottle. Also, I've included
a simple way to fix the use-after-free issue in the InterceptNavigationThrottle
by doing the callback check asynchronously. If it agrees with you, I'll expand
the InterceptNavigationThrottleTests to cover it.

[nasko, 2015-11-04 22:44:27]

Mostly nits.

https://codereview.chromium.org/1414723008/diff/1/components/navigation_inter...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/1/components/navigation_inter...
components/navigation_interception/intercept_navigation_throttle.cc:87:
RunCallback(
If the callback can destruct this object, then it doesn't seem correct to still
run it through this object. Shouldn't the PostTask on line 74 bind it to the
helper function? This way there is no object being destroyed that we return back
into.

The way the code is structured right now, the only guard is for
InterceptNavigationThrottle being deleted between the posting of the task and
the execution. But if the callback can delete the object, we aren't guarding
this.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:85: // destroyed in the
middle of the NavigationThrottles checks.
nit: s/was destroyed/is being destroyed/, unless I'm misunderstanding the
context that this code will run only in the case of deleting WebContents in the
observer.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:174: void
NavigationHandleImpl::CancelDeferredNavigation(bool ignore) {
Why not use the enum as the parameter type? In general, boolean arguments are
somewhat hard to read at the time of the method call. This will be a bit more
verbose in characters, but will be clear which intention is meant when reading
the code.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:175: CHECK(state_ ==
DEFERRING_START || state_ == DEFERRING_REDIRECT);
DCHECK?

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:366:
complete_callback_.Reset();
nit: Empty line between the if statement and this call.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_unittest.cc:210:
EXPECT_TRUE(IsDeferringStart());
If the navigation is cancelled, should the throttle be in a DeferringStart
state?

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_unittest.cc:234: EXPECT_EQ(0,
test_throttle->will_start_calls());
It shouldn't be possible to have 0 calls to WillStart and 1 call to WillRedirect
in real code, should it? It will be better to match the semantics of real world,
so maybe in a separate patch change it such that simluating WillRedirect ensures
that WillSend has been called?

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:293: // TODO(clamy):
distinguish between CANCEL and CANCEL_AND_IGNORE.
What would be the practical difference?

https://codereview.chromium.org/1414723008/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1414723008/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:97: // If |ignore| is true, the
issuer will act as if the request was never
Issuer of the cancellation or of the navigation?

[clamy, 2015-11-05 15:52:11]

Patchset #2 (id:20001) has been deleted

[clamy, 2015-11-05 15:52:24]

Thanks!

https://codereview.chromium.org/1414723008/diff/1/components/navigation_inter...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/1/components/navigation_inter...
components/navigation_interception/intercept_navigation_throttle.cc:87:
RunCallback(
On 2015/11/04 22:44:26, nasko (slow to review) wrote:
> If the callback can destruct this object, then it doesn't seem correct to
still
> run it through this object. Shouldn't the PostTask on line 74 bind it to the
> helper function? This way there is no object being destroyed that we return
back
> into.
> 
> The way the code is structured right now, the only guard is for
> InterceptNavigationThrottle being deleted between the posting of the task and
> the execution. But if the callback can delete the object, we aren't guarding
> this.

The callback needs the WebContents to run, which may go away, hence why I can't
PostTask the execution of the callback (unless I pull out the routing id +
process id for the RVH and get the WebContents back using those).

This is why I made sure we would not execute anything in the object after
running the callback. I thought that having the handling of the result in a
second callback bound on a weak_ptr to this object would ensure the second
callback did not run when this was deleted.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:85: // destroyed in the
middle of the NavigationThrottles checks.
On 2015/11/04 22:44:26, nasko (slow to review) wrote:
> nit: s/was destroyed/is being destroyed/, unless I'm misunderstanding the
> context that this code will run only in the case of deleting WebContents in
the
> observer.

Done.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:174: void
NavigationHandleImpl::CancelDeferredNavigation(bool ignore) {
On 2015/11/04 22:44:26, nasko (slow to review) wrote:
> Why not use the enum as the parameter type? In general, boolean arguments are
> somewhat hard to read at the time of the method call. This will be a bit more
> verbose in characters, but will be clear which intention is meant when reading
> the code.

Done.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:175: CHECK(state_ ==
DEFERRING_START || state_ == DEFERRING_REDIRECT);
On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:366:
complete_callback_.Reset();
On 2015/11/04 22:44:26, nasko (slow to review) wrote:
> nit: Empty line between the if statement and this call.

Done.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_unittest.cc:210:
EXPECT_TRUE(IsDeferringStart());
On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> If the navigation is cancelled, should the throttle be in a DeferringStart
> state?

I have introduced a CANCELING state to deal with the amount of time between the
moment the throttle wants to cancel the navigation and the moment we get the
DidStopLoading IPC in the current architecture (in PlzNavigate it's immediate).

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl_unittest.cc:234: EXPECT_EQ(0,
test_throttle->will_start_calls());
On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> It shouldn't be possible to have 0 calls to WillStart and 1 call to
WillRedirect
> in real code, should it? It will be better to match the semantics of real
world,
> so maybe in a separate patch change it such that simluating WillRedirect
ensures
> that WillSend has been called?

Added a TODO for this (this require a few modifications to the
TestNavigationThrottle to work, so I'll do that in another patch.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:293: // TODO(clamy):
distinguish between CANCEL and CANCEL_AND_IGNORE.
On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> What would be the practical difference?

It's different at the ResourceController level. Apparently CANCEL_AND_IGNORE
results in a flag was_ignored_by_handler being set to true, and this is passed
around through blink and back to the browser, where it is a parameter to
WCO::DidFailProvisionalLoad. From code search, it does not seem that this
parameter is used anywhere though. Maybe we'll be able to remove it? (unless it
is used somewhere on Android).

https://codereview.chromium.org/1414723008/diff/1/content/public/browser/navi...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1414723008/diff/1/content/public/browser/navi...
content/public/browser/navigation_handle.h:97: // If |ignore| is true, the
issuer will act as if the request was never
On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> Issuer of the cancellation or of the navigation? 

Changed this comment following the change of arguments.

[nasko, 2015-11-06 17:31:59]

LGTM with a few nits and comments to be added.

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1414723008/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:293: // TODO(clamy):
distinguish between CANCEL and CANCEL_AND_IGNORE.
On 2015/11/05 15:52:23, clamy wrote:
> On 2015/11/04 22:44:27, nasko (slow to review) wrote:
> > What would be the practical difference?
> 
> It's different at the ResourceController level. Apparently CANCEL_AND_IGNORE
> results in a flag was_ignored_by_handler being set to true, and this is passed
> around through blink and back to the browser, where it is a parameter to
> WCO::DidFailProvisionalLoad. From code search, it does not seem that this
> parameter is used anywhere though. Maybe we'll be able to remove it? (unless
it
> is used somewhere on Android).

If it isn't used, removing it will be great! I honestly have no idea what this
"ignored by handler" implies, so it is a good candidate for deleting if
possible.

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:74:
BrowserThread::PostTask(
Let's also add a comment here why there is a PostTask - mainly to ensure there
are no objects on the stack that can be deleted and unwinding through them in
deleted state.

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:92: }
As discussed, let's put a very scary warning here that no code should be added
after the RunCallback, as the object can be deleted and it will result in a
use-after-free bug.

https://codereview.chromium.org/1414723008/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1414723008/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:173: void
RunCompleteCallback(NavigationThrottle::ThrottleCheckResult result);
A small blurb about what this method is meant to do will be useful.

https://codereview.chromium.org/1414723008/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1414723008/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:96: // Cancels a navigation that was
previously paused by a NavigationThrottle.
nit: s/paused/deferred/, so we are consistent in our terminology.

[clamy, 2015-11-09 13:54:29]

clamy@chromium.org changed reviewers:
+ mnaganov@chromium.org

[clamy, 2015-11-09 13:56:27]

@nasko: thanks! Updated the comments in the latest version
@mnaganov: PTAL at the changes in navigation_interception/. This fixes properly
a use-after-free bug with the InterceptNavigationThrottle on Android (calling
the callback results in the WebContents being destroyed synchronously, along
with the NavigationHandle and the InterceptNavigationThrottle).

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:74:
BrowserThread::PostTask(
On 2015/11/06 17:31:59, nasko (slow to review) wrote:
> Let's also add a comment here why there is a PostTask - mainly to ensure there
> are no objects on the stack that can be deleted and unwinding through them in
> deleted state.

Done.

https://codereview.chromium.org/1414723008/diff/40001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:92: }
On 2015/11/06 17:31:59, nasko (slow to review) wrote:
> As discussed, let's put a very scary warning here that no code should be added
> after the RunCallback, as the object can be deleted and it will result in a
> use-after-free bug.

Done.

https://codereview.chromium.org/1414723008/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1414723008/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:173: void
RunCompleteCallback(NavigationThrottle::ThrottleCheckResult result);
On 2015/11/06 17:31:59, nasko (slow to review) wrote:
> A small blurb about what this method is meant to do will be useful.

Done.

https://codereview.chromium.org/1414723008/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1414723008/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:96: // Cancels a navigation that was
previously paused by a NavigationThrottle.
On 2015/11/06 17:31:59, nasko (slow to review) wrote:
> nit: s/paused/deferred/, so we are consistent in our terminology.

Done.

[clamy, 2015-11-09 14:54:25]

Description was changed from

==========
Add a way to cancel deferred navigations in NavigationHandle

This CL introduces a way to cancel deferred navigations in NavigationHandle. It
also adds a CANCEL value for the NavigationThrottles that indicates that the
navigation should be cancelled without teh issuer making it like it was never
issued.

BUG=537634
==========

to

==========
Add a way to cancel deferred navigations in NavigationHandle

This CL introduces a way to cancel deferred navigations in NavigationHandle. It
also adds a CANCEL value for the NavigationThrottles that indicates that the
navigation should be cancelled without the issuer making it like it was never
issued.

BUG=537634
==========

[nasko, 2015-11-09 17:31:18]

Just one nit on a comment. The rest are awesome! Thanks for adding them.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:45: // WebContents, it should
return CANCEL, CANCEL_AND_IGNORE or DEFER and
Should we encourage the usage of DEFER if they will be deleting WebContents?

Also, let's clarify that it is the WebContents this throttle is associated with,
not just any WebContents.

[mnaganov (inactive), 2015-11-09 17:42:42]

Nice job!

LGTM with nits.

https://codereview.chromium.org/1414723008/diff/60001/components/navigation_i...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/60001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:114: if
(should_ignore_navigation)
nit: Curly brackets are required as the statement spans multiple lines, the
"else" path will also need brackets then. See
https://google.github.io/styleguide/cppguide.html#Conditionals

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:43: // alive during teh duration of
this method. Failing to do so will result in
typo: the

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:46: // perform the destrutction
asynchronously.
typo: destruction

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:52: // alive during teh duration of
this method. Failing to do so will result in
typo: the

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:55: // perform the destrutction
asynchronously.
typo: destruction

[clamy, 2015-11-10 11:58:31]

clamy@chromium.org changed reviewers:
+ asargent@chromium.org

[clamy, 2015-11-10 11:58:53]

Thanks!

@asargent: PTAL at the changes in chrome/browser/apps

https://codereview.chromium.org/1414723008/diff/60001/components/navigation_i...
File components/navigation_interception/intercept_navigation_throttle.cc
(right):

https://codereview.chromium.org/1414723008/diff/60001/components/navigation_i...
components/navigation_interception/intercept_navigation_throttle.cc:114: if
(should_ignore_navigation)
On 2015/11/09 17:42:41, mnaganov wrote:
> nit: Curly brackets are required as the statement spans multiple lines, the
> "else" path will also need brackets then. See
> https://google.github.io/styleguide/cppguide.html#Conditionals

Done.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:43: // alive during teh duration of
this method. Failing to do so will result in
On 2015/11/09 17:42:41, mnaganov wrote:
> typo: the

Done.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:45: // WebContents, it should
return CANCEL, CANCEL_AND_IGNORE or DEFER and
On 2015/11/09 17:31:18, nasko (slow to review) wrote:
> Should we encourage the usage of DEFER if they will be deleting WebContents?
> 
> Also, let's clarify that it is the WebContents this throttle is associated
with,
> not just any WebContents.

I think it's more of a special case for when the check may destroy the
WebContents when it's performed. If you know during WillStartRequest that you
will destroy it (asynchronously) then canceling seems more proper.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:46: // perform the destrutction
asynchronously.
On 2015/11/09 17:42:41, mnaganov wrote:
> typo: destruction

Done.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:52: // alive during teh duration of
this method. Failing to do so will result in
On 2015/11/09 17:42:41, mnaganov wrote:
> typo: the

Done.

https://codereview.chromium.org/1414723008/diff/60001/content/public/browser/...
content/public/browser/navigation_throttle.h:55: // perform the destrutction
asynchronously.
On 2015/11/09 17:42:41, mnaganov wrote:
> typo: destruction

Done.

[clamy, 2015-11-13 14:24:56]

@asargent: friendly ping :)

[clamy, 2015-11-17 14:32:00]

clamy@chromium.org changed reviewers:
+ benwells@chromium.org

[clamy, 2015-11-17 14:32:24]

@benwells: PTAL at the changes in chrome/browser/apps

[asargent_no_longer_on_chrome, 2015-11-17 17:39:28]

On 2015/11/13 14:24:56, clamy wrote:
> @asargent: friendly ping :)

Apologies for delay - I was out on vacation for a week and am slowly digging
myself out from under the backlog. I'll have a look today.

[asargent_no_longer_on_chrome, 2015-11-17 17:41:50]

lgtm

[clamy, 2015-11-18 10:41:25]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-11-18 10:41:25]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
mnaganov@chromium.org
Link to the patchset: https://codereview.chromium.org/1414723008/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-11-18 10:41:46]

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

[commit-bot: I haz the power, 2015-11-18 12:49:06]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-11-18 12:50:05]

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