Fix homepage migration for users who never changed their settings.

Fix homepage migration for users who never changed their settings.


BUG=122936
TEST=Upgrade from 18 to 19
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133740

[Tyler Breisacher (Chromium), 2012-04-11 22:18:23]

Follow-up to my favorite CL ever :)

[Ivan Korotkov, 2012-04-12 19:44:45]

Can you please ensure that
tests/functional/protector.py:testSessionStartupPrefMigration passes and
(ideally) add a similar test case for the setup this CL is fixing?

[Tyler Breisacher (Chromium), 2012-04-12 23:07:16]

In case anyone had already started looking at it, the previous patch didn't
*quite* work because if a user installs m19, then changes their homepage without
changing their startup prefs, we would migrate their startup prefs. Patch set 4
fixes this.

On 2012/04/12 19:44:45, Ivan Korotkov wrote:
> Can you please ensure that
> tests/functional/protector.py:testSessionStartupPrefMigration passes and
> (ideally) add a similar test case for the setup this CL is fixing?

I started doing this and that led me to discover the issue above. :) Working on
it...

[Tyler Breisacher (Chromium), 2012-04-13 00:03:54]

I added a test for the case I just described, but I'm not sure how to test the
case where we actually *do* want to migrate, without actually loading an m18
build (which is what I did to test it manually). We want to modify the
preferences so that they look like they came from an m18 user
(kRestoreOnStartup=blank, kHomePageIsNewTabPage=false), and as far as I can see,
pyauto doesn't give us a way of doing that. We can set kHomePageIsNewTabPage to
false, but kRestoreOnStartup is set explicitly at session_startup_pref.cc line
156 in the new code, so it's not blank. Any ideas? Do we need to add a ClearPref
method to pyauto?

[Ivan Korotkov, 2012-04-13 08:18:28]

Why not modify Preferences directly outside of Chrome in between browser starts,
like testSessionStartupPrefMigrationFromHomepage does it (see pre_launch_hook)?

[Tyler Breisacher (Chromium), 2012-04-13 18:03:11]

On 2012/04/13 08:18:28, Ivan Korotkov wrote:
> Why not modify Preferences directly outside of Chrome in between browser
starts,
> like testSessionStartupPrefMigrationFromHomepage does it (see
pre_launch_hook)?

That's perfect, thank you! I didn't realize that method was modifying the
Preferences file directly, rather than communicating with the Chrome process and
having the PrefService make the changes.

[Tyler Breisacher (Chromium), 2012-04-13 21:57:43]

+nirnimesh for new functional tests
+mnissler for chrome/browser/prefs OWNER review

[Tyler Breisacher (Chromium), 2012-04-13 21:58:28]

Oops, +mnissler for reals now.

[csilv, 2012-04-13 22:19:03]

lgtm

[Nirnimesh, 2012-04-14 08:09:54]

chrome/test/functional LGTM

http://codereview.chromium.org/10049005/diff/16001/chrome/test/functional/pro...
File chrome/test/functional/protector.py (right):

http://codereview.chromium.org/10049005/diff/16001/chrome/test/functional/pro...
chrome/test/functional/protector.py:497:
self.GetPrefsInfo().Prefs(pyauto.kURLsToRestoreOnStartup))
use assertFalse instead

[Mattias Nissler (ping if slow), 2012-04-16 09:21:46]

http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
File chrome/browser/prefs/session_startup_pref.cc (right):

http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
chrome/browser/prefs/session_startup_pref.cc:148:
!prefs->GetBoolean(prefs::kHomePageIsNewTabPage))) {
This is likely to break in case there is policy present that affects these
prefs. For example, if policy forces prefs::kRestoreOnStartup to HOMEPAGE,
you'll run this migration code every time, because the
SetInteger(prefs::kRestoreOnStartup) below will be masked by policy.

http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
chrome/browser/prefs/session_startup_pref.cc:152: } else if
(!prefs->HasPrefPath(prefs::kRestoreOnStartup)) {
Setting a pref to the default is bad. This breaks situations where the user
hasn't made any decision and the admin wants to change the default through
recommended policy (which is lower priority than user prefs, so your forced
default will mask the admin-provided default).

[Tyler Breisacher (Chromium), 2012-04-16 16:38:17]

On 2012/04/16 09:21:46, Mattias Nissler wrote:
>
http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> File chrome/browser/prefs/session_startup_pref.cc (right):
> 
>
http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> chrome/browser/prefs/session_startup_pref.cc:148:
> !prefs->GetBoolean(prefs::kHomePageIsNewTabPage))) {
> This is likely to break in case there is policy present that affects these
> prefs. For example, if policy forces prefs::kRestoreOnStartup to HOMEPAGE,
> you'll run this migration code every time, because the
> SetInteger(prefs::kRestoreOnStartup) below will be masked by policy.

I think (though I'm not sure) that this is okay, because the UI is set up to
deal with that situation and show the right thing.

http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> chrome/browser/prefs/session_startup_pref.cc:152: } else if
> (!prefs->HasPrefPath(prefs::kRestoreOnStartup)) {
> Setting a pref to the default is bad. This breaks situations where the user
> hasn't made any decision and the admin wants to change the default through
> recommended policy (which is lower priority than user prefs, so your forced
> default will mask the admin-provided default).

There might be a way to deal with this too, but I'm thinking this code is
getting too tricky and fragile already, so I'm going to try and step back and
think of a simpler approach. Some ideas:

1. Do we record anywhere in the profile what the last-launched version of Chrome
was? If so, we can just say "If the previous launch was 18 or lower, check
whether we need to do this migration, and if so, do it." But AFAIK nothing like
that exists.

2. After we have checked whether we need to do the migration (whether we
actually did it or not), we can set a preference like
"session.restore_on_startup.migrated". Then, if that preference is already set,
we skip the migration. The only disadvantage I see here would be that it might
give someone a way to get around the protector -- if the user changes their
homepage and then a third party switches "session.restore_on_startup.migrated"
then we'll be tricked into running the migration again. ivankr@, if we add
"session.restore_on_startup.migrated" to the list of protected preferences, does
that solve the problem?

[Tyler Breisacher (Chromium), 2012-04-16 16:38:18]

On 2012/04/16 09:21:46, Mattias Nissler wrote:
>
http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> File chrome/browser/prefs/session_startup_pref.cc (right):
> 
>
http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> chrome/browser/prefs/session_startup_pref.cc:148:
> !prefs->GetBoolean(prefs::kHomePageIsNewTabPage))) {
> This is likely to break in case there is policy present that affects these
> prefs. For example, if policy forces prefs::kRestoreOnStartup to HOMEPAGE,
> you'll run this migration code every time, because the
> SetInteger(prefs::kRestoreOnStartup) below will be masked by policy.

I think (though I'm not sure) that this is okay, because the UI is set up to
deal with that situation and show the right thing.

http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/sessi...
> chrome/browser/prefs/session_startup_pref.cc:152: } else if
> (!prefs->HasPrefPath(prefs::kRestoreOnStartup)) {
> Setting a pref to the default is bad. This breaks situations where the user
> hasn't made any decision and the admin wants to change the default through
> recommended policy (which is lower priority than user prefs, so your forced
> default will mask the admin-provided default).

There might be a way to deal with this too, but I'm thinking this code is
getting too tricky and fragile already, so I'm going to try and step back and
think of a simpler approach. Some ideas:

1. Do we record anywhere in the profile what the last-launched version of Chrome
was? If so, we can just say "If the previous launch was 18 or lower, check
whether we need to do this migration, and if so, do it." But AFAIK nothing like
that exists.

2. After we have checked whether we need to do the migration (whether we
actually did it or not), we can set a preference like
"session.restore_on_startup.migrated". Then, if that preference is already set,
we skip the migration. The only disadvantage I see here would be that it might
give someone a way to get around the protector -- if the user changes their
homepage and then a third party switches "session.restore_on_startup.migrated"
then we'll be tricked into running the migration again. ivankr@, if we add
"session.restore_on_startup.migrated" to the list of protected preferences, does
that solve the problem?

[Mattias Nissler (ping if slow), 2012-04-16 17:00:41]

On Mon, Apr 16, 2012 at 6:38 PM, <tbreisacher@chromium.org> wrote:

> On 2012/04/16 09:21:46, Mattias Nissler wrote:
>
> http://codereview.chromium.**org/10049005/diff/16001/**
>
chrome/browser/prefs/session_**startup_pref.cc<http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/session_startup_pref.cc>
>
>> File chrome/browser/prefs/session_**startup_pref.cc (right):
>>
>
>
> http://codereview.chromium.**org/10049005/diff/16001/**
>
chrome/browser/prefs/session_**startup_pref.cc#newcode148<http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/session_startup_pref.cc#newcode148>
>
>> chrome/browser/prefs/session_**startup_pref.cc:148:
>> !prefs->GetBoolean(prefs::**kHomePageIsNewTabPage))) {
>> This is likely to break in case there is policy present that affects these
>> prefs. For example, if policy forces prefs::kRestoreOnStartup to HOMEPAGE,
>> you'll run this migration code every time, because the
>> SetInteger(prefs::**kRestoreOnStartup) below will be masked by policy.
>>
>
> I think (though I'm not sure) that this is okay, because the UI is set up
> to
> deal with that situation and show the right thing.


Right. But there's the other issue that the pref reads give you
policy-supplied values and you migrate them to the user pref, which is
obviously bad in case the policy goes away, at which we'll start to sync
the policy value... So you either want to migrate only if the user value is
effective, or make sure you migrate the user value. Note that you there's
currently no good way to ask PrefService for the user value even if that's
masked, but we could add that relatively easily.


>
>
> http://codereview.chromium.**org/10049005/diff/16001/**
>
chrome/browser/prefs/session_**startup_pref.cc#newcode152<http://codereview.chromium.org/10049005/diff/16001/chrome/browser/prefs/session_startup_pref.cc#newcode152>
>
>> chrome/browser/prefs/session_**startup_pref.cc:152: } else if
>> (!prefs->HasPrefPath(prefs::**kRestoreOnStartup)) {
>> Setting a pref to the default is bad. This breaks situations where the
>> user
>> hasn't made any decision and the admin wants to change the default through
>> recommended policy (which is lower priority than user prefs, so your
>> forced
>> default will mask the admin-provided default).
>>
>
> There might be a way to deal with this too, but I'm thinking this code is
> getting too tricky and fragile already, so I'm going to try and step back
> and
> think of a simpler approach. Some ideas:
>
> 1. Do we record anywhere in the profile what the last-launched version of
> Chrome
> was? If so, we can just say "If the previous launch was 18 or lower, check
> whether we need to do this migration, and if so, do it." But AFAIK nothing
> like
> that exists.
>

I'm not aware of anything like this.


>
> 2. After we have checked whether we need to do the migration (whether we
> actually did it or not), we can set a preference like
> "session.restore_on_startup.**migrated". Then, if that preference is
> already set,
> we skip the migration. The only disadvantage I see here would be that it
> might
> give someone a way to get around the protector -- if the user changes their
> homepage and then a third party switches "session.restore_on_startup.**
> migrated"
> then we'll be tricked into running the migration again. ivankr@, if we add
> "session.restore_on_startup.**migrated" to the list of protected
> preferences, does
> that solve the problem?
>

I've seen this approach a couple times before, so that should be good.
Another proven way of doing migrations is to deliberately create a new pref
to migrate to and use the existence of the old pref as a signal that
migration needs to be done. Obviously, after the migration the old pref
would be cleared.


>
http://codereview.chromium.**org/10049005/<http://codereview.chromium.org/100...
>

[Tyler Breisacher (Chromium), 2012-04-16 21:43:40]

I added a new pref, session.restore_on_startup_migrated, as I suggested. I
haven't made any changes to the protector yet, but csilv and mnissler, please
take a look and see if you approve of the general approach.

[Ivan Korotkov, 2012-04-17 09:27:51]

LGTM from protector side

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:138: if
(!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
Could you move the migration to a separate function and then call it from
ProtectedPrefsWatcher::EnsurePrefsMigration() instead of GetStartupPref()?

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
File chrome/test/functional/protector.py (right):

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
chrome/test/functional/protector.py:481: """Test migration from
session.restore_on_startup being blank, as it would
Please align pydoc as in the rest of the file, i.e.
"""blah
blah
"""

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
chrome/test/functional/protector.py:528: # Now the new preference values are
active.
Please add assert that protector isn't showing a bubble.

[Mattias Nissler (ping if slow), 2012-04-17 09:54:48]

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:147: if
(prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {
These values may still come from policy, in which case you write policy-supplied
data to the user prefs. Bad. You probably want another check on whether the
stuff you migrate is actually coming from user prefs. Alternatively, add code to
pull actual user prefs from the PrefService regardless of whether the effective
pref value comes from some other PrefStore.

[Tyler Breisacher (Chromium), 2012-04-17 19:54:33]

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:138: if
(!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
On 2012/04/17 09:27:51, Ivan Korotkov wrote:
> Could you move the migration to a separate function and then call it from
> ProtectedPrefsWatcher::EnsurePrefsMigration() instead of GetStartupPref()?

Done.

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:147: if
(prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {
On 2012/04/17 09:54:48, Mattias Nissler wrote:
> These values may still come from policy, in which case you write
policy-supplied
> data to the user prefs. Bad. You probably want another check on whether the
> stuff you migrate is actually coming from user prefs. Alternatively, add code
to
> pull actual user prefs from the PrefService regardless of whether the
effective
> pref value comes from some other PrefStore.

Done.

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
File chrome/test/functional/protector.py (right):

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
chrome/test/functional/protector.py:481: """Test migration from
session.restore_on_startup being blank, as it would
On 2012/04/17 09:27:51, Ivan Korotkov wrote:
> Please align pydoc as in the rest of the file, i.e.
> """blah
> blah
> """

Done.

https://chromiumcodereview.appspot.com/10049005/diff/27001/chrome/test/functi...
chrome/test/functional/protector.py:528: # Now the new preference values are
active.
On 2012/04/17 09:27:51, Ivan Korotkov wrote:
> Please add assert that protector isn't showing a bubble.

Done.

[Mattias Nissler (ping if slow), 2012-04-18 11:40:54]

Sorry to keep bashing on this. Let's take a step back and see what the
constraints here really are:

1. We should only migrate if there are previously existing  pref values.
2. We want to migrate prefs that belong to the user (not injected by policy or
anything else). So we need to make sure that the values we get are not coming
from another source, as we'd inadvertently leak them to user prefs when writing
back.

I think the easiest way of doing this is adding another GetUserPrefValue()
member function to PrefService that allows us to read user pref values. This is
similar to the existing GetMutableUserPrefs but with no special-casing on the
pref type. Something like this should do the trick:

const Value* PrefService::GetUserPrefValue(const char* path) const {
  DCHECK(CalledOnValidThread());

  const Preference* pref = FindPreference(path);
  if (!pref) {
    NOTREACHED() << "Trying to get an unregistered pref: " << path;
    return NULL;
  }

  // Look for an existing preference in the user store. If it doesn't
  // exist, return NULL.
  Value* value = NULL;
  if (user_pref_store_->GetMutableValue(path, &value)
          != PersistentPrefStore::READ_OK) {
    return NULL;
  }

  if (!value->IsType(pref->GetType())) {
    NOTREACHED() << "Pref value type doesn't match registered type.";
    return NULL;
  }

  return value;
}

Then you can write your migration logic as:

void SessionStartupPref::MigrateIfNecessary(PrefService* prefs) {
  DCHECK(prefs);

  if (!prefs->GetBoolean(prefs::kRestoreOnStartupMigrated)) {
    // Read existing values.
    Value* hompage_is_new_tab_page_value =
        prefs->GetUserPrefValue(prefs::kHomePageIsNewTabPage);
    bool homepage_is_new_tab_page = true;
    if (homepage_is_new_tab_page_value)
      homepage_is_new_tab_page_value->GetAsBoolean(&hompage_is_new_tab_page);

    Value* restore_on_startup_value =
        prefs->GetUserPrefValue(prefs::kRestoreOnStartup);
    int restore_on_startup = -1;
    if (restore_on_startup_value)
      restore_on_startup_value->GetAsInteger(&restore_on_startup);

    // Migrate if kRestoreOnStartup has the deprecated kPrefValueHomePage set,
    // or... (I don't really understand the reasoning behind your logic here,
    // you probably want to add a comment explaining this).
    if ((restore_on_startup_value &&
         restore_on_startup == kPrefValueHomePage) ||
        (!restore_on_startup_value && !home_page_is_new_tab_page) {

      if (home_page_is_new_tab_page) {
        prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueNewTab);
      } else {
        prefs->SetInteger(prefs::kRestoreOnStartup, kPrefValueURLs);
        SetNewURLList(prefs);
      }
    }

    prefs->SetBoolean(prefs::kRestoreOnStartupMigrated, true);
  }
}

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:147: // URLS was selected.
We should change RestoreOnStartupPolicyHandler in
configuration_policy_handler.cc to translate policy to the correct pref then. OK
to do in a follow-up CL though. Can you file a bug and put a reference here?

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:169: if
(prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {
This still breaks if prefs::kHomePageIsNewTabPage is under policy control, in
which case you write the policy-enforced value to user prefs.

[Tyler Breisacher (Chromium), 2012-04-18 18:26:03]

On 2012/04/18 11:40:54, Mattias Nissler wrote:
> Sorry to keep bashing on this.

Not at all! Sorry it's taking me so long to figure all this out.

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:147: // URLS was selected.
On 2012/04/18 11:40:54, Mattias Nissler wrote:
> We should change RestoreOnStartupPolicyHandler in
> configuration_policy_handler.cc to translate policy to the correct pref then.
OK
> to do in a follow-up CL though. Can you file a bug and put a reference here?

http://crbug.com/124027

The patch I just uploaded, I wrote as though that had already been fixed. This
makes it a lot easier to write, of course, but if the "RestoreOnStartup" policy
is set to 0, we hit a NOTREACHED() in browser_init.cc

So it might be better to fix the policy-preference mapping first, and then come
back to this CL.

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:169: if
(prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {
On 2012/04/18 11:40:54, Mattias Nissler wrote:
> This still breaks if prefs::kHomePageIsNewTabPage is under policy control, in
> which case you write the policy-enforced value to user prefs.

I don't think we change the value of kHomePageIsNewTabPage during the migration.

[Mattias Nissler (ping if slow), 2012-04-19 13:23:17]

Thanks for bearing with me, LGTM!

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:147: // URLS was selected.
On 2012/04/18 18:26:03, Tyler Breisacher wrote:
> On 2012/04/18 11:40:54, Mattias Nissler wrote:
> > We should change RestoreOnStartupPolicyHandler in
> > configuration_policy_handler.cc to translate policy to the correct pref
then.
> OK
> > to do in a follow-up CL though. Can you file a bug and put a reference here?
> 
> <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/issue.png);
">http://crbug.com/124027</crxref>
> 
> The patch I just uploaded, I wrote as though that had already been fixed. This
> makes it a lot easier to write, of course, but if the "RestoreOnStartup"
policy
> is set to 0, we hit a NOTREACHED() in browser_init.cc
> 
> So it might be better to fix the policy-preference mapping first, and then
come
> back to this CL.

Thanks for fixing the policy!

https://chromiumcodereview.appspot.com/10049005/diff/34001/chrome/browser/pre...
chrome/browser/prefs/session_startup_pref.cc:169: if
(prefs->GetBoolean(prefs::kHomePageIsNewTabPage)) {
On 2012/04/18 18:26:03, Tyler Breisacher wrote:
> On 2012/04/18 11:40:54, Mattias Nissler wrote:
> > This still breaks if prefs::kHomePageIsNewTabPage is under policy control,
in
> > which case you write the policy-enforced value to user prefs.
> 
> I don't think we change the value of kHomePageIsNewTabPage during the
migration.

What I meant was that the pref value you get in line 169 could actually be not
what the user configured, but what policy enforced. In that case you make a
wrong decision, since you're really meaning to migrate user choice, and not
policy-adjusted user choice.

[Tyler Breisacher (Chromium), 2012-04-24 00:21:17]

This is now rebased on top of the CL that changes the way we handle the
RestoreOnStartup policy, so a few changes from that (and other CLs) are showing
up in the diffs.

[Mattias Nissler (ping if slow), 2012-04-24 07:23:59]

LGTM

http://codereview.chromium.org/10049005/diff/43001/chrome/browser/prefs/sessi...
File chrome/browser/prefs/session_startup_pref_unittest.cc (right):

http://codereview.chromium.org/10049005/diff/43001/chrome/browser/prefs/sessi...
chrome/browser/prefs/session_startup_pref_unittest.cc:63: // their preferences
are migrated on upgrade to m19.
nit: M19 (also below)