Use new sync backup DB if settings are reset to avoid undoing reset.

Use new sync backup DB if settings are reset to avoid undoing reset from backup data, e.g. reinstall removed extensions, etc.


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

[gab, 2014-07-03 13:08:53]

lgtm w/ one request

Would also be nice to have a test for this.

Thanks!
Gab

https://codereview.chromium.org/367063005/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/367063005/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:524: <<
PrefHashFilter::GetResetTime(profile_->GetPrefs()).ToInternalValue();
Please use chrome_prefs::GetResetTime(Profile*) from
chrome_pref_service_factory.h here and below.

[haitaol1, 2014-07-07 20:43:19]

https://codereview.chromium.org/367063005/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/367063005/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:524: <<
PrefHashFilter::GetResetTime(profile_->GetPrefs()).ToInternalValue();
On 2014/07/03 13:08:53, gab wrote:
> Please use chrome_prefs::GetResetTime(Profile*) from
> chrome_pref_service_factory.h here and below.

Done.

[gab, 2014-07-07 20:56:12]

Would ProfileSyncServiceStartupTest be well-suited to add a test for this?
(e.g., to catch a regression where the relation between reset_time and
ProfileImpl::start_time_ changes)

https://codereview.chromium.org/367063005/diff/40001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/367063005/diff/40001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:533: profile_->GetStartTime() <=
chrome_prefs::GetResetTime(profile_)) {
Technically this will work if chrome_prefs::GetResetTime returns a NULL time as
it's internally represented as 0, but feels like a !is_null() check would be
safer (i.e., no need to even compare if there is no reset time).

[haitaol1, 2014-07-07 21:09:15]

On 2014/07/07 20:56:12, gab wrote:
> Would ProfileSyncServiceStartupTest be well-suited to add a test for this?
> (e.g., to catch a regression where the relation between reset_time and
> ProfileImpl::start_time_ changes)

Is there a help method to introduce tampering for test? If possible, I'd like to
add browser test too to make sure it works end-to-end.

> 
>
https://codereview.chromium.org/367063005/diff/40001/chrome/browser/sync/prof...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://codereview.chromium.org/367063005/diff/40001/chrome/browser/sync/prof...
> chrome/browser/sync/profile_sync_service.cc:533: profile_->GetStartTime() <=
> chrome_prefs::GetResetTime(profile_)) {
> Technically this will work if chrome_prefs::GetResetTime returns a NULL time
as
> it's internally represented as 0, but feels like a !is_null() check would be
> safer (i.e., no need to even compare if there is no reset time).

done

[gab, 2014-07-07 21:22:28]

On 2014/07/07 21:09:15, haitaol1 wrote:
> On 2014/07/07 20:56:12, gab wrote:
> > Would ProfileSyncServiceStartupTest be well-suited to add a test for this?
> > (e.g., to catch a regression where the relation between reset_time and
> > ProfileImpl::start_time_ changes)
> 
> Is there a help method to introduce tampering for test? If possible, I'd like
to
> add browser test too to make sure it works end-to-end.

There used to be an end-to-end test but it was removed in a recent refactoring,
I'm going to bring it back soon, but you probably don't want to depend on this
(especially since you'll need to merge upstream).

In a browser_test though I guess you can have your test set the
kPreferencesResetTime pref under-the-hood vs Profile::GetResetTime() without
actually going through a real reset for now.

If you want to test under a real reset you'll probably need a PRE_ test to init
your Secure Preferences file; then tamper with the Secure Preferences file
between the PRE_ test and the main test.

[haitaol1, 2014-07-07 23:40:44]

On 2014/07/07 21:22:28, gab wrote:
> On 2014/07/07 21:09:15, haitaol1 wrote:
> > On 2014/07/07 20:56:12, gab wrote:
> > > Would ProfileSyncServiceStartupTest be well-suited to add a test for this?
> > > (e.g., to catch a regression where the relation between reset_time and
> > > ProfileImpl::start_time_ changes)
> > 
> > Is there a help method to introduce tampering for test? If possible, I'd
like
> to
> > add browser test too to make sure it works end-to-end.
> 
> There used to be an end-to-end test but it was removed in a recent
refactoring,
> I'm going to bring it back soon, but you probably don't want to depend on this
> (especially since you'll need to merge upstream).
> 
> In a browser_test though I guess you can have your test set the
> kPreferencesResetTime pref under-the-hood vs Profile::GetResetTime() without
> actually going through a real reset for now.
> 
> If you want to test under a real reset you'll probably need a PRE_ test to
init
> your Secure Preferences file; then tamper with the Secure Preferences file
> between the PRE_ test and the main test.

Added unit test. PTAL.

[gab, 2014-07-08 14:27:06]

lgtm w/ nits

Thanks!
Gab

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:606: FROM_HERE, 
base::Bind(&QuitLoop),
nit: one space too many after "FROM_HERE,"

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:606: FROM_HERE, 
base::Bind(&QuitLoop),
Instead of base::Bind(&QuitLoop) these tests should use RunLoop::QuitClosure()
-- which is essentially the same but avoids rolling your own quit closure.

[haitaol1, 2014-07-08 20:31:40]

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:606: FROM_HERE, 
base::Bind(&QuitLoop),
On 2014/07/08 14:27:06, gab wrote:
> Instead of base::Bind(&QuitLoop) these tests should use RunLoop::QuitClosure()
> -- which is essentially the same but avoids rolling your own quit closure.

Done.

https://codereview.chromium.org/367063005/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_unittest.cc:606: FROM_HERE, 
base::Bind(&QuitLoop),
On 2014/07/08 14:27:06, gab wrote:
> nit: one space too many after "FROM_HERE,"

Done.

[haitaol1, 2014-07-09 15:45:30]

Nicolas: can you take a look?

[rlarocque, 2014-07-10 18:37:36]

lgtm

[haitaol1, 2014-07-10 19:01:25]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-07-10 19:02:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/367063005/100001

[commit-bot: I haz the power, 2014-07-10 20:05:31]

Change committed as 282409

[gab, 2014-07-11 20:15:52]

Hey Haitao, I just realized that Time::Now() is not guaranteed to be
monolithically increasing... this could be problematic with the check used in
this CL.

I haven't looked at the impl in details to understand when that is, would be
worth figuring out, perhaps it's only if the user changes his system clock which
would then be okay...

Cheers,
Gab

[gab, 2014-07-11 20:16:03]

On 2014/07/11 20:15:52, gab wrote:
> Hey Haitao, I just realized that Time::Now() is not guaranteed to be
> monolithically increasing... this could be problematic with the check used in
> this CL.

https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...

> 
> I haven't looked at the impl in details to understand when that is, would be
> worth figuring out, perhaps it's only if the user changes his system clock
which
> would then be okay...
> 
> Cheers,
> Gab

[haitaol1, 2014-07-11 21:05:20]

On 2014/07/11 20:16:03, gab wrote:
> On 2014/07/11 20:15:52, gab wrote:
> > Hey Haitao, I just realized that Time::Now() is not guaranteed to be
> > monolithically increasing... this could be problematic with the check used
in
> > this CL.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...
> 
> > 
> > I haven't looked at the impl in details to understand when that is, would be
> > worth figuring out, perhaps it's only if the user changes his system clock
> which
> > would then be okay...
> > 
> > Cheers,
> > Gab

That's true. The previous saved reset time could be after profile start time. In
that case, sync backup db is erased unnecessarily but it's still correct.
Because the window between profile start and pref check is very small, I think
the chance that clock goes back in that window is very small.

[gab, 2014-07-14 15:21:47]

On 2014/07/11 21:05:20, haitaol1 wrote:
> On 2014/07/11 20:16:03, gab wrote:
> > On 2014/07/11 20:15:52, gab wrote:
> > > Hey Haitao, I just realized that Time::Now() is not guaranteed to be
> > > monolithically increasing... this could be problematic with the check used
> in
> > > this CL.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...
> > 
> > > 
> > > I haven't looked at the impl in details to understand when that is, would
be
> > > worth figuring out, perhaps it's only if the user changes his system clock
> > which
> > > would then be okay...
> > > 
> > > Cheers,
> > > Gab
> 
> That's true. The previous saved reset time could be after profile start time.
In
> that case, sync backup db is erased unnecessarily but it's still correct.
> Because the window between profile start and pref check is very small, I think
> the chance that clock goes back in that window is very small.

Okay sgtm. Glad we at least made sure this was okay.