PlzNavigate: properly set the initiator of the navigation

PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

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

[clamy, 2016-06-27 11:44:23]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
==========

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-06-27 11:44:59]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-06-27 11:44:59]

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

[clamy, 2016-06-27 11:45:14]

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

[clamy, 2016-06-27 11:46:08]

@nkwst, nasko: PTAL
@carlosk: FYI (this fixes the tests you're adding to the filter in your CQ CL).

[Mike West, 2016-06-27 12:10:08]

Moving the data to 'BeginNavigationParams' looks good % comments. Thanks for
poking at this!

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

https://codereview.chromium.org/2099243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:92: :
frame_tree_node->frame_tree()->root()->current_origin();
I don't believe that this is correct, as it's always going to pull the main
frame's origin as opposed to the origin that actually initiated the navigation.
If A is nested in B, navigations that A initiates should have A as their
initiator. This code would give them B.

That said, I don't think the previous code (that I wrote, sorry!) was correct
either, as it picked the wrong initiator in the opposite case (where the parent
navigates the child). I didn't write enough tests, obviously.

I think we're actually going to have to do this when triggering the navigation
in Blink to have real assurance that we're doing the right thing. But I can do
that in a subsequent CL, that's not your problem.

https://codereview.chromium.org/2099243002/diff/1/content/common/navigation_p...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/1/content/common/navigation_p...
content/common/navigation_params.h:167: // Indicates the initiator of the
request. Normally this is the top origin of
What is a "top origin"?

https://codereview.chromium.org/2099243002/diff/1/content/test/test_render_fr...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2099243002/diff/1/content/test/test_render_fr...
content/test/test_render_frame_host.cc:368: REQUEST_CONTEXT_TYPE_HYPERLINK,
url::Origin(url));
Can you change this to a unique origin (`url::Origin()`), and add a comment to
the effect of `// TODO(mkwst): The initiator origin here is incorrect.`? I don't
think any test which relies on this code actually cares about the initiator, but
if any are added, we'll need to be a little more clever with our implementation
here.

[clamy, 2016-06-27 12:33:29]

Thanks!

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

https://codereview.chromium.org/2099243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:92: :
frame_tree_node->frame_tree()->root()->current_origin();
On 2016/06/27 12:10:08, Mike West wrote:
> I don't believe that this is correct, as it's always going to pull the main
> frame's origin as opposed to the origin that actually initiated the
navigation.
> If A is nested in B, navigations that A initiates should have A as their
> initiator. This code would give them B.
> 
> That said, I don't think the previous code (that I wrote, sorry!) was correct
> either, as it picked the wrong initiator in the opposite case (where the
parent
> navigates the child). I didn't write enough tests, obviously.
> 
> I think we're actually going to have to do this when triggering the navigation
> in Blink to have real assurance that we're doing the right thing. But I can do
> that in a subsequent CL, that's not your problem.

This is only triggered by browser-initiated navigations (in which we have no
info on initiator). In practice, we would only reach this for subframes during a
history navigation. Unless we have the information in history, I don't really
have a better choice than using the top document origin. For renderer-initiated
navigations (ie all other subframe navigations), we rely on Blink's information
that is transmitted in the BeginNavigationparams.

If this is problematic for browser-initiated navigations, I think it is also the
case in the current architecture, as I couldn't find any info about initiator in
either OpenURl or NavigationEntry/FrameNavigationEntry.

[Mike West, 2016-06-27 12:55:13]

Thanks for the clarification. LGTM % pending testing nit and compilation error.
:)

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

https://codereview.chromium.org/2099243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:92: :
frame_tree_node->frame_tree()->root()->current_origin();
On 2016/06/27 at 12:33:29, clamy wrote:
> This is only triggered by browser-initiated navigations (in which we have no
info on initiator). In practice, we would only reach this for subframes during a
history navigation. Unless we have the information in history, I don't really
have a better choice than using the top document origin. For renderer-initiated
navigations (ie all other subframe navigations), we rely on Blink's information
that is transmitted in the BeginNavigationparams.

Ah, right. I missed the change in `content/renderer/render_frame_impl.cc` where
you're populating that data. I think there still might be some work to do there
to verify that things are set correctly on the Blink side, but that's work for a
separate CL.

[clamy, 2016-06-27 13:36:15]

Thanks!

https://codereview.chromium.org/2099243002/diff/1/content/common/navigation_p...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/1/content/common/navigation_p...
content/common/navigation_params.h:167: // Indicates the initiator of the
request. Normally this is the top origin of
On 2016/06/27 12:10:08, Mike West wrote:
> What is a "top origin"?

Clarified the comment.

https://codereview.chromium.org/2099243002/diff/1/content/test/test_render_fr...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/2099243002/diff/1/content/test/test_render_fr...
content/test/test_render_frame_host.cc:368: REQUEST_CONTEXT_TYPE_HYPERLINK,
url::Origin(url));
On 2016/06/27 12:10:08, Mike West wrote:
> Can you change this to a unique origin (`url::Origin()`), and add a comment to
> the effect of `// TODO(mkwst): The initiator origin here is incorrect.`? I
don't
> think any test which relies on this code actually cares about the initiator,
but
> if any are added, we'll need to be a little more clever with our
implementation
> here.

Done.

[carlosk, 2016-06-28 12:03:17]

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

[carlosk, 2016-06-28 12:03:17]

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

https://codereview.chromium.org/2099243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:92: :
frame_tree_node->frame_tree()->root()->current_origin();
On 2016/06/27 12:55:13, Mike West wrote:
> On 2016/06/27 at 12:33:29, clamy wrote:
> > This is only triggered by browser-initiated navigations (in which we have no
> info on initiator). In practice, we would only reach this for subframes during
a
> history navigation. Unless we have the information in history, I don't really
> have a better choice than using the top document origin. For
renderer-initiated
> navigations (ie all other subframe navigations), we rely on Blink's
information
> that is transmitted in the BeginNavigationparams.
> 
> Ah, right. I missed the change in `content/renderer/render_frame_impl.cc`
where
> you're populating that data. I think there still might be some work to do
there
> to verify that things are set correctly on the Blink side, but that's work for
a
> separate CL.

nit: should a TODO be added for that too?

[nasko, 2016-06-28 18:32:23]

https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:91: ? url::Origin(dest_url)
This just feels wrong. Why would the initiator be something we want to go to?!

https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
content/common/navigation_params.h:168: // url of the navigation. In auxilliary
navigations, this is the origin of
Really? I always thought of initiator as the current document that triggered the
navigation, not the URL we are navigating to.

[clamy, 2016-06-29 11:23:20]

https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:91: ? url::Origin(dest_url)
On 2016/06/28 18:32:22, nasko wrote:
> This just feels wrong. Why would the initiator be something we want to go to?!

I based myself on
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
which states "User-initiated top-level navigations have a first-party and
initiator that matches the URL to which they navigate."

This is the browser-initiated case, so I think this counts as user-initiated. On
top of that, it being the user-initiated case means that I don't have any
initiator info available (it's not in the NavigationEntry, and it's not in the
FrameNavigationEntry).

https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
content/common/navigation_params.h:168: // url of the navigation. In auxilliary
navigations, this is the origin of
On 2016/06/28 18:32:23, nasko wrote:
> Really? I always thought of initiator as the current document that triggered
the
> navigation, not the URL we are navigating to.

I don't know. I followed the comments of the
RequestDataResourceDispatcherHostBrowserTests
(https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...).

[nasko, 2016-06-30 22:47:25]

On 2016/06/29 11:23:20, clamy wrote:
>
https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/navigation_request.cc (right):
> 
>
https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigation_request.cc:91: ? url::Origin(dest_url)
> On 2016/06/28 18:32:22, nasko wrote:
> > This just feels wrong. Why would the initiator be something we want to go
to?!
> 
> I based myself on
>
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
> which states "User-initiated top-level navigations have a first-party and
> initiator that matches the URL to which they navigate."
> 
> This is the browser-initiated case, so I think this counts as user-initiated.
On
> top of that, it being the user-initiated case means that I don't have any
> initiator info available (it's not in the NavigationEntry, and it's not in the
> FrameNavigationEntry).
> 
>
https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
> File content/common/navigation_params.h (right):
> 
>
https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
> content/common/navigation_params.h:168: // url of the navigation. In
auxilliary
> navigations, this is the origin of
> On 2016/06/28 18:32:23, nasko wrote:
> > Really? I always thought of initiator as the current document that triggered
> the
> > navigation, not the URL we are navigating to.
> 
> I don't know. I followed the comments of the
> RequestDataResourceDispatcherHostBrowserTests
>
(https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...).

It looks like Mike is out for the next couple of days. I would rather clarify
this initiator concept than proceed forward with something not super intuitive. 

Mike, can you help me understand why an initiator of a navigation will be the
URL we are *going* to? It just doesn't make sense.

[Mike West, 2016-07-04 08:51:03]

On 2016/06/30 at 22:47:25, nasko wrote:
> On 2016/06/29 11:23:20, clamy wrote:
> >
https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_request.cc (right):
> > 
> >
https://codereview.chromium.org/2099243002/diff/20001/content/browser/frame_h...
> > content/browser/frame_host/navigation_request.cc:91: ? url::Origin(dest_url)
> > On 2016/06/28 18:32:22, nasko wrote:
> > > This just feels wrong. Why would the initiator be something we want to go
to?!
> > 
> > I based myself on
> >
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
> > which states "User-initiated top-level navigations have a first-party and
> > initiator that matches the URL to which they navigate."
> > 
> > This is the browser-initiated case, so I think this counts as
user-initiated. On
> > top of that, it being the user-initiated case means that I don't have any
> > initiator info available (it's not in the NavigationEntry, and it's not in
the
> > FrameNavigationEntry).
> > 
> >
https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
> > File content/common/navigation_params.h (right):
> > 
> >
https://codereview.chromium.org/2099243002/diff/20001/content/common/navigati...
> > content/common/navigation_params.h:168: // url of the navigation. In
auxilliary
> > navigations, this is the origin of
> > On 2016/06/28 18:32:23, nasko wrote:
> > > Really? I always thought of initiator as the current document that
triggered
> > the
> > > navigation, not the URL we are navigating to.
> > 
> > I don't know. I followed the comments of the
> > RequestDataResourceDispatcherHostBrowserTests
> >
(https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...).
> 
> It looks like Mike is out for the next couple of days. I would rather clarify
this initiator concept than proceed forward with something not super intuitive. 
> 
> Mike, can you help me understand why an initiator of a navigation will be the
URL we are *going* to? It just doesn't make sense.

At a low-level, we're currently doing this because we can't distinguish between
an "empty" initiator and a "unique" initiator for two reasons:

1. `url::Origin` lacks the distinction entirely. We should add it.
2. `(blink::)ResourceRequest` is initiated with a unique origin. I have half a
patch in flight that will convert it to initialize with a `nullptr` instead.

At a high-level, we want to treat user-initiated top-level navigations as
"same-site" for the purposes of cookies. Setting the initiator to the endpoint's
origin is one way to accomplish that.

[Mike West, 2016-07-06 12:16:08]

On 2016/07/04 at 08:51:03, Mike West wrote:
> At a low-level, we're currently doing this because we can't distinguish
between an "empty" initiator and a "unique" initiator for two reasons:
> 
> 1. `url::Origin` lacks the distinction entirely. We should add it.
> 2. `(blink::)ResourceRequest` is initiated with a unique origin. I have half a
patch in flight that will convert it to initialize with a `nullptr` instead.
> 
> At a high-level, we want to treat user-initiated top-level navigations as
"same-site" for the purposes of cookies. Setting the initiator to the endpoint's
origin is one way to accomplish that.

This wasn't clear: the current behavior works, but is confusing. We should
change it. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=625969 to
cover this cleanup.

[nasko, 2016-07-18 23:39:30]

On 2016/07/06 12:16:08, Mike West wrote:
> On 2016/07/04 at 08:51:03, Mike West wrote:
> > At a low-level, we're currently doing this because we can't distinguish
> between an "empty" initiator and a "unique" initiator for two reasons:
> > 
> > 1. `url::Origin` lacks the distinction entirely. We should add it.
> > 2. `(blink::)ResourceRequest` is initiated with a unique origin. I have half
a
> patch in flight that will convert it to initialize with a `nullptr` instead.
> > 
> > At a high-level, we want to treat user-initiated top-level navigations as
> "same-site" for the purposes of cookies. Setting the initiator to the
endpoint's
> origin is one way to accomplish that.
> 
> This wasn't clear: the current behavior works, but is confusing. We should
> change it. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=625969
to
> cover this cleanup.

I assume this CL will wait for https://codereview.chromium.org/2128503002/, but
let me know if that is not the case.

[clamy, 2016-11-10 17:06:29]

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

[commit-bot: I haz the power, 2016-11-10 17:07:05]

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-11-10 17:07:09]

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

[commit-bot: I haz the power, 2016-11-10 17:07:10]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[clamy, 2016-11-10 17:32:00]

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

[commit-bot: I haz the power, 2016-11-10 17:32:28]

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-11-10 17:32:29]

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

[commit-bot: I haz the power, 2016-11-10 17:32:30]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[clamy, 2016-11-10 17:32:32]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
==========

[clamy, 2016-11-10 17:32:47]

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

[commit-bot: I haz the power, 2016-11-10 17:33: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-11-10 18:21:17]

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

[commit-bot: I haz the power, 2016-11-10 18:21:18]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-11-14 13:37:49]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

BUG=475027
==========

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

[clamy, 2016-11-14 13:37:55]

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

[commit-bot: I haz the power, 2016-11-14 13:38:08]

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

[clamy, 2016-11-14 13:42:04]

@nasko, mkwst: PTAL. This is a rebase on the optional initiator patch. Note that
while it is easy in PlzNavigate to have browser initiated main frame navigations
have an empty initiator, this is not the case currently.

https://codereview.chromium.org/2099243002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/2099243002/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_browsertest.cc:833: if
(IsBrowserSideNavigationEnabled()) {
Currently we always set an initiator for requests, so I had to put in place
different checks for PlzNavigate.

[commit-bot: I haz the power, 2016-11-14 15:29:31]

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

[commit-bot: I haz the power, 2016-11-14 15:29:32]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-17 14:20:41]

The high-level change LGTM. If carlosk@/nasko@ are happy with the details, I'm
happy with the end result. :)

[nasko, 2016-11-18 19:49:15]

https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:156:
frame_tree_node->frame_tree()->root()->current_origin());
Why have an initiator origin for browser-initiated navigations? Also, why the
main document for subframes? It seems somewhat arbitrary.

Question for Mike: Is there any spec what this initiator is supposed to
represent and be used for?

https://codereview.chromium.org/2099243002/diff/60001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/60001/content/common/navigati...
content/common/navigation_params.h:173: // can be null during browser-initiated
navigations.
nit: s/during/for/

[Mike West, 2016-11-23 13:10:12]

On 2016/11/18 at 19:49:15, nasko wrote:
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_request.cc (right):
> 
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_request.cc:156:
frame_tree_node->frame_tree()->root()->current_origin());
> Why have an initiator origin for browser-initiated navigations?

We shouldn't have an origin for navigations that are purely browser-initiated.
That is, if the user types something in the omnibox, `null` is reasonable.

> Also, why the main document for subframes? It seems somewhat arbitrary.

What document would you suggest?

> Question for Mike: Is there any spec what this initiator is supposed to
represent and be used for?

https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.1
<-- This stuff. Basically, we need to know the site that initiated the request
so that we can properly determine whether or not a given request is "cross-site"
or "same-site" during redirects.

[nasko, 2016-11-23 18:22:45]

On 2016/11/23 13:10:12, Mike West (slow) wrote:
> On 2016/11/18 at 19:49:15, nasko wrote:
> >
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> > File content/browser/frame_host/navigation_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> > content/browser/frame_host/navigation_request.cc:156:
> frame_tree_node->frame_tree()->root()->current_origin());
> > Why have an initiator origin for browser-initiated navigations?
> 
> We shouldn't have an origin for navigations that are purely browser-initiated.
> That is, if the user types something in the omnibox, `null` is reasonable.

Outside of browser tests, I don't think there is a way to create
browser-initiated navigations in subframes, so maybe we shouldn't special case
main frames.

> > Also, why the main document for subframes? It seems somewhat arbitrary.
> 
> What document would you suggest?

I don't know : ). But reading the spec you pointed at makes it a bit more clear.
 
> > Question for Mike: Is there any spec what this initiator is supposed to
> represent and be used for?
> 
> https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.1
> <-- This stuff. Basically, we need to know the site that initiated the request
> so that we can properly determine whether or not a given request is
"cross-site"
> or "same-site" during redirects.

Well, here is where things get a bit loosely defined. The site that initiated
the navigation can be arbitrary, as you might be aware from dcheng@'s effort to
introduce the concept of navigation initiator into Blink. So is it supposed to
be the initiator site or just the main frame document's site for the page in
which the frame being navigated lives?

[clamy, 2016-11-24 10:06:58]

On 2016/11/23 18:22:45, nasko wrote:
> On 2016/11/23 13:10:12, Mike West (slow) wrote:
> > On 2016/11/18 at 19:49:15, nasko wrote:
> > >
> >
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> > > File content/browser/frame_host/navigation_request.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2099243002/diff/60001/content/browser/frame_h...
> > > content/browser/frame_host/navigation_request.cc:156:
> > frame_tree_node->frame_tree()->root()->current_origin());
> > > Why have an initiator origin for browser-initiated navigations?
> > 
> > We shouldn't have an origin for navigations that are purely
browser-initiated.
> > That is, if the user types something in the omnibox, `null` is reasonable.
> 
> Outside of browser tests, I don't think there is a way to create
> browser-initiated navigations in subframes, so maybe we shouldn't special case
> main frames.

History navigations in subframes are browser-initiated in PlzNavigate, hence the
special casing.

> 
> > > Also, why the main document for subframes? It seems somewhat arbitrary.
> > 
> > What document would you suggest?
> 
> I don't know : ). But reading the spec you pointed at makes it a bit more
clear.
>  
> > > Question for Mike: Is there any spec what this initiator is supposed to
> > represent and be used for?
> > 
> >
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.1
> > <-- This stuff. Basically, we need to know the site that initiated the
request
> > so that we can properly determine whether or not a given request is
> "cross-site"
> > or "same-site" during redirects.
> 
> Well, here is where things get a bit loosely defined. The site that initiated
> the navigation can be arbitrary, as you might be aware from dcheng@'s effort
to
> introduce the concept of navigation initiator into Blink. So is it supposed to
> be the initiator site or just the main frame document's site for the page in
> which the frame being navigated lives?

[clamy, 2016-12-06 14:19:50]

Let me sum up where we're at with this patch so that it can eventually land :):
- we agree that top-level browser-initiated navigations should not have an
initiator
- for renderer-initiated navigations, the initiator is set in the renderer so
this should be fine

The only remaining problematic case is subframe browser-initiated navigations,
which happen in PlzNavigate during history navigations. The current patch sets
the initiator in that case to the origin of the top-level frame: this ultimately
ensures that the first party url for cookies will be that of the main frame,
which I think is the correct one to use in that case. Wdyt?

[nasko, 2016-12-08 17:00:00]

On 2016/12/06 14:19:50, clamy wrote:
> Let me sum up where we're at with this patch so that it can eventually land
:):
> - we agree that top-level browser-initiated navigations should not have an
> initiator

Yes.

> - for renderer-initiated navigations, the initiator is set in the renderer so
> this should be fine

In the cases that matter for PlzNavigate, I think that's correct. With OOPIFs,
I'm not sure it is correct, but it belongs to a different discussion.

> The only remaining problematic case is subframe browser-initiated navigations,
> which happen in PlzNavigate during history navigations. The current patch sets
> the initiator in that case to the origin of the top-level frame: this
ultimately
> ensures that the first party url for cookies will be that of the main frame,
> which I think is the correct one to use in that case. Wdyt?

For the sake of making progress, I think this is reasonable, so let's commit
this and I think more in-depth discussion will happen when we are trying to fix
the issues with navigation initiators more holistically. 

LGTM

[clamy, 2016-12-08 18:07:46]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-12-08 18:07:46]

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

[commit-bot: I haz the power, 2016-12-08 18:08:45]

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-12-08 19:43:25]

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

[commit-bot: I haz the power, 2016-12-08 19:43:26]

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

[clamy, 2016-12-09 17:30:14]

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

[commit-bot: I haz the power, 2016-12-09 17:30:45]

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

[clamy, 2016-12-09 17:31:24]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

[clamy, 2016-12-09 17:31:24]

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

[clamy, 2016-12-09 17:31:44]

@horo: PTAL at the changes in the SW code.

[commit-bot: I haz the power, 2016-12-09 20:22:24]

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

[commit-bot: I haz the power, 2016-12-09 20:22:25]

Dry run: This issue passed the CQ dry run.

[horo, 2016-12-12 07:07:58]

The change in service_worker_fetch_dispatcher.cc LGTM.

> - we agree that top-level browser-initiated navigations should not have an
> initiator

There is a user facing difference of sending cookies in browser-initiated
navigation (ex: clicking bookmark).
- If PlzNavigate is NOT enabled, URLRequestHttpJob::AddCookieHeaderAndStart()
  sets SameSiteCookieMode::INCLUDE_LAX because the initiator is not set. So
  cookies which SameSite mode is strict is sent.
- If PlzNavigate is enabled, URLRequestHttpJob::AddCookieHeaderAndStart() sets
  SameSiteCookieMode::INCLUDE_LAX. So cookies which SameSite mode is strict is
  NOT sent. 

Did you agree about this difference??

https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:421:
request.request_initiator = original_request->initiator().has_value()
Please add this TODO comment:
// TODO(horo): Set first_party_for_cookies to support Same-site Cookies.

MaybeStartNavigationPreload() doesn't set first_party_for_cookies, so same-site
cookies are not included in the navigation preload request. I will create a CL
with tests to support them.

[clamy, 2016-12-12 16:52:20]

On 2016/12/12 07:07:58, horo (ooo Dec.14 - Jan.4) wrote:
> The change in service_worker_fetch_dispatcher.cc LGTM.
> 
> > - we agree that top-level browser-initiated navigations should not have an
> > initiator
> 
> There is a user facing difference of sending cookies in browser-initiated
> navigation (ex: clicking bookmark).
> - If PlzNavigate is NOT enabled, URLRequestHttpJob::AddCookieHeaderAndStart()
>   sets SameSiteCookieMode::INCLUDE_LAX because the initiator is not set. So
>   cookies which SameSite mode is strict is sent.
> - If PlzNavigate is enabled, URLRequestHttpJob::AddCookieHeaderAndStart() sets
>   SameSiteCookieMode::INCLUDE_LAX. So cookies which SameSite mode is strict is
>   NOT sent. 
> 
> Did you agree about this difference??

No thanks for pointing it out!
@mkwst & nasko:

It seems that URLRequestHttpJob::AddCookieHeaderAndStart() will treat
PlzNavigate browser-initiated navigations & non-PlzNavigate browser-initiated
navigations directly. Should we set the initiator to the url we're navigating to
to avoid that, or should we modify URLRequestHttpJob::AddCookieHeaderAndStart so
that it treats the no initiator case as the same as having the initiator be the
url we're navigating to?

> 
>
https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
> File content/browser/service_worker/service_worker_fetch_dispatcher.cc
(right):
> 
>
https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
> content/browser/service_worker/service_worker_fetch_dispatcher.cc:421:
> request.request_initiator = original_request->initiator().has_value()
> Please add this TODO comment:
> // TODO(horo): Set first_party_for_cookies to support Same-site Cookies.
> 
> MaybeStartNavigationPreload() doesn't set first_party_for_cookies, so
same-site
> cookies are not included in the navigation preload request. I will create a CL
> with tests to support them.

[clamy, 2016-12-12 16:52:33]

https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:421:
request.request_initiator = original_request->initiator().has_value()
On 2016/12/12 07:07:57, horo (ooo Dec.14 - Jan.4) wrote:
> Please add this TODO comment:
> // TODO(horo): Set first_party_for_cookies to support Same-site Cookies.
> 
> MaybeStartNavigationPreload() doesn't set first_party_for_cookies, so
same-site
> cookies are not included in the navigation preload request. I will create a CL
> with tests to support them.

Done.

[nasko, 2016-12-12 16:56:53]

On 2016/12/12 16:52:20, clamy wrote:
> On 2016/12/12 07:07:58, horo (ooo Dec.14 - Jan.4) wrote:
> > The change in service_worker_fetch_dispatcher.cc LGTM.
> > 
> > > - we agree that top-level browser-initiated navigations should not have an
> > > initiator
> > 
> > There is a user facing difference of sending cookies in browser-initiated
> > navigation (ex: clicking bookmark).
> > - If PlzNavigate is NOT enabled,
URLRequestHttpJob::AddCookieHeaderAndStart()
> >   sets SameSiteCookieMode::INCLUDE_LAX because the initiator is not set. So
> >   cookies which SameSite mode is strict is sent.
> > - If PlzNavigate is enabled, URLRequestHttpJob::AddCookieHeaderAndStart()
sets
> >   SameSiteCookieMode::INCLUDE_LAX. So cookies which SameSite mode is strict
is
> >   NOT sent. 
> > 
> > Did you agree about this difference??
> 
> No thanks for pointing it out!
> @mkwst & nasko:
> 
> It seems that URLRequestHttpJob::AddCookieHeaderAndStart() will treat
> PlzNavigate browser-initiated navigations & non-PlzNavigate browser-initiated
> navigations directly. Should we set the initiator to the url we're navigating
to
> to avoid that, or should we modify URLRequestHttpJob::AddCookieHeaderAndStart
so
> that it treats the no initiator case as the same as having the initiator be
the
> url we're navigating to?

It seems wrong to me to have the destination URL also be initiator - it is
contradictory. Wouldn't it be best to consider the lack of explicit initiator as
the signal that this is likely user-initiated top-level navigation and act
appropriately?

> >
>
https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
> > File content/browser/service_worker/service_worker_fetch_dispatcher.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2099243002/diff/100001/content/browser/servic...
> > content/browser/service_worker/service_worker_fetch_dispatcher.cc:421:
> > request.request_initiator = original_request->initiator().has_value()
> > Please add this TODO comment:
> > // TODO(horo): Set first_party_for_cookies to support Same-site Cookies.
> > 
> > MaybeStartNavigationPreload() doesn't set first_party_for_cookies, so
> same-site
> > cookies are not included in the navigation preload request. I will create a
CL
> > with tests to support them.

[clamy, 2016-12-14 15:25:49]

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

[commit-bot: I haz the power, 2016-12-14 15:26:05]

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

[clamy, 2016-12-14 15:27:12]

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

[clamy, 2016-12-14 15:28:25]

I've updated the patch so that we set the cookies similarly if we have no
initiator or if the initiator is the url we're navigating to.
@mmenke: PTAL at the changes in net/
@mkwst,nasko: can you confirm that this work for you?

Thanks!

[commit-bot: I haz the power, 2016-12-14 15:41:51]

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

[commit-bot: I haz the power, 2016-12-14 15:41:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2016-12-14 16:28:57]

https://codereview.chromium.org/2099243002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2099243002/diff/160001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:700: request_->url(),
request_->initiator().value().GetURL(),
Won't this crash if initiator is nullptr now?  Also, we should have a net test
for the change.

[clamy, 2016-12-16 16:15:48]

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

[commit-bot: I haz the power, 2016-12-16 16:16:11]

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

[clamy, 2016-12-16 17:07:42]

@mmenke: PTAL

https://codereview.chromium.org/2099243002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2099243002/diff/160001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:700: request_->url(),
request_->initiator().value().GetURL(),
On 2016/12/14 16:28:57, mmenke (Out Dec 17 to Jan 2) wrote:
> Won't this crash if initiator is nullptr now?  Also, we should have a net test
> for the change.

Yes it would, because I actually meant to write !request_->initiator() || .... .
Corrected it & added a case for it in URLRequestTest.SameSiteCookies in the
latest patch set.

[mmenke, 2016-12-16 17:21:39]

net/ LGTM

[commit-bot: I haz the power, 2016-12-16 18:04:00]

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

[commit-bot: I haz the power, 2016-12-16 18:04:02]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-12-16 20:32:42]

I don't want to derail this work any more than I have already done. If mkwst@ is
fine with the patch, let's land it and discuss separately the notion of
initiator and related behavior.

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
content/common/navigation_params.h:152: const base::Optional<url::Origin>&
initiator);
nit: initiator_origin

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
content/common/navigation_params.h:178: base::Optional<url::Origin> initiator;
nit: initiator_origin

https://codereview.chromium.org/2099243002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2099243002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6128: base::Optional<url::Origin>
initiator =
nit: initiator_origin.

https://codereview.chromium.org/2099243002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2099243002/diff/200001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:691: //   without an initiatore (only
happens for browser-initiated main frame
nit: initiator, no e needed at the end.

https://codereview.chromium.org/2099243002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
(left):

https://codereview.chromium.org/2099243002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:1:
# These tests currently fail when run with --enable-browser-side-navigation
Why is this comment removed?

[clamy, 2016-12-21 14:54:19]

Thanks!

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
content/common/navigation_params.h:152: const base::Optional<url::Origin>&
initiator);
On 2016/12/16 20:32:42, nasko wrote:
> nit: initiator_origin

Done.

https://codereview.chromium.org/2099243002/diff/200001/content/common/navigat...
content/common/navigation_params.h:178: base::Optional<url::Origin> initiator;
On 2016/12/16 20:32:42, nasko wrote:
> nit: initiator_origin

Done.

https://codereview.chromium.org/2099243002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2099243002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6128: base::Optional<url::Origin>
initiator =
On 2016/12/16 20:32:42, nasko wrote:
> nit: initiator_origin.

Done.

https://codereview.chromium.org/2099243002/diff/200001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2099243002/diff/200001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:691: //   without an initiatore (only
happens for browser-initiated main frame
On 2016/12/16 20:32:42, nasko wrote:
> nit: initiator, no e needed at the end.

Done.

https://codereview.chromium.org/2099243002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
(left):

https://codereview.chromium.org/2099243002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:1:
# These tests currently fail when run with --enable-browser-side-navigation
On 2016/12/16 20:32:42, nasko wrote:
> Why is this comment removed?

This seems to be a mistake. It's now back.

[clamy, 2016-12-21 14:54:29]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-12-21 14:54:30]

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

[commit-bot: I haz the power, 2016-12-21 14:54:49]

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-12-21 17:18:36]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482332069671450,
"parent_rev": "ad77c7d4d5095ee2240f2634fd7b46b624d969c2", "commit_rev":
"467cebed89680471e731d5cc413396e09e55934c"}

[commit-bot: I haz the power, 2016-12-21 17:19:04]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

Review-Url: https://codereview.chromium.org/2099243002
==========

[commit-bot: I haz the power, 2016-12-21 17:19:06]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-21 17:23:48]

Description was changed from

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

Review-Url: https://codereview.chromium.org/2099243002
==========

to

==========
PlzNavigate: properly set the initiator of the navigation

This CL ensures that the request initiator is properly set in navigations when
PlzNavigate is enabled. This fixes RequestDataResourceDispatcherHostBrowserTest
realted to the request initiator.

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

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

[commit-bot: I haz the power, 2016-12-21 17:23:50]

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