Allow getting both reading/setting cookie settings at one time

Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174
Committed: https://crrev.com/db2deb26ff656262712010cfd9a8580a005dc16f
Cr-Commit-Position: refs/heads/master@{#434691}

[Charlie Harrison, 2016-11-15 19:58:40]

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

[commit-bot: I haz the power, 2016-11-15 19:59:38]

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

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

[commit-bot: I haz the power, 2016-11-15 20:14:16]

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...)

[Charlie Harrison, 2016-11-15 22:04:38]

Description was changed from

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12%5 after this
patch (Linux perf results).

BUG=664174
==========

to

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174
==========

[Charlie Harrison, 2016-11-15 22:41:54]

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

[commit-bot: I haz the power, 2016-11-15 22:42:29]

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

[Charlie Harrison, 2016-11-15 22:48:12]

csharrison@chromium.org changed reviewers:
+ bauerb@chromium.org

[Charlie Harrison, 2016-11-15 22:48:12]

bauerb: WDYT about this patch? I'm not super enthusiastic about an API using
bool pointers, but I wanted to make sure the underlying GetCookieSetting didn't
have to get policy for both settings if it wasn't necessary (Can{Set,Get}Cookie
can call registry_controlled_domains::SameDomainOrHost which is a fairly
expensive method).

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

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

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

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...)

[raymes, 2016-11-16 03:02:28]

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

[raymes, 2016-11-16 03:02:28]

I'm curious if you know why it's expensive to get the underlying website
setting? I think it should be in-memory so I'm interested in which part is
expensive?

[Charlie Harrison, 2016-11-16 03:12:37]

On 2016/11/16 03:02:28, raymes wrote:
> I'm curious if you know why it's expensive to get the underlying website
> setting? I think it should be in-memory so I'm interested in which part is
> expensive?

From profiles, it looks like half the time is getting the various rule
iterators, and half the time is in GetContentSettingValueAndPatterns. I can look
into this in more details, as we're still spending a decent chunk of time here
per-request even with this patch. Note that I recorded the profile with
extensions enabled, so we spend a bit of time getting the custom extensions rule
iterator.

[Charlie Harrison, 2016-11-16 13:32:44]

One reasonably hot method is ContentSettingsPattern::Wildcard which seems like
it could be optimized. It's a static method which always returns the same
ContentSettingsPattern, but we end up going through Build() which does a lot:
 - Canonicalize, which does a bunch of string copies and host canonicalization
 - ContentSettingsPattern::Compare

The hottest rule iterator is
content_settings::OriginIdentifierValueMap::GetRuleIterator which
 - takes a lock
 - searches a map (how big?)

The only trivial change I can see is removing some string copies (esp for
comparisons). I'm not sure how tough it would be to ensure that schemes/hosts
are already canonicalized so there's no need to lower() them or canonicalize
them using //net APIs.

[msramek, 2016-11-16 14:05:49]

On 2016/11/16 13:32:44, Charlie Harrison wrote:
> One reasonably hot method is ContentSettingsPattern::Wildcard which seems like
> it could be optimized. It's a static method which always returns the same
> ContentSettingsPattern, but we end up going through Build() which does a lot:
>  - Canonicalize, which does a bunch of string copies and host canonicalization
>  - ContentSettingsPattern::Compare
> 
> The hottest rule iterator is
> content_settings::OriginIdentifierValueMap::GetRuleIterator which
>  - takes a lock
>  - searches a map (how big?)
> 
> The only trivial change I can see is removing some string copies (esp for
> comparisons). I'm not sure how tough it would be to ensure that schemes/hosts
> are already canonicalized so there's no need to lower() them or canonicalize
> them using //net APIs.

(Another uninvited reviewer jumping in!)

Wildcard() is used not only in the default setting, but also in the secondary
pattern of basically every setting. So, indeed a lot. And can be easily improved
by just setting all parts_ to a wildcard (+test that this is equivalent to what
builder would do). I would support this change.

Regarding GetRuleIterator(), not sure what can be done there. The maps are not
large - look into your chrome://settings/content, you probably don't have more
than a hundred exception. Or check ContentSettings.Exceptions.* histograms for
some data. The lock claims that it's there for thread safety of
GetRuleIterator(), but HostContentSettingsMap and all providers should be on the
UI thread, no? Maybe we can carefully replace the lock with ThreadChecker and
see what happens?

[Charlie Harrison, 2016-11-16 15:14:59]

On 2016/11/16 14:05:49, msramek wrote:
> On 2016/11/16 13:32:44, Charlie Harrison wrote:
> > One reasonably hot method is ContentSettingsPattern::Wildcard which seems
like
> > it could be optimized. It's a static method which always returns the same
> > ContentSettingsPattern, but we end up going through Build() which does a
lot:
> >  - Canonicalize, which does a bunch of string copies and host
canonicalization
> >  - ContentSettingsPattern::Compare
> > 
> > The hottest rule iterator is
> > content_settings::OriginIdentifierValueMap::GetRuleIterator which
> >  - takes a lock
> >  - searches a map (how big?)
> > 
> > The only trivial change I can see is removing some string copies (esp for
> > comparisons). I'm not sure how tough it would be to ensure that
schemes/hosts
> > are already canonicalized so there's no need to lower() them or canonicalize
> > them using //net APIs.
> 
> (Another uninvited reviewer jumping in!)
The more the merrier :)
> 
> Wildcard() is used not only in the default setting, but also in the secondary
> pattern of basically every setting. So, indeed a lot. And can be easily
improved
> by just setting all parts_ to a wildcard (+test that this is equivalent to
what
> builder would do). I would support this change.
Cool, that looks doable. I can look into that as a separate optimization!
First draft up here:
https://codereview.chromium.org/2507813002/
> 
> Regarding GetRuleIterator(), not sure what can be done there. The maps are not
> large - look into your chrome://settings/content, you probably don't have more
> than a hundred exception. Or check ContentSettings.Exceptions.* histograms for
> some data. The lock claims that it's there for thread safety of
> GetRuleIterator(), but HostContentSettingsMap and all providers should be on
the
> UI thread, no? Maybe we can carefully replace the lock with ThreadChecker and
> see what happens?
Hm I wonder if this the map is a reasonable candidate for base::SmallMap? For
small maps I imagine we'll get locality wins. Something to think about at least.

Re: lock. All the code I'm profiling is on the IO thread, so I bet the map is
necessary :) We poll this data to see if we should be enabling privacy mode for
our sockets (ChromeNetworkDelegate::OnCanEnablePrivacyMode).

[msramek, 2016-11-16 15:45:27]

On 2016/11/16 15:14:59, Charlie Harrison wrote:
> On 2016/11/16 14:05:49, msramek wrote:
> > On 2016/11/16 13:32:44, Charlie Harrison wrote:
> > > One reasonably hot method is ContentSettingsPattern::Wildcard which seems
> like
> > > it could be optimized. It's a static method which always returns the same
> > > ContentSettingsPattern, but we end up going through Build() which does a
> lot:
> > >  - Canonicalize, which does a bunch of string copies and host
> canonicalization
> > >  - ContentSettingsPattern::Compare
> > > 
> > > The hottest rule iterator is
> > > content_settings::OriginIdentifierValueMap::GetRuleIterator which
> > >  - takes a lock
> > >  - searches a map (how big?)
> > > 
> > > The only trivial change I can see is removing some string copies (esp for
> > > comparisons). I'm not sure how tough it would be to ensure that
> schemes/hosts
> > > are already canonicalized so there's no need to lower() them or
canonicalize
> > > them using //net APIs.
> > 
> > (Another uninvited reviewer jumping in!)
> The more the merrier :)
> > 
> > Wildcard() is used not only in the default setting, but also in the
secondary
> > pattern of basically every setting. So, indeed a lot. And can be easily
> improved
> > by just setting all parts_ to a wildcard (+test that this is equivalent to
> what
> > builder would do). I would support this change.
> Cool, that looks doable. I can look into that as a separate optimization!
> First draft up here:
> https://codereview.chromium.org/2507813002/
> > 
> > Regarding GetRuleIterator(), not sure what can be done there. The maps are
not
> > large - look into your chrome://settings/content, you probably don't have
more
> > than a hundred exception. Or check ContentSettings.Exceptions.* histograms
for
> > some data. The lock claims that it's there for thread safety of
> > GetRuleIterator(), but HostContentSettingsMap and all providers should be on
> the
> > UI thread, no? Maybe we can carefully replace the lock with ThreadChecker
and
> > see what happens?
> Hm I wonder if this the map is a reasonable candidate for base::SmallMap? For
> small maps I imagine we'll get locality wins. Something to think about at
least.
> 
> Re: lock. All the code I'm profiling is on the IO thread, so I bet the map is
> necessary :) We poll this data to see if we should be enabling privacy mode
for
> our sockets (ChromeNetworkDelegate::OnCanEnablePrivacyMode).

I LGTM'd that CL.

Yeah, I knew I was missing something :) The updates come from the UI and are
written into PrefService, that's all UI thread, but setting cookies is of course
IO thread...

[msramek, 2016-11-16 15:48:09]

On 2016/11/16 15:45:27, msramek wrote:
> On 2016/11/16 15:14:59, Charlie Harrison wrote:
> > On 2016/11/16 14:05:49, msramek wrote:
> > > On 2016/11/16 13:32:44, Charlie Harrison wrote:
> > > > One reasonably hot method is ContentSettingsPattern::Wildcard which
seems
> > like
> > > > it could be optimized. It's a static method which always returns the
same
> > > > ContentSettingsPattern, but we end up going through Build() which does a
> > lot:
> > > >  - Canonicalize, which does a bunch of string copies and host
> > canonicalization
> > > >  - ContentSettingsPattern::Compare
> > > > 
> > > > The hottest rule iterator is
> > > > content_settings::OriginIdentifierValueMap::GetRuleIterator which
> > > >  - takes a lock
> > > >  - searches a map (how big?)
> > > > 
> > > > The only trivial change I can see is removing some string copies (esp
for
> > > > comparisons). I'm not sure how tough it would be to ensure that
> > schemes/hosts
> > > > are already canonicalized so there's no need to lower() them or
> canonicalize
> > > > them using //net APIs.
> > > 
> > > (Another uninvited reviewer jumping in!)
> > The more the merrier :)
> > > 
> > > Wildcard() is used not only in the default setting, but also in the
> secondary
> > > pattern of basically every setting. So, indeed a lot. And can be easily
> > improved
> > > by just setting all parts_ to a wildcard (+test that this is equivalent to
> > what
> > > builder would do). I would support this change.
> > Cool, that looks doable. I can look into that as a separate optimization!
> > First draft up here:
> > https://codereview.chromium.org/2507813002/
> > > 
> > > Regarding GetRuleIterator(), not sure what can be done there. The maps are
> not
> > > large - look into your chrome://settings/content, you probably don't have
> more
> > > than a hundred exception. Or check ContentSettings.Exceptions.* histograms
> for
> > > some data. The lock claims that it's there for thread safety of
> > > GetRuleIterator(), but HostContentSettingsMap and all providers should be
on
> > the
> > > UI thread, no? Maybe we can carefully replace the lock with ThreadChecker
> and
> > > see what happens?
> > Hm I wonder if this the map is a reasonable candidate for base::SmallMap?
For
> > small maps I imagine we'll get locality wins. Something to think about at
> least.
> > 
> > Re: lock. All the code I'm profiling is on the IO thread, so I bet the map
is
> > necessary :) We poll this data to see if we should be enabling privacy mode
> for
> > our sockets (ChromeNetworkDelegate::OnCanEnablePrivacyMode).
> 
> I LGTM'd that CL.
> 
> Yeah, I knew I was missing something :) The updates come from the UI and are
> written into PrefService, that's all UI thread, but setting cookies is of
course
> IO thread...

...and I accidentally L-G-T-M'd this CL as well. I'll leave it as it is, but
wait for Bernhard please :)

[Bernhard Bauer, 2016-11-16 16:55:45]

lgtm

https://codereview.chromium.org/2502743003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/2502743003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:157:
cookie_settings->GetCookieSetting(primary_url, secondary_url, NULL,
While you're here, use nullptr instead of NULL?

https://codereview.chromium.org/2502743003/diff/20001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:81: ContentSetting
setting_setting;
Heh, setting_setting :)

[Charlie Harrison, 2016-11-16 17:01:05]

Thanks everyone for taking a look!

https://codereview.chromium.org/2502743003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/2502743003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:157:
cookie_settings->GetCookieSetting(primary_url, secondary_url, NULL,
On 2016/11/16 16:55:45, Bernhard Bauer wrote:
> While you're here, use nullptr instead of NULL?

Done (across the whole file).

https://codereview.chromium.org/2502743003/diff/20001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/20001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:81: ContentSetting
setting_setting;
On 2016/11/16 16:55:45, Bernhard Bauer wrote:
> Heh, setting_setting :)

;)

[Charlie Harrison, 2016-11-16 17:01:09]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-16 17:01:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2502743003/#ps40001
(title: "bauerb review")

[Charlie Harrison, 2016-11-16 17:01:13]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-11-16 17:03:17]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org, rdevlin.cronin@chromium.org

[Charlie Harrison, 2016-11-16 17:03:18]

oops jumped the gun a bit
Matt: would you look at chrome/browser/net?
Devlin: would you look at chrome/browser/extensions?

Thanks!

[mmenke, 2016-11-16 17:28:45]

On 2016/11/16 17:03:18, Charlie Harrison wrote:
> oops jumped the gun a bit
> Matt: would you look at chrome/browser/net?
> Devlin: would you look at chrome/browser/extensions?
> 
> Thanks!

I don't suppose there's some way to speed up the underlying code?  The fact that
such an obscure, rarely used (I assume) feature takes up significant CPU time
for every request seems problematic.

[mmenke, 2016-11-16 17:29:29]

On 2016/11/16 17:28:45, mmenke wrote:
> On 2016/11/16 17:03:18, Charlie Harrison wrote:
> > oops jumped the gun a bit
> > Matt: would you look at chrome/browser/net?
> > Devlin: would you look at chrome/browser/extensions?
> > 
> > Thanks!
> 
> I don't suppose there's some way to speed up the underlying code?  The fact
that
> such an obscure, rarely used (I assume) feature takes up significant CPU time
> for every request seems problematic.

Oh, and chrome/browser/net LGTM.

[Charlie Harrison, 2016-11-16 17:33:30]

On 2016/11/16 17:28:45, mmenke wrote:
> On 2016/11/16 17:03:18, Charlie Harrison wrote:
> > oops jumped the gun a bit
> > Matt: would you look at chrome/browser/net?
> > Devlin: would you look at chrome/browser/extensions?
> > 
> > Thanks!
> 
> I don't suppose there's some way to speed up the underlying code?  The fact
that
> such an obscure, rarely used (I assume) feature takes up significant CPU time
> for every request seems problematic.

Yep, working on that in with additional changes (one piece is linked in the
comments and already landed).

[mmenke, 2016-11-16 17:37:44]

https://codereview.chromium.org/2502743003/diff/40001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/40001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:150:
first_party_url.SchemeIs(kChromeUIScheme)) {
Shouldn't it make sense to flip these?  Not a huge per difference, but
SchemeIsCryptographic does 3 comparisons, and probably returns true more often
than the second check, so we'll often have to do 4 string compares.  Flip the
order, and we're usually doing just 1.

Probably not a huge difference, as I assume the "GetWebsiteSetting" and
"StaticCookiePolicy" calls are what really matters.

[Charlie Harrison, 2016-11-16 17:48:05]

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

[commit-bot: I haz the power, 2016-11-16 17:48:39]

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

[Charlie Harrison, 2016-11-16 17:50:03]

https://codereview.chromium.org/2502743003/diff/40001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/40001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:150:
first_party_url.SchemeIs(kChromeUIScheme)) {
On 2016/11/16 17:37:44, mmenke wrote:
> Shouldn't it make sense to flip these?  Not a huge per difference, but
> SchemeIsCryptographic does 3 comparisons, and probably returns true more often
> than the second check, so we'll often have to do 4 string compares.  Flip the
> order, and we're usually doing just 1.
> 
> Probably not a huge difference, as I assume the "GetWebsiteSetting" and
> "StaticCookiePolicy" calls are what really matters.

Swapped them. Sometimes string comparisons / memcmps lose their stacks with the
profiler toolchain I'm using so it's a bit of a blindspot. Better safe than
sorry!

[commit-bot: I haz the power, 2016-11-16 19:51:33]

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

[commit-bot: I haz the power, 2016-11-16 19:51:34]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-11-16 20:12:00]

lgtm with some questions about the existing code.

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:160:
first_party_url.SchemeIs(kExtensionScheme)) {
Also not your code, but a question for the cookies experts on this CL. :)

We auto-allow for extensions here - why do we not need to check the host (i.e.,
which extension it is)?  Does this allow unintentional cross-extension
interaction?

(My cookie knowledge is pretty limited, so I'm probably way off base, but I'd
like to understand it. :))

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:183:
!first_party_url.SchemeIs(extension_scheme_);
Not your code, but.... why do we use extension_scheme_ here and kExtensionScheme
above?

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.h (right):

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.h:60: void
GetReadingAndSettingCookieAllowed(const GURL& url,
drive-by - function comment?  Particularly since thread-safety in this class
isn't always apparent (some functions are safe to be called on any thread,
others only on the UI thread, etc).

[Charlie Harrison, 2016-11-16 22:04:04]

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

[Charlie Harrison, 2016-11-16 22:04:15]

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:160:
first_party_url.SchemeIs(kExtensionScheme)) {
On 2016/11/16 20:12:00, Devlin wrote:
> Also not your code, but a question for the cookies experts on this CL. :)
> 
> We auto-allow for extensions here - why do we not need to check the host
(i.e.,
> which extension it is)?  Does this allow unintentional cross-extension
> interaction?
> 
> (My cookie knowledge is pretty limited, so I'm probably way off base, but I'd
> like to understand it. :))

This strikes me as problematic. Welcome to ideas from cookie experts though

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:183:
!first_party_url.SchemeIs(extension_scheme_);
On 2016/11/16 20:12:00, Devlin wrote:
> Not your code, but.... why do we use extension_scheme_ here and
kExtensionScheme
> above?

I think we should move this to extension_scheme_ to allow dependency injection
via tests. I'm not sure it's strictly necessary though.

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.h (right):

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.h:60: void
GetReadingAndSettingCookieAllowed(const GURL& url,
On 2016/11/16 20:12:00, Devlin wrote:
> drive-by - function comment?  Particularly since thread-safety in this class
> isn't always apparent (some functions are safe to be called on any thread,
> others only on the UI thread, etc).

Done.

[commit-bot: I haz the power, 2016-11-16 22:05:34]

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

[mmenke, 2016-11-16 22:13:22]

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
File components/content_settings/core/browser/cookie_settings.cc (right):

https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
components/content_settings/core/browser/cookie_settings.cc:160:
first_party_url.SchemeIs(kExtensionScheme)) {
On 2016/11/16 22:04:14, Charlie Harrison wrote:
> On 2016/11/16 20:12:00, Devlin wrote:
> > Also not your code, but a question for the cookies experts on this CL. :)
> > 
> > We auto-allow for extensions here - why do we not need to check the host
> (i.e.,
> > which extension it is)?  Does this allow unintentional cross-extension
> > interaction?
> > 
> > (My cookie knowledge is pretty limited, so I'm probably way off base, but
I'd
> > like to understand it. :))
> 
> This strikes me as problematic. Welcome to ideas from cookie experts though

So anything set on an extensions URL is not, by definition, a real cookie.  They
use their own cookie store, and aren't set/read by URLRequests.  So I think you
actually need an extensions expert here.  :)

I guess this code is also used for document.cookie, which is when extensions
come into play?  I guess the root document of one extension can host another
extension in an iframe, and this lets the iframed extension use document.cookie?

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

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

[commit-bot: I haz the power, 2016-11-16 23:46:13]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-21 02:02:50]

On 2016/11/16 22:13:22, mmenke wrote:
>
https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
> File components/content_settings/core/browser/cookie_settings.cc (right):
> 
>
https://codereview.chromium.org/2502743003/diff/60001/components/content_sett...
> components/content_settings/core/browser/cookie_settings.cc:160:
> first_party_url.SchemeIs(kExtensionScheme)) {
> On 2016/11/16 22:04:14, Charlie Harrison wrote:
> > On 2016/11/16 20:12:00, Devlin wrote:
> > > Also not your code, but a question for the cookies experts on this CL. :)
> > > 
> > > We auto-allow for extensions here - why do we not need to check the host
> > (i.e.,
> > > which extension it is)?  Does this allow unintentional cross-extension
> > > interaction?
> > > 
> > > (My cookie knowledge is pretty limited, so I'm probably way off base, but
> I'd
> > > like to understand it. :))
> > 
> > This strikes me as problematic. Welcome to ideas from cookie experts though
> 
> So anything set on an extensions URL is not, by definition, a real cookie. 
They
> use their own cookie store, and aren't set/read by URLRequests.  So I think
you
> actually need an extensions expert here.  :)
> 
> I guess this code is also used for document.cookie, which is when extensions
> come into play?  I guess the root document of one extension can host another
> extension in an iframe, and this lets the iframed extension use
document.cookie?

Yeah I think iframed extensions modifying the parent is the concern here, but if
the damage is limited to document.cookie that seems relatively benign. Devlin
WDYT?

[Devlin, 2016-11-22 16:52:10]

On 2016/11/21 02:02:50, Charlie Harrison wrote:
> On 2016/11/16 22:13:22, mmenke wrote:
> > So anything set on an extensions URL is not, by definition, a real cookie. 
> They
> > use their own cookie store, and aren't set/read by URLRequests.  So I think
> you
> > actually need an extensions expert here.  :)

Do you happen to know where the code for that is?

> > I guess this code is also used for document.cookie, which is when extensions
> > come into play?  I guess the root document of one extension can host another
> > extension in an iframe, and this lets the iframed extension use
> document.cookie?
> 
> Yeah I think iframed extensions modifying the parent is the concern here, but
if
> the damage is limited to document.cookie that seems relatively benign. Devlin
> WDYT?

Yep, this is my concern - an extension context iframed by another extension. 
It's rare, but it can happen.

[mmenke, 2016-11-22 17:19:33]

On 2016/11/22 16:52:10, Devlin wrote:
> On 2016/11/21 02:02:50, Charlie Harrison wrote:
> > On 2016/11/16 22:13:22, mmenke wrote:
> > > So anything set on an extensions URL is not, by definition, a real cookie.

> > They
> > > use their own cookie store, and aren't set/read by URLRequests.  So I
think
> > you
> > > actually need an extensions expert here.  :)
> 
> Do you happen to know where the code for that is?

See callers of ContentBrowserClient::OverrideRequestContextForURL

> > > I guess this code is also used for document.cookie, which is when
extensions
> > > come into play?  I guess the root document of one extension can host
another
> > > extension in an iframe, and this lets the iframed extension use
> > document.cookie?
> > 
> > Yeah I think iframed extensions modifying the parent is the concern here,
but
> if
> > the damage is limited to document.cookie that seems relatively benign.
Devlin
> > WDYT?
> 
> Yep, this is my concern - an extension context iframed by another extension. 
> It's rare, but it can happen.

[Charlie Harrison, 2016-11-28 16:15:58]

I did a bit of code archaeology and found that the code exempting extensions
from third party blocking rules was landed in this CL:

https://codereview.chromium.org/8462003
with associated issue crbug.com/99588

[Devlin, 2016-11-28 17:06:18]

On 2016/11/28 16:15:58, Charlie Harrison(OOO Nov23-28) wrote:
> I did a bit of code archaeology and found that the code exempting extensions
> from third party blocking rules was landed in this CL:
> 
> https://codereview.chromium.org/8462003
> with associated issue crbug.com/99588

Hmm... the goal of that CL seems to be to ensure extension cookies are deleted
when other cookies are.  So I think that the byproduct of allowing cookie access
between extensions is probably unintended (and undesirable).  Seems like
something we should fix.

Note - there's no reason this has to hold up your CL, and probably should land
separately anyway since there's a chance stuff may break. :)

[Charlie Harrison, 2016-11-28 17:22:35]

On 2016/11/28 17:06:18, Devlin wrote:
> On 2016/11/28 16:15:58, Charlie Harrison(OOO Nov23-28) wrote:
> > I did a bit of code archaeology and found that the code exempting extensions
> > from third party blocking rules was landed in this CL:
> > 
> > https://codereview.chromium.org/8462003
> > with associated issue crbug.com/99588
> 
> Hmm... the goal of that CL seems to be to ensure extension cookies are deleted
> when other cookies are.  So I think that the byproduct of allowing cookie
access
> between extensions is probably unintended (and undesirable).  Seems like
> something we should fix.
SGTM, let's file a bug for this.
> 
> Note - there's no reason this has to hold up your CL, and probably should land
> separately anyway since there's a chance stuff may break. :)
Agreed, I'll land this now. Thanks everyone for the reviews and discussion.

[Charlie Harrison, 2016-11-28 17:23:09]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-28 17:23:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
msramek@chromium.org, mmenke@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2502743003/#ps80001
(title: "rdevlin review")

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

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-28 18:10:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480353789457000,
"parent_rev": "e2e5b9931e89b36236331d8ab845fe8ebb02f6d1", "commit_rev":
"1e34e8e21bede5eb19077695ca5032981aad3db9"}

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

Description was changed from

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174
==========

to

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-28 18:12:48]

Description was changed from

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174
==========

to

==========
Allow getting both reading/setting cookie settings at one time

Getting the underlying WebsiteSetting to find whether reading/setting
cookies is allowed is fairly expensive. The current cookie API requires
retrieving these settings twice if we want to know both values.

This patch updates the API to allow for querying reading and setting
policies with one call to GetWebsiteSettings. This is used in
net::NetworkDelegate::CanEnablePrivacyMode, which went from taking 18%
of the CPU time in net::URLRequestHttpJob::Start, to 12% after this
patch (Linux perf results).

BUG=664174

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

[commit-bot: I haz the power, 2016-11-28 18:12:49]

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