Pref service: support for incognito prefs

Pref service: support for incognito prefs

Works by adding another read-only pref store type, with lower priority than the
user store, that connects to the user store of the underlying profile.

preferences.mojom and preferences_configuration.mojom had to be merged as they
would cause a circular import otherwise.

BUG=654988

[tibell, 2017-05-03 04:25:56]

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

[commit-bot: I haz the power, 2017-05-03 04:26:12]

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, 2017-05-03 04:29:06]

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

[commit-bot: I haz the power, 2017-05-03 04:29:07]

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

[tibell, 2017-05-03 04:34:31]

Description was changed from

==========
Pref service: support for incognito prefs

BUG=654988
==========

to

==========
Pref service: support for incognito prefs

Works by adding another read-only pref store type, with lower priority than the
user store, that connects to the user store of the underlying profile.

preferences.mojom and preferences_configuration.mojom had to be merged as they
would cause a circular import otherwise.

BUG=654988
==========

[tibell, 2017-05-03 04:34:31]

tibell@chromium.org changed reviewers:
+ sammc@chromium.org

[tibell, 2017-05-03 05:24:58]

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

[commit-bot: I haz the power, 2017-05-03 05:25:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2017-05-03 05:25:55]

Description was changed from

==========
Pref service: support for incognito prefs

Works by adding another read-only pref store type, with lower priority than the
user store, that connects to the user store of the underlying profile.

preferences.mojom and preferences_configuration.mojom had to be merged as they
would cause a circular import otherwise.

BUG=654988
==========

to

==========
Pref service: support for incognito prefs

Works by adding another read-only pref store type, with lower priority than the
user store, that connects to the user store of the underlying profile.

preferences.mojom and preferences_configuration.mojom had to be merged as they
would cause a circular import otherwise.

BUG=654988
==========

[tibell, 2017-05-03 05:35:31]

I'm still adding a test but this is ready for a first look. The (WIP) CL that
uses this is http://crrev/2860683002.

[commit-bot: I haz the power, 2017-05-03 05:38:32]

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

[commit-bot: I haz the power, 2017-05-03 05:38:33]

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

[tibell, 2017-05-03 05:55:24]

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

[commit-bot: I haz the power, 2017-05-03 05:55:37]

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, 2017-05-03 06:36:47]

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

[commit-bot: I haz the power, 2017-05-03 06:36:48]

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

[tibell, 2017-05-03 07:13:39]

tibell@chromium.org changed reviewers:
+ bauerb@chromium.org, mbarbella@chromium.org

[tibell, 2017-05-03 07:13:40]

Hi!

This CL adds support for incognito to the new pref service. This will be used in
https://codereview.chromium.org/2860683002/, which is still WIP.

mbarbella@chromium.org: Please review
  - services/preferences/public/cpp/preferences_struct_traits.cc
  - services/preferences/public/interfaces/preferences.mojom
  - services/preferences/public/interfaces/preferences_configuration.mojom
This is just a move + adding one enum value.

bauerb@chromium.org: Please review
  chrome/browser/prefs/active_profile_pref_service.cc
  chrome/browser/prefs/active_profile_pref_service.h
  chrome/browser/prefs/profile_pref_store_manager.h
  components/prefs/pref_value_store.h
The active_profile_pref_service.* is a simple Mojo call forward (which is used
by ash). The pref_value_store.h change is a bit more interesting perhaps. The
way we're doing incognito in the new pref service is by slotting-in the user
pref store for the underlying profile as a read-only store (of type
INCOGNITO_STORE) in the incognito instance of the service. This makes sure, by
construction, that no writes can ever reach the underlying profile, without
having clients enforce this, which is the case today.

[tibell, 2017-05-03 07:20:36]

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

[commit-bot: I haz the power, 2017-05-03 07:20:48]

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, 2017-05-03 08:19:07]

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

[commit-bot: I haz the power, 2017-05-03 08:19:08]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-05-03 10:39:52]

bauerb@chromium.org changed reviewers:
+ battre@chromium.org, msramek@chromium.org

[Bernhard Bauer, 2017-05-03 10:39:52]

On 2017/05/03 07:13:40, tibell wrote:
> The pref_value_store.h change is a bit more interesting perhaps. The
> way we're doing incognito in the new pref service is by slotting-in the user
> pref store for the underlying profile as a read-only store (of type
> INCOGNITO_STORE) in the incognito instance of the service. This makes sure, by
> construction, that no writes can ever reach the underlying profile, without
> having clients enforce this, which is the case today.

Wait, that doesn't quite match the existing behavior for incognito, which is
that changes to user prefs are either written through to the underlying profile
or kept in memory, based on the pref key (and defaulting to write-through).
Adding Dominic and Martin who are more familiar with the requirements.

[tibell, 2017-05-03 20:21:32]

On 2017/05/03 10:39:52, Bernhard Bauer wrote:
> On 2017/05/03 07:13:40, tibell wrote:
> > The pref_value_store.h change is a bit more interesting perhaps. The
> > way we're doing incognito in the new pref service is by slotting-in the user
> > pref store for the underlying profile as a read-only store (of type
> > INCOGNITO_STORE) in the incognito instance of the service. This makes sure,
by
> > construction, that no writes can ever reach the underlying profile, without
> > having clients enforce this, which is the case today.
> 
> Wait, that doesn't quite match the existing behavior for incognito, which is
> that changes to user prefs are either written through to the underlying
profile
> or kept in memory, based on the pref key (and defaulting to write-through).
> Adding Dominic and Martin who are more familiar with the requirements.

It would be good to get clarification on this point.

I did instrument Chrome to crash (CHECK) if any writes actually hit the
underlying profile's persistent pref store and no manual testing or browser/unit
tests did cause a crash so it doesn't seem this functionality is used. It also
seems a bit suspect, because currently it's up to users of prefs to get this
right. I can see code failing to do that and leaking information from incognito
to the real profile.

[Martin Barbella, 2017-05-04 19:48:51]

mojom lgtm

[tibell, 2017-05-08 00:13:09]

Dominic and Martin could you please chime in here? As I mention below this
"write through to the real user profile" functionality doesn't seem to be used
(at least it's not tested). Do we need it? Ideally we could get rid of it and
make Chrome more secure at the same time.

On 2017/05/03 10:39:52, Bernhard Bauer wrote:
> On 2017/05/03 07:13:40, tibell wrote:
> > The pref_value_store.h change is a bit more interesting perhaps. The
> > way we're doing incognito in the new pref service is by slotting-in the user
> > pref store for the underlying profile as a read-only store (of type
> > INCOGNITO_STORE) in the incognito instance of the service. This makes sure,
by
> > construction, that no writes can ever reach the underlying profile, without
> > having clients enforce this, which is the case today.
> 
> Wait, that doesn't quite match the existing behavior for incognito, which is
> that changes to user prefs are either written through to the underlying
profile
> or kept in memory, based on the pref key (and defaulting to write-through).
> Adding Dominic and Martin who are more familiar with the requirements.

[Sam McNally, 2017-05-10 01:22:16]

https://codereview.chromium.org/2856083002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.cc (right):

https://codereview.chromium.org/2856083002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.cc:34: auto* connector =
content::BrowserContext::GetConnectorFor(
Add a helper for getting the prefs::mojom::PrefStoreConnector.

https://codereview.chromium.org/2856083002/diff/60001/components/prefs/pref_v...
File components/prefs/pref_value_store.h (right):

https://codereview.chromium.org/2856083002/diff/60001/components/prefs/pref_v...
components/prefs/pref_value_store.h:41: //   INCOGNITO_STORE contains
preferences that were written while in incognito
Isn't that USER_STORE when running in incognito? INCOGNITO_STORE in this
position would refer to the USER_STORE of the base profile.

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pe...
File services/preferences/persistent_pref_store_impl.cc (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pe...
services/preferences/persistent_pref_store_impl.cc:51: void OnConnectionError()
{ pref_store_->OnConnectionError(this); }
Methods before fields.

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:130: // This store is done
in-process so it's already "registered":
in-service

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:186:
incognito_connector_->ConnectToUserPrefStore(
Move this to Init() and remove |incognito_connector_|.

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.h (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pr...
services/preferences/pref_store_manager_impl.h:124:
ro_persistent_pref_store_bindings_;
Don't abbreviate. Also, why is this here instead of PersistentPrefStoreImpl?

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
services/preferences/public/interfaces/preferences.mojom:23: INCOGNITO,
Below USER.

[battre, 2017-05-10 11:03:38]

Apologies, I won't be able to look into this before I/O.

[Bernhard Bauer, 2017-05-11 14:34:33]

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
services/preferences/public/interfaces/preferences.mojom:23: INCOGNITO,
On 2017/05/10 01:22:16, Sam McNally wrote:
> Below USER.

I thought the idea was that this would be the store where pref writes end up,
and the user store below it is only to get copy-on-write semantics?

Another way might be to just get rid of the whitelisting in the existing
OverlayPrefStore (so it would essentially do the copy-on-write for all prefs)?

(Also, I'm sorry this is taking so long, but while I'm somewhat familiar with
the technical workings, I really don't own the feature anymore, so I'm hesistant
to just give a go-ahead here.)

[tibell, 2017-05-11 22:44:43]

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
services/preferences/public/interfaces/preferences.mojom:23: INCOGNITO,
On 2017/05/11 14:34:33, Bernhard Bauer wrote:
> On 2017/05/10 01:22:16, Sam McNally wrote:
> > Below USER.
> 
> I thought the idea was that this would be the store where pref writes end up,
> and the user store below it is only to get copy-on-write semantics?
> 
> Another way might be to just get rid of the whitelisting in the existing
> OverlayPrefStore (so it would essentially do the copy-on-write for all prefs)?
> 
> (Also, I'm sorry this is taking so long, but while I'm somewhat familiar with
> the technical workings, I really don't own the feature anymore, so I'm
hesistant
> to just give a go-ahead here.)

I should probably name this enum value better (UNDERLYING_USER perhaps, with a
comment explaining what that means). The intention was for this enum value to be
for the read-only view of the underlying profile's USER store and have the USER
store for the incognito profile be an in-memory read-write store.

Note that the current OverlayPrefStore is not a whitelist of prefs that may be
written to the underlying profile but a blacklist of prefs which are only stored
in-memory. This is one reason I want to get rid of the current thing. It makes
it very easy to write to the underlying profile by misstake.

[Bernhard Bauer, 2017-05-12 09:44:30]

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
File services/preferences/public/interfaces/preferences.mojom (right):

https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
services/preferences/public/interfaces/preferences.mojom:23: INCOGNITO,
On 2017/05/11 22:44:42, tibell wrote:
> On 2017/05/11 14:34:33, Bernhard Bauer wrote:
> > On 2017/05/10 01:22:16, Sam McNally wrote:
> > > Below USER.
> > 
> > I thought the idea was that this would be the store where pref writes end
up,
> > and the user store below it is only to get copy-on-write semantics?
> > 
> > Another way might be to just get rid of the whitelisting in the existing
> > OverlayPrefStore (so it would essentially do the copy-on-write for all
prefs)?
> > 
> > (Also, I'm sorry this is taking so long, but while I'm somewhat familiar
with
> > the technical workings, I really don't own the feature anymore, so I'm
> hesistant
> > to just give a go-ahead here.)
> 
> I should probably name this enum value better (UNDERLYING_USER perhaps, with a
> comment explaining what that means). The intention was for this enum value to
be
> for the read-only view of the underlying profile's USER store and have the
USER
> store for the incognito profile be an in-memory read-write store.
> 
> Note that the current OverlayPrefStore is not a whitelist of prefs that may be
> written to the underlying profile but a blacklist of prefs which are only
stored
> in-memory. This is one reason I want to get rid of the current thing. It makes
> it very easy to write to the underlying profile by misstake.

Right, what I meant was: assuming that the change in behavior is desired, could
we just change the default behavior to what you describe?

[tibell, 2017-05-25 04:03:31]

On 2017/05/12 09:44:30, Bernhard Bauer wrote:
>
https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
> File services/preferences/public/interfaces/preferences.mojom (right):
> 
>
https://codereview.chromium.org/2856083002/diff/60001/services/preferences/pu...
> services/preferences/public/interfaces/preferences.mojom:23: INCOGNITO,
> On 2017/05/11 22:44:42, tibell wrote:
> > On 2017/05/11 14:34:33, Bernhard Bauer wrote:
> > > On 2017/05/10 01:22:16, Sam McNally wrote:
> > > > Below USER.
> > > 
> > > I thought the idea was that this would be the store where pref writes end
> up,
> > > and the user store below it is only to get copy-on-write semantics?
> > > 
> > > Another way might be to just get rid of the whitelisting in the existing
> > > OverlayPrefStore (so it would essentially do the copy-on-write for all
> prefs)?
> > > 
> > > (Also, I'm sorry this is taking so long, but while I'm somewhat familiar
> with
> > > the technical workings, I really don't own the feature anymore, so I'm
> > hesistant
> > > to just give a go-ahead here.)
> > 
> > I should probably name this enum value better (UNDERLYING_USER perhaps, with
a
> > comment explaining what that means). The intention was for this enum value
to
> be
> > for the read-only view of the underlying profile's USER store and have the
> USER
> > store for the incognito profile be an in-memory read-write store.
> > 
> > Note that the current OverlayPrefStore is not a whitelist of prefs that may
be
> > written to the underlying profile but a blacklist of prefs which are only
> stored
> > in-memory. This is one reason I want to get rid of the current thing. It
makes
> > it very easy to write to the underlying profile by misstake.
> 
> Right, what I meant was: assuming that the change in behavior is desired,
could
> we just change the default behavior to what you describe?

I'm closing this in favor of https://chromium-review.googlesource.com/c/514902/
which doesn't try to change the current write-through behavior.