Make the request initiator Optional

Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

BUG=475027
Committed: https://crrev.com/f8d16c7cc9a1ca8e3f861616c495d3a1a9557fc5
Cr-Commit-Position: refs/heads/master@{#430276}

[clamy, 2016-10-07 14:57:43]

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

[commit-bot: I haz the power, 2016-10-07 14:57:56]

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

[clamy, 2016-10-07 14:58:34]

Description was changed from

==========
Introduce NullableOrigin to use for request initiator

This CL introduces a nullable version of url::Origin to use for the
request initiator. This will eventually allow to distinguish between a
unique initiator and a request with no initiator (eg. a top-level
browser-initiated navigation).

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

to

==========
Introduce NullableOrigin to use for request initiator

This CL introduces a nullable version of url::Origin to use for the
request initiator. This will eventually allow to distinguish between a
unique initiator and a request with no initiator (eg. a top-level
browser-initiated navigation).

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

[clamy, 2016-10-07 14:58:34]

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

[clamy, 2016-10-07 15:07:44]

@nasko, mkwst: PTAL. This is a follow-up on
https://codereview.chromium.org/2099243002/ where there was an issue with
differentiating a unique initiator from no initiator (for a browser-initiated
nav for example).

Note that this doesn't actually set empty initiators right now, since we're
never called with an empty SecurityOrigin in Blink.

As for the approach, it turns out that Origin is used a lot in the code, so
making it refcounted did not seem very practical. Instead, I preferred to create
another class, to use when the Origin can actually be null (like in the iniator
case).

[commit-bot: I haz the power, 2016-10-07 15:10:23]

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

[commit-bot: I haz the power, 2016-10-07 15:10:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[clamy, 2016-10-07 15:47:43]

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

[commit-bot: I haz the power, 2016-10-07 15:48:04]

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-10-07 16:02:40]

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

[commit-bot: I haz the power, 2016-10-07 16:02:41]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[nasko, 2016-10-10 22:28:12]

https://codereview.chromium.org/2405483002/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2405483002/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:534: new
url::NullableOrigin(request.requestorOrigin());
Isn't the goal of a nullable origin to have it always present, but being able to
have a null value? In this case I wouldn't expect to have a conditional
statement around this assignment.

https://codereview.chromium.org/2405483002/diff/20001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2405483002/diff/20001/content/common/resource...
content/common/resource_messages.h:111: struct
ParamTraits<scoped_refptr<url::NullableOrigin>> {
Why not place this next to the ParamTraits for url::Origin? After all, this one
inherits url::Origin. Is it because it is content/public/? Would we need it
outside of content/ to handle the omnibox, browser-initiated navigation?

[Mike West, 2016-10-11 08:50:31]

//url LGTM, if you add tests for `NullableOrigin` that show it behaving
similarly to `Origin`, and add a `TODO` for me to throw it away in the future by
making `Origin` refcounted.

https://codereview.chromium.org/2405483002/diff/20001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/2405483002/diff/20001/url/origin.h#newcode151
url/origin.h:151: class URL_EXPORT NullableOrigin : public Origin,
This feels like a stopgap that we're doing just because it's a pain in the butt
to make `Origin` itself refcounted. I'm fine with that as a temporary measure
(and because you shouldn't have to do work I said I was going to do and then
didn't...), but I'd at least like a `TODO(mkwst)` here to remove
`NullableOrigin` as future cleanup.

[clamy, 2016-10-11 12:49:35]

Ok so to sum it up, we have two possible approaches:
1) Eventually make Origin refcounted, while starting with special cases first.
This is what this patch is doing, and this is what matches what Blink is doing
as well (it keeps its SecureOrigin in a ref pointer). The issue with this is
that a lot of code needs to be modified to account for the fact that the origin
can be null.
2) Add an is_null_ member to origin. This can potentially lead to less
modifications (we only set it when it is null), but we may miss areas of the
code that are checking for unique origins and maybe should behave differently
when the origin is in fact null.

IMO, making all Origins ref counted may take some time since it's used in a lot
of places, so if we go with 1, we should accept that NullableOrigin may exist
for some amount of time. Other than that, I don't feel strongly about one or the
other.

[dcheng, 2016-10-11 17:02:16]

On 2016/10/11 12:49:35, clamy wrote:
> Ok so to sum it up, we have two possible approaches:
> 1) Eventually make Origin refcounted, while starting with special cases first.
> This is what this patch is doing, and this is what matches what Blink is doing
> as well (it keeps its SecureOrigin in a ref pointer). The issue with this is
> that a lot of code needs to be modified to account for the fact that the
origin
> can be null.

Out of curiosity, would Optional<url::Origin> work in lieu of making it
refcounted? Optional has the same issues that you mentioned for making it
refcounted: lots of things don't expect it to be unset, but I'm wondering if we
can get away without refcounting here.

> 2) Add an is_null_ member to origin. This can potentially lead to less
> modifications (we only set it when it is null), but we may miss areas of the
> code that are checking for unique origins and maybe should behave differently
> when the origin is in fact null.
> 
> IMO, making all Origins ref counted may take some time since it's used in a
lot
> of places, so if we go with 1, we should accept that NullableOrigin may exist
> for some amount of time. Other than that, I don't feel strongly about one or
the
> other.

[clamy, 2016-10-12 11:46:47]

On 2016/10/11 17:02:16, dcheng wrote:
> On 2016/10/11 12:49:35, clamy wrote:
> > Ok so to sum it up, we have two possible approaches:
> > 1) Eventually make Origin refcounted, while starting with special cases
first.
> > This is what this patch is doing, and this is what matches what Blink is
doing
> > as well (it keeps its SecureOrigin in a ref pointer). The issue with this is
> > that a lot of code needs to be modified to account for the fact that the
> origin
> > can be null.
> 
> Out of curiosity, would Optional<url::Origin> work in lieu of making it
> refcounted? Optional has the same issues that you mentioned for making it
> refcounted: lots of things don't expect it to be unset, but I'm wondering if
we
> can get away without refcounting here.

I didn't know about base::Optional until today, so I did not use it :). But
indeed I think it would work and be cleaner than using refcount. Thanks!

> 
> > 2) Add an is_null_ member to origin. This can potentially lead to less
> > modifications (we only set it when it is null), but we may miss areas of the
> > code that are checking for unique origins and maybe should behave
differently
> > when the origin is in fact null.
> > 
> > IMO, making all Origins ref counted may take some time since it's used in a
> lot
> > of places, so if we go with 1, we should accept that NullableOrigin may
exist
> > for some amount of time. Other than that, I don't feel strongly about one or
> the
> > other.

[clamy, 2016-10-14 12:57:34]

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

[commit-bot: I haz the power, 2016-10-14 12:57:49]

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

[clamy, 2016-10-14 12:58:13]

Following dcheng's suggestion, I rewrote the patch using Optional, which makes
it nicer. PTAL.

[Mike West, 2016-10-14 13:05:59]

The lack of //url LGTM. Even LGTMer than before. :)

https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:229: origin1,
origin2.has_value() ? origin2.value() : url::Origin(), filter);
Rather than passing in `url::Origin()`, could you just return `false` if
`!origin2.has_value()`?

[commit-bot: I haz the power, 2016-10-14 13:32:16]

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

[commit-bot: I haz the power, 2016-10-14 13:32:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nasko, 2016-10-17 17:44:07]

The code looks good, but the description and title of the CL should change to
match the new implementation.

[clamy, 2016-10-19 12:30:59]

Description was changed from

==========
Introduce NullableOrigin to use for request initiator

This CL introduces a nullable version of url::Origin to use for the
request initiator. This will eventually allow to distinguish between a
unique initiator and a request with no initiator (eg. a top-level
browser-initiated navigation).

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

to

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

[clamy, 2016-10-19 12:31:15]

Description was changed from

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

to

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

[clamy, 2016-10-19 14:55:08]

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

[commit-bot: I haz the power, 2016-10-19 14:55:26]

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

[clamy, 2016-10-19 16:00:43]

PTAL. The description & title of the CL have been updated.

https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:229: origin1,
origin2.has_value() ? origin2.value() : url::Origin(), filter);
On 2016/10/14 13:05:59, Mike West wrote:
> Rather than passing in `url::Origin()`, could you just return `false` if
> `!origin2.has_value()`?

Done.

[commit-bot: I haz the power, 2016-10-19 17:02:56]

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

[commit-bot: I haz the power, 2016-10-19 17:02:57]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-10-20 09:33:23]

On 2016/10/19 at 16:00:43, clamy wrote:
> PTAL. The description & title of the CL have been updated.
> 
>
https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
> File net/base/registry_controlled_domains/registry_controlled_domain.cc
(right):
> 
>
https://codereview.chromium.org/2405483002/diff/40001/net/base/registry_contr...
> net/base/registry_controlled_domains/registry_controlled_domain.cc:229:
origin1, origin2.has_value() ? origin2.value() : url::Origin(), filter);
> On 2016/10/14 13:05:59, Mike West wrote:
> > Rather than passing in `url::Origin()`, could you just return `false` if
> > `!origin2.has_value()`?
> 
> Done.

Still LGTM, thanks!

[nasko, 2016-10-21 16:45:39]

LGTM

[clamy, 2016-11-02 13:48:18]

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

[clamy, 2016-11-02 13:48:39]

@mmenke: PTAL at the changes in net/

[mmenke1, 2016-11-02 13:51:33]

On 2016/11/02 13:48:39, clamy (slow) wrote:
> @mmenke: PTAL at the changes in net/

Won't URLRequestHttpJob::AddCookieHeaderAndStart now crash if initiator_ isn't
set?

[clamy, 2016-11-02 13:58:50]

On 2016/11/02 13:51:33, mmenke (incorrect) wrote:
> On 2016/11/02 13:48:39, clamy (slow) wrote:
> > @mmenke: PTAL at the changes in net/
> 
> Won't URLRequestHttpJob::AddCookieHeaderAndStart now crash if initiator_ isn't
> set?

It should be using the version of SameDomainOrHost that supports an optional
Origin.

[mmenke, 2016-11-02 14:05:20]

On 2016/11/02 13:58:50, clamy (slow) wrote:
> On 2016/11/02 13:51:33, mmenke (incorrect) wrote:
> > On 2016/11/02 13:48:39, clamy (slow) wrote:
> > > @mmenke: PTAL at the changes in net/
> > 
> > Won't URLRequestHttpJob::AddCookieHeaderAndStart now crash if initiator_
isn't
> > set?
> 
> It should be using the version of SameDomainOrHost that supports an optional
> Origin.

Oops, missed the changes in that file.

Could you update url_fetcher as well?

Also, what do you think about DCHECKing in set_initiator if it's valid or
nullptr?  I think having it one of <not set, invalid GURL, valid GURL> is weird
- I'd rather just have one type of "invalid" value.  Does that make sense?

[clamy, 2016-11-02 18:09:16]

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

[commit-bot: I haz the power, 2016-11-02 18:09:54]

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-02 18:10:48]

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

[clamy, 2016-11-02 18:10:49]

Thanks! I've made the change to URLFetcher in the latest patch set, and added a
DCHECK to URLRequest.

+horo@ for the changes in service_worker/

[commit-bot: I haz the power, 2016-11-02 18:58:12]

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

[commit-bot: I haz the power, 2016-11-02 18:58:13]

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

[clamy, 2016-11-03 12:58:07]

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

[commit-bot: I haz the power, 2016-11-03 12:58:18]

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-03 15:10:07]

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

[commit-bot: I haz the power, 2016-11-03 15:10:09]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-03 17:58:58]

net/ LGTM

https://codereview.chromium.org/2405483002/diff/130001/net/base/registry_cont...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/2405483002/diff/130001/net/base/registry_cont...
net/base/registry_controlled_domains/registry_controlled_domain.h:205:
PrivateRegistryFilter filter);
Mention behavior for a null origin?

https://codereview.chromium.org/2405483002/diff/130001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2405483002/diff/130001/net/url_request/url_re...
net/url_request/url_request.h:308: // This method may only be called before
Start().
Is it worth mentioning the difference between a unique and invalid initiator?

[horo, 2016-11-04 03:13:35]

content/browser/service_worker/*: lgtm

https://codereview.chromium.org/2405483002/diff/130001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/2405483002/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:779:
(!request()->initiator().has_value() ||
Just to confirm: The initiator must be set when the request is not main resource
load. Right?

[clamy, 2016-11-04 16:00:04]

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

[commit-bot: I haz the power, 2016-11-04 16:00:40]

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-04 16:03:48]

clamy@chromium.org changed reviewers:
+ lfg@chromium.org, sky@chromium.org

[clamy, 2016-11-04 16:03:50]

Thanks!

@lfg: PTAL at the changes in extensions/browser/guest_view
@sky: PTAL at the changes in chrome/browser/ui/search_engines

https://codereview.chromium.org/2405483002/diff/130001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/2405483002/diff/130001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job.cc:779:
(!request()->initiator().has_value() ||
On 2016/11/04 03:13:35, horo wrote:
> Just to confirm: The initiator must be set when the request is not main
resource
> load. Right? 

Yes.

https://codereview.chromium.org/2405483002/diff/130001/net/base/registry_cont...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/2405483002/diff/130001/net/base/registry_cont...
net/base/registry_controlled_domains/registry_controlled_domain.h:205:
PrivateRegistryFilter filter);
On 2016/11/03 17:58:58, mmenke wrote:
> Mention behavior for a null origin?

Done.

https://codereview.chromium.org/2405483002/diff/130001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/2405483002/diff/130001/net/url_request/url_re...
net/url_request/url_request.h:308: // This method may only be called before
Start().
On 2016/11/03 17:58:58, mmenke wrote:
> Is it worth mentioning the difference between a unique and invalid initiator?

Done.

[lfg, 2016-11-04 16:18:47]

lgtm

[commit-bot: I haz the power, 2016-11-04 17:46:16]

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

[commit-bot: I haz the power, 2016-11-04 17:46:17]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-04 17:52:54]

LGTM

[clamy, 2016-11-07 13:35:37]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-11-07 13:35:38]

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

[commit-bot: I haz the power, 2016-11-07 13:35:57]

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-07 15:21:14]

Description was changed from

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

to

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

[commit-bot: I haz the power, 2016-11-07 15:21:16]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-11-07 15:23:16]

Description was changed from

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

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

to

==========
Make the request initiator Optional

This CL makes the request initiator used in ResourceRequests and NetURLRequests
optional. This will eventually allow to distinguish between a unique initiator
and a request with no initiator (eg. a top-level browser-initiated navigation).

BUG=475027

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

[commit-bot: I haz the power, 2016-11-07 15:23:18]

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