Create NavigationHandles for interstitials if needed

Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

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

[clamy, 2016-09-23 15:16:53]

Description was changed from

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

BUG=621856
==========

to

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

[clamy, 2016-09-23 15:16:59]

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

[commit-bot: I haz the power, 2016-09-23 15:17:11]

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, 2016-09-23 15:30:35]

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

[commit-bot: I haz the power, 2016-09-23 15:30:36]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[clamy, 2016-09-23 15:45:36]

Description was changed from

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

to

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

[clamy, 2016-09-23 15:45:36]

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

[clamy, 2016-09-23 15:47:39]

@creis, nasko: PTAL

This is a spin off from https://codereview.chromium.org/2321543002/ to get a
NavigationHandle even in interstitial cases. I though that in the interstitial
case, where we should only have one navigation keeping the handle in the
Navigator made sense, but I can also store it in the RFH. Let me know what you
think.

[nasko, 2016-09-23 17:41:35]

I really like this! However, I do have one concern - as it is, we will emit
WCO::DidStartNavigation/DidFinishNavigation for the interstitial page. I don't
think this is a desirable behavior, as we don't emit the current notifications
for interstitials and I'd like to keep it that way.

It can be easily solved by either passing the NavigatorDelegate in the
constructor with nullptr in the case of interstitial, or pass in an explicit
is_interstitial boolean. I don't know if we will need more knowledge of
interstitials in NavigationHandle, but I think it is a bit more descriptive/self
documenting way of why the notifications are skipped.

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

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.cc:42: -1);  //
pending_nav_entry_id
nit: A bunch of other places in code use 0 for invalid nav_entry_id. Maybe we
should create a symbolic constant and use that, though it is better done in
another CL.

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.cc:51:
navigation_handle_ = nullptr;
nit: navigation_handle_.reset()

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:37: // Used in unit tests
to make UI checks succeed even if there is no
nit: UI thread

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:53: // by
nit: Combine with next line.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:59: // a vlue of true.
nit: value

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:296: void
NavigationResourceThrottle::SetUIChecksAlwaysSucceedForTesting(
nit: Why the CamelCase when this is a simple setter?

[nasko, 2016-09-23 17:43:15]

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:73: if
(!render_frame_host) {
In what case are we going to reach this? DCHECK maybe?

[Charlie Reis, 2016-09-23 18:51:11]

Thanks!  LGTM, if my reasoning below is correct.

On 2016/09/23 17:41:35, nasko (slow) wrote:
> I really like this! However, I do have one concern - as it is, we will emit
> WCO::DidStartNavigation/DidFinishNavigation for the interstitial page. I don't
> think this is a desirable behavior, as we don't emit the current notifications
> for interstitials and I'd like to keep it that way.
> 
> It can be easily solved by either passing the NavigatorDelegate in the
> constructor with nullptr in the case of interstitial, or pass in an explicit
> is_interstitial boolean. I don't know if we will need more knowledge of
> interstitials in NavigationHandle, but I think it is a bit more
descriptive/self
> documenting way of why the notifications are skipped.

I agree that we don't want to send WebContentsObserver
Did{Start,Finish}Navigation events for interstitials, but I think we might be
safe already.  NavigationHandleImpl sends them to GetDelegate(), which returns
frame_tree_node_->navigator()->GetDelegate(), which (I think) will be
InterstitialPageImpl and not WebContents.  InterstitialPageImpl doesn't override
Did{Start,Finish}Navigation, so hopefully those will be no-ops.

Maybe a comment would be sufficient, as noted below?

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.h:49: // The
NavigationHandle associated with the interstitial navigation.
Let's add a note that interstitials are assumed to only have a single
RenderFrameHost, so it's ok to track the NavigationHandle here rather than
per-frame.

Let's also mention that we don't expect this to send DidStartNavigation or
DidFinishNavigation events to the WebContentsObserver, since those will go
through InterstitialPageImpl as the delegate and get dropped.

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator.h:183: virtual NavigationHandleImpl*
GetNavigationHandleForFrameHost(
Please add a comment about why this takes in a RFH (i.e., each frame can have
its own NavHandle).

[clamy, 2016-09-26 11:36:18]

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

[commit-bot: I haz the power, 2016-09-26 11:36:36]

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

[clamy, 2016-09-26 11:37:13]

On 2016/09/23 18:51:11, Charlie Reis (slow) wrote:
> Thanks!  LGTM, if my reasoning below is correct.
> 
> On 2016/09/23 17:41:35, nasko (slow) wrote:
> > I really like this! However, I do have one concern - as it is, we will emit
> > WCO::DidStartNavigation/DidFinishNavigation for the interstitial page. I
don't
> > think this is a desirable behavior, as we don't emit the current
notifications
> > for interstitials and I'd like to keep it that way.
> > 
> > It can be easily solved by either passing the NavigatorDelegate in the
> > constructor with nullptr in the case of interstitial, or pass in an explicit
> > is_interstitial boolean. I don't know if we will need more knowledge of
> > interstitials in NavigationHandle, but I think it is a bit more
> descriptive/self
> > documenting way of why the notifications are skipped.
> 
> I agree that we don't want to send WebContentsObserver
> Did{Start,Finish}Navigation events for interstitials, but I think we might be
> safe already.  NavigationHandleImpl sends them to GetDelegate(), which returns
> frame_tree_node_->navigator()->GetDelegate(), which (I think) will be
> InterstitialPageImpl and not WebContents.  InterstitialPageImpl doesn't
override
> Did{Start,Finish}Navigation, so hopefully those will be no-ops.
> 
> Maybe a comment would be sufficient, as noted below?

Yes that was my reasoning as well. I have added a comment as you suggested.

> 
>
https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/interstitial_page_navigator_impl.h (right):
> 
>
https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/interstitial_page_navigator_impl.h:49: // The
> NavigationHandle associated with the interstitial navigation.
> Let's add a note that interstitials are assumed to only have a single
> RenderFrameHost, so it's ok to track the NavigationHandle here rather than
> per-frame.
> 
> Let's also mention that we don't expect this to send DidStartNavigation or
> DidFinishNavigation events to the WebContentsObserver, since those will go
> through InterstitialPageImpl as the delegate and get dropped.
> 
>
https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/navigator.h (right):
> 
>
https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigator.h:183: virtual NavigationHandleImpl*
> GetNavigationHandleForFrameHost(
> Please add a comment about why this takes in a RFH (i.e., each frame can have
> its own NavHandle).

[clamy, 2016-09-26 11:37:27]

Thanks!

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

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.cc:42: -1);  //
pending_nav_entry_id
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: A bunch of other places in code use 0 for invalid nav_entry_id. Maybe we
> should create a symbolic constant and use that, though it is better done in
> another CL.

Acknowledged. Switched to 0 here.

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.cc:51:
navigation_handle_ = nullptr;
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: navigation_handle_.reset()

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/interstitial_page_navigator_impl.h:49: // The
NavigationHandle associated with the interstitial navigation.
On 2016/09/23 18:51:11, Charlie Reis (slow) wrote:
> Let's add a note that interstitials are assumed to only have a single
> RenderFrameHost, so it's ok to track the NavigationHandle here rather than
> per-frame.
> 
> Let's also mention that we don't expect this to send DidStartNavigation or
> DidFinishNavigation events to the WebContentsObserver, since those will go
> through InterstitialPageImpl as the delegate and get dropped.

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator.h:183: virtual NavigationHandleImpl*
GetNavigationHandleForFrameHost(
On 2016/09/23 18:51:11, Charlie Reis (slow) wrote:
> Please add a comment about why this takes in a RFH (i.e., each frame can have
> its own NavHandle).

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:37: // Used in unit tests
to make UI checks succeed even if there is no
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: UI thread

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:53: // by
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: Combine with next line.

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:59: // a vlue of true.
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: value

Done.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:73: if
(!render_frame_host) {
On 2016/09/23 17:43:14, nasko (slow) wrote:
> In what case are we going to reach this? DCHECK maybe?

I think it could be possible to reach it in race conditions where we delete a
RFH that has an ongoing navigation. One case I'm thinking about is that it's
possible to delete the WebContents directly on Android without going through the
proper close sequence. This would also delete RFHs before they have time to
cancel their ongoing navigations.

https://codereview.chromium.org/2364943002/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_resource_throttle.cc:296: void
NavigationResourceThrottle::SetUIChecksAlwaysSucceedForTesting(
On 2016/09/23 17:41:35, nasko (slow) wrote:
> nit: Why the CamelCase when this is a simple setter?

Done.

[commit-bot: I haz the power, 2016-09-26 12:34:47]

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

[commit-bot: I haz the power, 2016-09-26 12:34:48]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nasko, 2016-09-26 15:51:50]

LGTM with a couple of minor formatting nits.

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

https://codereview.chromium.org/2364943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator.h:183: // Returns the NavigationHandle
associated to a navigation in
minor nit: associated with

https://codereview.chromium.org/2364943002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2364943002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:59: // called with
nit: combine lines together.

[clamy, 2016-09-26 16:27:29]

Thanks!

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

https://codereview.chromium.org/2364943002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator.h:183: // Returns the NavigationHandle
associated to a navigation in
On 2016/09/26 15:51:50, nasko (slow) wrote:
> minor nit: associated with

Done.

https://codereview.chromium.org/2364943002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2364943002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.cc:59: // called with
On 2016/09/26 15:51:50, nasko (slow) wrote:
> nit: combine lines together.

Done.

[clamy, 2016-09-26 16:27:48]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-09-26 16:27:48]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2364943002/#ps60001
(title: "Rebase + addressed nits")

[commit-bot: I haz the power, 2016-09-26 16:28:13]

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, 2016-09-26 21:22:19]

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

[commit-bot: I haz the power, 2016-09-26 21:22:21]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nasko, 2016-09-26 21:29:17]

On 2016/09/26 21:22:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

The failure is in
org.chromium.chrome.browser.toolbar.BrandColorTest#testBrandColorInterstitial,
which seems like related to this CL, which touches interstitials.

[clamy, 2016-09-27 11:56:28]

I've investigated the Android issue and it was due to the NavigationHandle
calling directly the ContentBrowserClient to get the NavigationThrottle. Since
we do not want to add them during a navigation to an interstitial, I'm now
making this go through the NavigatorDelegate, and the call to the
ContentBrowserClient is made inside the implementation in WebContentsImpl.

I'm going to go ahead and land this, in order to have enough time to land the
dependent patch set. If you have any comments, I'll address them in a next patch
set.

[clamy, 2016-09-27 11:56:35]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-09-27 11:56:35]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2364943002/#ps80001
(title: "Fixed Android issue")

[commit-bot: I haz the power, 2016-09-27 11:56:46]

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, 2016-09-27 12:48:58]

Description was changed from

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

to

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

[commit-bot: I haz the power, 2016-09-27 12:48:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-27 12:50:24]

Description was changed from

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

to

==========
Create NavigationHandles for interstitials if needed

On Android, navigations for interstitials go through the network stack.
Not having a NavigationHandle for them prevents us from enforcing that
every navigation has a NavigationHandle. This CL fixes and adds this
requirement.

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

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

[commit-bot: I haz the power, 2016-09-27 12:50:25]

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