Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate. https://crbug.com/621856

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[Mike West, 2016-06-24 14:50:19]

Description was changed from

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in '::GetRenderFrameHost()' is no longer accurate.

BUG=555418
R=clamy@chromium.org
==========

to

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in '::GetRenderFrameHost()' is no longer accurate.

BUG=555418
R=clamy@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-24 14:51:34]

mkwst@chromium.org changed reviewers:
+ melandory@chromium.org, msramek@chromium.org

[Mike West, 2016-06-24 14:51:35]

Since we have three patches in flight that change this line, I pulled this out
into a separate patch. WDYT, clamy@?

melandory@, msramek@: FYI.

[msramek, 2016-06-24 15:01:11]

LGTM in the sense that I need it, but Tanja has more background (this is not
*yet* working properly with web<->extension navigations IIRC?).

[nasko, 2016-06-24 16:08:40]

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

[nasko, 2016-06-24 16:08:41]

Thanks for putting this up! It allows for some discussion to be had, separate
from the actual usage of it.

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

https://codereview.chromium.org/2097083002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:188: "delivered for
processing.";
I'm afraid of making this change. Mainly because with the current navigation
path calling this at WillProcessResponse can give you the wrong RenderFrameHost.
If used to update some state, it will be incorrect.

I was actually thinking about this on my travel back from BlinkOn. There are two
types of cases that I am aware of that will call this:
* XFO throttle, which doesn't actually care about the RFH itself. It cares to
check the ancestor chain. In that case, we can expose the parent RFH as a
specific method one can call on the handle. 
* Code which needs to setup state on the RFH before it has committed. This type
of code should use ReadyToCommitNavigation. Yes, it doesn't currently work in
default navigation code, but we are working on getting it there.

So overall, I think we can do this change, but put a TODO to remove it once
ReadyToCommitNavigation is supported in the current navigation code.

[Mike West, 2016-06-27 08:15:00]

On 2016/06/24 at 15:01:11, msramek wrote:
> LGTM in the sense that I need it, but Tanja has more background (this is not
*yet* working properly with web<->extension navigations IIRC?).

Do we allow web->extension navigations? That's surprising.

On 2016/06/24 at 16:08:41, nasko wrote:
>
https://codereview.chromium.org/2097083002/diff/1/content/browser/frame_host/...
> content/browser/frame_host/navigation_handle_impl.cc:188: "delivered for
processing.";
> I'm afraid of making this change. Mainly because with the current navigation
path calling this at WillProcessResponse can give you the wrong RenderFrameHost.
If used to update some state, it will be incorrect.

We could return a const variant if that would be helpful. The use cases I know
of don't need to modify anything on the RenderFrameHost:

* https://codereview.chromium.org/2025683003 and
https://codereview.chromium.org/1617043002/ are only using the RenderFrameHost
to warn developers of missteps via console messages. This currently requires a
non-const call to `RenderFrameHost::Send`, but I'm sure we could come up with
something to make it a const operation.

* https://codereview.chromium.org/2060313002 uses it to look up a corresponding
ContentSubresourceFilterDriver, and could use a const version instead of the
current raw pointer.

Would that make you happier?

> I was actually thinking about this on my travel back from BlinkOn. There are
two types of cases that I am aware of that will call this:
> * XFO throttle, which doesn't actually care about the RFH itself. It cares to
check the ancestor chain. In that case, we can expose the parent RFH as a
specific method one can call on the handle.

The ancestor chain check isn't done via the RFH, but via the FrameTree (which we
can get to via the NavigationHandle itself).

> * Code which needs to setup state on the RFH before it has committed. This
type of code should use ReadyToCommitNavigation. Yes, it doesn't currently work
in default navigation code, but we are working on getting it there.

I don't think any of the new uses need this capability, nor do any existing uses
require it (because of the DCHECK).

> So overall, I think we can do this change, but put a TODO to remove it once
ReadyToCommitNavigation is supported in the current navigation code.

I've added a TODO.

[nasko, 2016-06-27 17:09:44]

On 2016/06/27 08:15:00, Mike West wrote:
> On 2016/06/24 at 15:01:11, msramek wrote:
> > LGTM in the sense that I need it, but Tanja has more background (this is not
> *yet* working properly with web<->extension navigations IIRC?).
> 
> Do we allow web->extension navigations? That's surprising.

Ever heard of "web_accessible_resources"? ;)
 
> On 2016/06/24 at 16:08:41, nasko wrote:
> >
>
https://codereview.chromium.org/2097083002/diff/1/content/browser/frame_host/...
> > content/browser/frame_host/navigation_handle_impl.cc:188: "delivered for
> processing.";
> > I'm afraid of making this change. Mainly because with the current navigation
> path calling this at WillProcessResponse can give you the wrong
RenderFrameHost.
> If used to update some state, it will be incorrect.
> 
> We could return a const variant if that would be helpful. 

Usage of const in the content/public API is forbidden
(https://www.chromium.org/developers/content-module/content-api), so that won't
be possible.

> The use cases I know
> of don't need to modify anything on the RenderFrameHost:
> 
> * https://codereview.chromium.org/2025683003 and
> https://codereview.chromium.org/1617043002/ are only using the RenderFrameHost
> to warn developers of missteps via console messages. This currently requires a
> non-const call to `RenderFrameHost::Send`, but I'm sure we could come up with
> something to make it a const operation.

If it is for messaging to the devs in the console, then we definitely don't want
the RFH that is navigating (conceptually, even though it might be the same). We
want the existing RFH, as if you block the navigation, you want the message to
appear in the existing one, right? Maybe this calls for an API on
NavigationHandle, something specifically designed for this use case?
 
> * https://codereview.chromium.org/2060313002 uses it to look up a
corresponding
> ContentSubresourceFilterDriver, and could use a const version instead of the
> current raw pointer.

This is specifically the reason why we want to support ReadyToCommit in default
navigation code. When a transfer is about to happen, this CL will do the wrong
thing. We had a lengthy discussion on the topic when in town for BlinkOn.

> Would that make you happier?

Shipping PlzNavigate will make me happier ;).
 
> > I was actually thinking about this on my travel back from BlinkOn. There are
> two types of cases that I am aware of that will call this:
> > * XFO throttle, which doesn't actually care about the RFH itself. It cares
to
> check the ancestor chain. In that case, we can expose the parent RFH as a
> specific method one can call on the handle.
> 
> The ancestor chain check isn't done via the RFH, but via the FrameTree (which
we
> can get to via the NavigationHandle itself).

Even more reason to not need the RFH itself.
 
> > * Code which needs to setup state on the RFH before it has committed. This
> type of code should use ReadyToCommitNavigation. Yes, it doesn't currently
work
> in default navigation code, but we are working on getting it there.
> 
> I don't think any of the new uses need this capability, nor do any existing
uses
> require it (because of the DCHECK).

The subresource filter one needs exactly that.
 
> > So overall, I think we can do this change, but put a TODO to remove it once
> ReadyToCommitNavigation is supported in the current navigation code.
> 
> I've added a TODO.

Thanks!

[Mike West, 2016-06-27 17:31:17]

On 2016/06/27 at 17:09:44, nasko wrote:
> > Do we allow web->extension navigations? That's surprising.
> 
> Ever heard of "web_accessible_resources"? ;)

I thought that allowed subresource loading, not main-frame navigations.
 
> > On 2016/06/24 at 16:08:41, nasko wrote:
> > > I'm afraid of making this change. Mainly because with the current
navigation
> > path calling this at WillProcessResponse can give you the wrong
RenderFrameHost.
> > If used to update some state, it will be incorrect.
> > 
> > We could return a const variant if that would be helpful. 
> 
> Usage of const in the content/public API is forbidden
(https://www.chromium.org/developers/content-module/content-api), so that won't
be possible.

I'm suggesting `const RenderFrameHostImpl* GetRenderFrameHost()`, not
`RenderFrameHostImpl* GetRenderFrameHost() const`. My understanding is that the
latter is not something we want, but that the former is fine. Is that not
accurate?

> > * https://codereview.chromium.org/2025683003 and
> > https://codereview.chromium.org/1617043002/ are only using the
RenderFrameHost
> > to warn developers of missteps via console messages. This currently requires
a
> > non-const call to `RenderFrameHost::Send`, but I'm sure we could come up
with
> > something to make it a const operation.
> 
> If it is for messaging to the devs in the console, then we definitely don't
want the RFH that is navigating (conceptually, even though it might be the
same). We want the existing RFH, as if you block the navigation, you want the
message to appear in the existing one, right?

Correct.

> Maybe this calls for an API on NavigationHandle, something specifically
designed for this use case?

1. Putting an API on NavigationHandle sounds fine.

2. Putting the API in a new place doesn't solve the underlying problem of
needing to pass the message down to the right host, does it? It would be simpler
at the callsite, but the timing would be the same.

> > * https://codereview.chromium.org/2060313002 uses it to look up a
corresponding
> > ContentSubresourceFilterDriver, and could use a const version instead of the
> > current raw pointer.
> 
> This is specifically the reason why we want to support ReadyToCommit in
default navigation code. When a transfer is about to happen, this CL will do the
wrong thing. We had a lengthy discussion on the topic when in town for BlinkOn.

I'll defer to you here, as I wasn't in on those discussions.

I don't know if you're telling me "No, this CL is wrong. Let's wait for the
throttle refactorings that will support ReadyToCommit." or "Let's do this with
the understanding that it's incorrect in some cases." :)

> > > * Code which needs to setup state on the RFH before it has committed. This
> > type of code should use ReadyToCommitNavigation. Yes, it doesn't currently
work
> > in default navigation code, but we are working on getting it there.
> > 
> > I don't think any of the new uses need this capability, nor do any existing
uses
> > require it (because of the DCHECK).
> 
> The subresource filter one needs exactly that.

I might be misunderstanding things, but the patch melandory@ has up right now
doesn't set data on the host, it only uses the host to lookup the filter driver
in the existing `ContentSubresourceFilterDriverFactory::DriverFromFrameHost`.
It's an ID, not an object that's manipulated.

> > > So overall, I think we can do this change, but put a TODO to remove it
once
> > ReadyToCommitNavigation is supported in the current navigation code.
> > 
> > I've added a TODO.
> 
> Thanks!

Great!

So, do you want to land this, or do you want something different? :)

[nasko, 2016-06-27 17:43:32]

On 2016/06/27 17:31:17, Mike West wrote:
> On 2016/06/27 at 17:09:44, nasko wrote:
> > > Do we allow web->extension navigations? That's surprising.
> > 
> > Ever heard of "web_accessible_resources"? ;)
> 
> I thought that allowed subresource loading, not main-frame navigations.

You can iframe extensions, which count as navigations, not just inclusion of
subresources within a document.
  
> > > On 2016/06/24 at 16:08:41, nasko wrote:
> > > > I'm afraid of making this change. Mainly because with the current
> navigation
> > > path calling this at WillProcessResponse can give you the wrong
> RenderFrameHost.
> > > If used to update some state, it will be incorrect.
> > > 
> > > We could return a const variant if that would be helpful. 
> > 
> > Usage of const in the content/public API is forbidden
> (https://www.chromium.org/developers/content-module/content-api), so that
won't
> be possible.
> 
> I'm suggesting `const RenderFrameHostImpl* GetRenderFrameHost()`, not
> `RenderFrameHostImpl* GetRenderFrameHost() const`. My understanding is that
the
> latter is not something we want, but that the former is fine. Is that not
> accurate?
> 
> > > * https://codereview.chromium.org/2025683003 and
> > > https://codereview.chromium.org/1617043002/ are only using the
> RenderFrameHost
> > > to warn developers of missteps via console messages. This currently
requires
> a
> > > non-const call to `RenderFrameHost::Send`, but I'm sure we could come up
> with
> > > something to make it a const operation.
> > 
> > If it is for messaging to the devs in the console, then we definitely don't
> want the RFH that is navigating (conceptually, even though it might be the
> same). We want the existing RFH, as if you block the navigation, you want the
> message to appear in the existing one, right?
> 
> Correct.
> 
> > Maybe this calls for an API on NavigationHandle, something specifically
> designed for this use case?
> 
> 1. Putting an API on NavigationHandle sounds fine.
> 
> 2. Putting the API in a new place doesn't solve the underlying problem of
> needing to pass the message down to the right host, does it? It would be
simpler
> at the callsite, but the timing would be the same.

The callsite needs to have enough information, right? Also, for callers outside
of content/, they might not have access to poke at current RFH/navigating
RFH/and other RFHM internals.
 
> > > * https://codereview.chromium.org/2060313002 uses it to look up a
> corresponding
> > > ContentSubresourceFilterDriver, and could use a const version instead of
the
> > > current raw pointer.
> > 
> > This is specifically the reason why we want to support ReadyToCommit in
> default navigation code. When a transfer is about to happen, this CL will do
the
> wrong thing. We had a lengthy discussion on the topic when in town for
BlinkOn.
> 
> I'll defer to you here, as I wasn't in on those discussions.
> 
> I don't know if you're telling me "No, this CL is wrong. Let's wait for the
> throttle refactorings that will support ReadyToCommit." or "Let's do this with
> the understanding that it's incorrect in some cases." :)

It is the latter, as we agreed it will take a bit to get ReadyToCommit working.
Definitely not in M53.
 
> > > > * Code which needs to setup state on the RFH before it has committed.
This
> > > type of code should use ReadyToCommitNavigation. Yes, it doesn't currently
> work
> > > in default navigation code, but we are working on getting it there.
> > > 
> > > I don't think any of the new uses need this capability, nor do any
existing
> uses
> > > require it (because of the DCHECK).
> > 
> > The subresource filter one needs exactly that.
> 
> I might be misunderstanding things, but the patch melandory@ has up right now
> doesn't set data on the host, it only uses the host to lookup the filter
driver
> in the existing `ContentSubresourceFilterDriverFactory::DriverFromFrameHost`.
> It's an ID, not an object that's manipulated.

Last time I read the CL (granted about a week ago) it was sending an IPC to the
renderer to activate the filter. In the cases of transfer, this will go to the
wrong RFH. However, in the discussion we concluded that for M53 those cases will
be rare and it is acceptable trade-off.

> > > > So overall, I think we can do this change, but put a TODO to remove it
> once
> > > ReadyToCommitNavigation is supported in the current navigation code.
> > > 
> > > I've added a TODO.
> > 
> > Thanks!
> 
> Great!
> 
> So, do you want to land this, or do you want something different? :)

Land it : ). I wanted to see if there is another round of discussion from your
side, hence why I didn't stamp it.

As it is, LGTM to land this, with the goal of undoing it once ReadyToCommit is
working and exploring a solution to the messaging use case in parallel.

[Mike West, 2016-06-28 07:24:23]

Description was changed from

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in '::GetRenderFrameHost()' is no longer accurate.

BUG=555418
R=clamy@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommit' works correctly outside
of PlzNavigate.

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-28 07:24:48]

Description was changed from

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommit' works correctly outside
of PlzNavigate.

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate.

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-28 07:25:00]

Description was changed from

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate.

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate. https://crbug.com/621856

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-06-28 07:26:12]

On 2016/06/27 at 17:43:32, nasko wrote:
> The callsite needs to have enough information, right? Also, for callers
outside of content/, they might not have access to poke at current
RFH/navigating RFH/and other RFHM internals.

Ok. I'll look into adding something like this to the NavigationHandle itself.

> As it is, LGTM to land this, with the goal of undoing it once ReadyToCommit is
working and exploring a solution to the messaging use case in parallel.

Noted in the CL description, thanks!

[Mike West, 2016-06-28 07:26:17]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-06-28 07:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2097083002/#ps20001
(title: "nasko@")

[commit-bot: I haz the power, 2016-06-28 07:26:26]

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-06-28 09:34:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-28 09:36:34]

Description was changed from

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate. https://crbug.com/621856

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update the CHECK in 'NavigationHandleImpl::GetRenderFrameHost()'

After https://codereview.chromium.org/1659163006, the 'RenderFrameHost'
for each 'NavigationHandle' is set during execution of
'NavigationHandleImpl::WillProcessResponse()'. This means that the
CHECK in 'GetRenderFrameHost()' is no longer accurate.

That said, if PlzNavigate is inactive, the navigation may transfer to
a new host up until the point that 'DidFinishNavigation' is called. We
ought to revert this patch once 'ReadyToCommitNavigation' works
correctly outside of PlzNavigate. https://crbug.com/621856

BUG=555418
R=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-06-28 09:36:35]

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