[sync] Add a polling mechanism to CredentialCacheService for on-the-fly sign in / sign out / reconf…

[sync] Add a polling mechanism to CredentialCacheService for on-the-fly sign in / sign out / reconfigure detection

With sync credential caching on Win 8, if Metro and Desktop are both
running at the same time, and the user signs in / signs out / reconfigures
sync in one mode, the other mode gets updated only after restart.

This patch implements a polling mechanism to enable first-time
auto-sign-in, first time auto-sign-out, and sync setting / credential
reconfigurations without the need for browser restart.

BUG=139200
TEST=Launch clean Desktop and Metro chrome, sign in / sign out / reconfigure one mode, watch for a minute and the other mode should catch up

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

[Raghu Simha, 2012-08-01 05:42:26]

Drew, please review. Thanks.

[Andrew T Wilson (Slow), 2012-08-03 20:39:56]

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:260: return
!sync_prefs_.IsManaged() && !sync_prefs_.IsStartSuppressed();
Should we document why we want to look for credentials even if we're already
signed in (is this to detect signouts in the alternate profile?)

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:399: if
(!sync_prefs_.GetGoogleServicesUsername().empty() &&
Let's change all these references to SigninManager::GetAuthenticatedUsername()
instead of going through sync_prefs.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:402:
!service->setup_in_progress()) {
Why do we want to avoid signing out the user if they are configuring sync? How
does the code behave if I do this:

a) Bring up sync settings on Metro
b) on desktop sign out
c) switch to metro, close sync settings

At what point does Metro get signed out? It seems dangerous for us to continue
on in this routine in either case - maybe this case is handled by
MayReconfigureSync below?

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:430:
preferred_types.Put(it.Get());
Is this loop dangerous to execute if the user is configuring sync currently (we
might change settings out from under it?)

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:507:
service->DisableForUser();
This is fine, but it's slightly better to use SigninManager::SignOut() instead.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:539: // 3) The user is
signed in to the alternate profile with the same account.
You left out step 4: Profit

[Raghu Simha, 2012-08-03 21:57:34]

Drew, PTAL.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:260: return
!sync_prefs_.IsManaged() && !sync_prefs_.IsStartSuppressed();
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> Should we document why we want to look for credentials even if we're already
> signed in (is this to detect signouts in the alternate profile?)

Yes, it is to detect sign outs in the alternate profile. Comment added.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:399: if
(!sync_prefs_.GetGoogleServicesUsername().empty() &&
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> Let's change all these references to SigninManager::GetAuthenticatedUsername()
> instead of going through sync_prefs.

Done.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:402:
!service->setup_in_progress()) {
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> Why do we want to avoid signing out the user if they are configuring sync?

If the user is configuring sync, we want to avoid second guessing (and
overriding) their intentions. Signing out a user from underneath when they are
actively configuring sync seems like a bad idea.

 How
> does the code behave if I do this:
> 
> a) Bring up sync settings on Metro
> b) on desktop sign out
> c) switch to metro, close sync settings
> 
> At what point does Metro get signed out?

In the case you describe, Metro will get signed out during the next poll, after
you close the sync settings.

 It seems dangerous for us to continue
> on in this routine in either case - maybe this case is handled by
> MayReconfigureSync below?

Yes. MayReconfigureSync() will return false if the user is in the process of
configuring sync, and we take no further action in this routine.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:430:
preferred_types.Put(it.Get());
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> Is this loop dangerous to execute if the user is configuring sync currently
(we
> might change settings out from under it?)

I don't think it's dangerous to run this loop. It's merely extracting a set of
preferred sync datatypes from the blob that we read from the alternate
credential cache. We don't actually change any settings in the local pref store
in this loop. And when the prefs are actually applied to the local store, we
make sure that the user is not already configuring sync.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:507:
service->DisableForUser();
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> This is fine, but it's slightly better to use SigninManager::SignOut()
instead.

Done.

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:539: // 3) The user is
signed in to the alternate profile with the same account.
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> You left out step 4: Profit

Hah! Fixed.

[Raghu Simha, 2012-08-03 22:12:52]

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10828108/diff/10002/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:507:
service->DisableForUser();
On 2012/08/03 20:39:56, Andrew T Wilson wrote:
> This is fine, but it's slightly better to use SigninManager::SignOut()
instead.

Actually, PSS doesn't handle the notification sent by SigninManager:SignOut(),
so what you suggest is not going to work. Of course, DisableForUser() implicitly
calls SigninManager:SignOut(), so other parts of the browser that rely on the
notification will end up doing the right thing.

[Raghu Simha, 2012-08-03 23:16:19]

I just realized that if we sign in to both modes and exit, then open Desktop,
reconfigure sync, exit Desktop, and relaunch Desktop without having ever
launched Metro in the meantime, we will end up reverting to the old settings
that still exist in the Metro cache.

I've added a mechanism to avoid this by writing timestamps to the caches, so
that we know when a cache was last updated.

[Andrew T Wilson (Slow), 2012-08-03 23:33:24]

Mostly looks good - just want to understand how we handle logging out then
logging back in (see my comment on line 388 of credential_cache_service_win.cc).

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:40: const int
kCredentialCachePollIntervalSecs = 60;
Now I wonder if 60 secs is too frequent for us to be reading and re-parsing a
JSON file. What do you think?

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:223:
base::Value::CreateBooleanValue(new_value));
Incorrect indentation.

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:238:
last_updated_time_value->GetAsInteger(&last_updated_time);
Should we add a CHECK here for the return value? Or at least a
DCHECK(IsType(TYPE_INTEGER))?

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:388: if (HasUserSignedOut())
So, if the user logs out in one profile, that should get synced to the remote
profile.

Now, what if they log back in on the other profile - do we not want to sign them
back in here?

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:530: const ModelTypeSet&
preferred_types) const {
Part of the contract of ModelTypeSet is that you can just pass it around as a
value - no need to pass by reference. See enum_set.h in
sync/internal_api/public/base/

[Raghu Simha, 2012-08-04 00:18:02]

Thanks for the review. PTAL.

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:40: const int
kCredentialCachePollIntervalSecs = 60;
On 2012/08/03 23:33:24, Andrew T Wilson wrote:
> Now I wonder if 60 secs is too frequent for us to be reading and re-parsing a
> JSON file. What do you think?

I could increase this to 2 minutes, or even 5 minutes. I think increasing the
delay is perfectly fine for the sign-out case. The question is: How fast fo we
want a user to show up as signed in on the other side if both modes are active?
If we up the interval to 5 minutes, would the user be tempted to manually sign
in, thereby defeating the purpose of this feature?

How about making the timeout 2 minutes for now?

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:223:
base::Value::CreateBooleanValue(new_value));
On 2012/08/03 23:33:24, Andrew T Wilson wrote:
> Incorrect indentation.

Done.

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:238:
last_updated_time_value->GetAsInteger(&last_updated_time);
On 2012/08/03 23:33:24, Andrew T Wilson wrote:
> Should we add a CHECK here for the return value? Or at least a
> DCHECK(IsType(TYPE_INTEGER))?

I've now made this a string pref that's converted from the int64 returned by
Now(). We were losing precision with just an integer pref, which was a bad
thing. Also added a DCHECK to make sure that the conversion worked.

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:388: if (HasUserSignedOut())
On 2012/08/03 23:33:24, Andrew T Wilson wrote:
> So, if the user logs out in one profile, that should get synced to the remote
> profile.
> 
> Now, what if they log back in on the other profile - do we not want to sign
them
> back in here?

As of now, the answer is no, as discussed in the design doc. We don't want to
risk merging in a whole lot of old state to a sync account, and more
importantly, we don't want to inadvertently mix data from two accounts if the
user swaps the account with which they were signed in.

http://codereview.chromium.org/10828108/diff/4013/chrome/browser/sync/credent...
chrome/browser/sync/credential_cache_service_win.cc:530: const ModelTypeSet&
preferred_types) const {
On 2012/08/03 23:33:24, Andrew T Wilson wrote:
> Part of the contract of ModelTypeSet is that you can just pass it around as a
> value - no need to pass by reference. See enum_set.h in
> sync/internal_api/public/base/

Done.

[Andrew T Wilson (Slow), 2012-08-04 01:25:18]

OK, LGTM given your description of the desired behavior (logging out of one
profile disables credential caching permanently)

[commit-bot: I haz the power, 2012-08-04 01:28:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/10828108/1010

[commit-bot: I haz the power, 2012-08-04 01:43:43]

Try job failure for 10828108-1010 (retry) on win_rel for step "runhooks"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...