Ensure content settings aren't persisted in the guest profile

Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org

Committed: https://crrev.com/7963c649f581bcb3528670e4d91afe5ba5413e4d
Cr-Commit-Position: refs/heads/master@{#371992}

[raymes, 2016-01-19 03:19:38]

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

[raymes, 2016-01-19 03:20:20]

Description was changed from

==========
Ensure content settings aren't persisted in the guest profile

BUG=578130
==========

to

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
==========

[raymes, 2016-01-19 03:20:41]

Description was changed from

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
==========

to

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
==========

[msramek, 2016-01-19 13:35:08]

https://codereview.chromium.org/1605453003/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1605453003/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1165: 
Following the comment in host_content_settings_map.cc - please test that
potential changes in default settings are not persisted.

(although in reality, there should not be such changes, as the guest mode does
not offer UI to change the default settings; I wonder if we want to change that,
but it's not principially wrong - the initial default settings should be good
enough for a guest user)

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

https://codereview.chromium.org/1605453003/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:115: //
This ensures that content settings are cleraed for the guest profile. This
nit: cleared

https://codereview.chromium.org/1605453003/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:122: new
content_settings::DefaultProvider(prefs_, is_incognito_profile);
This should be |is_off_the_record_| (which will allow us to simplify the
constructor back to using just one boolean).

In the guest profile, it holds that |is_incognito_profile| == false, so here we
instantiate a regular DefaultProvider that writes into the pref. This means that
the default setting persists between guest sessions, which is not desired.

Note that in chrome://settings, most settings are hidden, which means that the
guest user should not touch them - and therefore, neither should
DefaultProvider.

Apparently it is possible to access chrome://settings/content directly by typing
the link, but that is a bug. I filed crbug.com/579052.

[raymes, 2016-01-19 23:31:39]

Thanks Martin!

https://codereview.chromium.org/1605453003/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1605453003/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1165: 
Agreed. This seems like a reasonable solution for now because otherwise we would
have to add more capabilities to the default provider.

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

https://codereview.chromium.org/1605453003/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:115: //
This ensures that content settings are cleraed for the guest profile. This
On 2016/01/19 13:35:07, msramek wrote:
> nit: cleared

Done.

https://codereview.chromium.org/1605453003/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:122: new
content_settings::DefaultProvider(prefs_, is_incognito_profile);
Ok - that sounds good. I wasn't sure of the better approach but this makes sense
to me.

I still need both bools in the constructor - see the line above which clears the
prefs only for a guest profile. If we do this in incognito we will delete the
underlying (inherited) prefs in the users profile.

[raymes, 2016-01-19 23:31:43]

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

[commit-bot: I haz the power, 2016-01-19 23:34:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/40001

[commit-bot: I haz the power, 2016-01-19 23:49:35]

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

[commit-bot: I haz the power, 2016-01-19 23:49:35]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[raymes, 2016-01-20 00:19:05]

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

[commit-bot: I haz the power, 2016-01-20 00:20:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/60001

[commit-bot: I haz the power, 2016-01-20 00:38:18]

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

[commit-bot: I haz the power, 2016-01-20 00:38:19]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[raymes, 2016-01-20 02:05:56]

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

[commit-bot: I haz the power, 2016-01-20 02:06:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/80001

[raymes, 2016-01-20 02:11:27]

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

[commit-bot: I haz the power, 2016-01-20 02:13:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/100001

[commit-bot: I haz the power, 2016-01-20 02:38:57]

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

[commit-bot: I haz the power, 2016-01-20 02:38:57]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-01-20 03:01:59]

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

[commit-bot: I haz the power, 2016-01-20 03:03:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/100001

[commit-bot: I haz the power, 2016-01-20 03:31:38]

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

[commit-bot: I haz the power, 2016-01-20 03:31:39]

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

[raymes, 2016-01-20 03:50:32]

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

[commit-bot: I haz the power, 2016-01-20 03:51:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/120001

[commit-bot: I haz the power, 2016-01-20 04:20:57]

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

[commit-bot: I haz the power, 2016-01-20 04:20:57]

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

[raymes, 2016-01-20 06:26:38]

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

[commit-bot: I haz the power, 2016-01-20 06:27:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/120001

[commit-bot: I haz the power, 2016-01-20 08:43:39]

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

[commit-bot: I haz the power, 2016-01-20 08:43:40]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-01-20 11:01:46]

LGTM % mostly nits.

Thanks a lot for fixing this!

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

https://codereview.chromium.org/1605453003/diff/20001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.cc:122: new
content_settings::DefaultProvider(prefs_, is_incognito_profile);
On 2016/01/19 23:31:39, raymes wrote:
> Ok - that sounds good. I wasn't sure of the better approach but this makes
sense
> to me.
> 
> I still need both bools in the constructor - see the line above which clears
the
> prefs only for a guest profile. If we do this in incognito we will delete the
> underlying (inherited) prefs in the users profile.

Acknowledged.

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1204:
ContentSettingsPattern pattern =
nit: pattern is unused

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter_unittest.cc:116: new
HostContentSettingsMap(profile_->GetPrefs(), false, false);
nit: please annotate the callsites as

"false /* incognito */, false /* guest */"

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:322:
ignore_result(value.release());
I think that the NOTREACHED() was not just supposed to check a leaked pointer,
but also to guarantee that at least one provider will accept the setting. I'd
prefer to keep it.

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:65: //
CalledOnValidThread() wrongly.
Please document that is_incognito_profile NAND is_guest_profile.

[raymes, 2016-01-20 22:55:42]

Thanks Martin! Please see the note about the NOTREACHED.

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1204:
ContentSettingsPattern pattern =
On 2016/01/20 11:01:46, msramek wrote:
> nit: pattern is unused

Done.

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/1605453003/diff/120001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter_unittest.cc:116: new
HostContentSettingsMap(profile_->GetPrefs(), false, false);
On 2016/01/20 11:01:46, msramek wrote:
> nit: please annotate the callsites as
> 
> "false /* incognito */, false /* guest */"

Done.

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:322:
ignore_result(value.release());
I had to remove it though because in the case of the default setting for the
guest case, none of the providers handle it.

Perhaps we should return true in DefaultProvider::SetWebsiteSetting? I've
changed the code to do that instead and keep the NOTREACHED.

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:65: //
CalledOnValidThread() wrongly.
On 2016/01/20 11:01:46, msramek wrote:
> Please document that is_incognito_profile NAND is_guest_profile.

Done.

[raymes, 2016-01-20 22:55:49]

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

[commit-bot: I haz the power, 2016-01-20 22:57:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/160001

[commit-bot: I haz the power, 2016-01-20 23:45:58]

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

[commit-bot: I haz the power, 2016-01-20 23:45:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[raymes, 2016-01-21 02:00:40]

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

[commit-bot: I haz the power, 2016-01-21 02:01:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/180001

[commit-bot: I haz the power, 2016-01-21 02:47:05]

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

[commit-bot: I haz the power, 2016-01-21 02:47:06]

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

[msramek, 2016-01-21 10:37:30]

Still LGTM.

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1605453003/diff/120001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:322:
ignore_result(value.release());
On 2016/01/20 22:55:42, raymes wrote:
> I had to remove it though because in the case of the default setting for the
> guest case, none of the providers handle it.
> 
> Perhaps we should return true in DefaultProvider::SetWebsiteSetting? I've
> changed the code to do that instead and keep the NOTREACHED. 

Interesting. So HostContentSettingsMap::SetWebsiteSetting(wildcard, wildcard)
would also crash if called in incognito. Which, of course, wasn't a problem,
since it wasn't possible to set default settings in the UI in incognito.

I agree with this solution. DefaultProvider returns true to acknowledge that it
knows how to handle the value (even if doesn't actually do anything).

In fact, I would say that this logic isn't really needed anymore. All providers
except Pref and Default always return false (since they read their settings from
elsewhere). And for these two, we know that Pref will always accept non-default
settings and Default will always accept default settings. (Just an observation,
I'm not asking you to fix this :) )

[raymes, 2016-01-28 02:25:25]

Description was changed from

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
==========

to

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org
==========

[raymes, 2016-01-28 02:28:34]

TBR owners for API changes:
asanka@chromium.org -
chrome/browser/download/download_request_limiter_unittest.cc
bzanotti@chromium.org -
	components/signin/ios/browser/account_consistency_service_unittest.mm
droger@chromium.org -
ios/chrome/browser/content_settings/host_content_settings_map_factory.cc

[raymes, 2016-01-28 02:28:53]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-01-28 02:28:53]

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

[commit-bot: I haz the power, 2016-01-28 02:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1605453003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1605453003/200001

[commit-bot: I haz the power, 2016-01-28 03:28:27]

Description was changed from

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org
==========

to

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org
==========

[commit-bot: I haz the power, 2016-01-28 03:28:28]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-01-28 03:29:19]

Description was changed from

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org
==========

to

==========
Ensure content settings aren't persisted in the guest profile

We used to incorrectly allow content settings to be persisted in the guest
profile. This CL prevents them from being persisted and ensures that any
existing settings are deleted.

BUG=578130
TBR=asanka@chromium.org,bzanotti@chromium.org,droger@chromium.org

Committed: https://crrev.com/7963c649f581bcb3528670e4d91afe5ba5413e4d
Cr-Commit-Position: refs/heads/master@{#371992}
==========

[commit-bot: I haz the power, 2016-01-28 03:29:20]

Patchset 11 (id:??) landed as
https://crrev.com/7963c649f581bcb3528670e4d91afe5ba5413e4d
Cr-Commit-Position: refs/heads/master@{#371992}