I looked at the code a bit and I think this happens because AccountReconcilor does not trigger on authentication errors. It only triggers on OnEndBatchChanges().

When the token becomes invalid, the cookie becomes invalid (i.e. we are logged out on the web) presumably because of cookie-token binding on the Gaia side. However the APISID probably does not change in this case, and the reconcilor does not trigger.

I think the fix is to trigger the reconcilor on auth errors.

David, will this have a side effect on how Profile.NumberOfAccountsPerProfile is counted?

Also, moving to M67, since we're moving Dice to M67.

I discussed this a bit with Mihai:
- we should force a ListAccounts rather than forcing a reconcilor cycle. Forcing a reconcilor cycle will do nothing if it runs on an outdated accounts list.
- doing it on authentication errors would catch some inconsistencies but not all.

The only possible complete fix would be to constantly ping ListAccounts to check that the accounts did not change. This would considerably increase the load on the Gaia server, so we may not want to do this.

Maybe an acceptable compromise would be to force the ListAccounts on authentication errors and also on a longish timer (like every ~1h) to not overload the Gaia servers too much.

It's hard to tell how important is it to fix this bug. It does not seem very important, except maybe for the ChildAccountService which may want to have an accurate view of the child accounts at all times.


ewald: I don't expect this will have a significant impact on metrics. This bug overall does not have a lot of consequences for Dice. Basically, when a cookie is invalidated from the server (which should not happen much: i.e. when the cookie is 2 years old on or password change), Chrome does not notice it until the next restart or the next signin/signout event. In the meantime we may be in an inconsistent state (although in practice, we will often still be consistent thanks to cookie-token binding on the server side), and some services/metrics can have an inaccurate view of what accounts are in the cookie.

Thanks David!

Do I get correctly that if you start using ListAccounts instead of the reconcilor cycle, the only inconsistencies would happen when Chrome doesn't actively use the tokens?

Now that many features depend on history sync being active, I think that shouldn't be an issue, since history sync actively uses the token regularly, so the features would learn of the issue. Is that right?

Perhaps with Unity, this could be a problem if the user opted in, but then turned off sync, and their token isn't being exercised regularly, and therefore we don't notice the server-side revocation?

I'm not sure what your concern is.

OnGaiaAccountsInCookieUpdated() is used to track the state of the accounts on the web.
The only features I know which really cares about the accounts on the web are:
- the ChildAccountService
- the about://signin-internals page

I thought Sync and dependent features only cared about the tokens in the token service.

Can you give more details on what the bug is concretely for you?

Thanks David!

I agree that in the current state, Sync and dependent features don't care about the accounts on the web, they use the token service instead, and therefore the issue in this bug right now should be limited to the few callers that actively use GaiaCookieManagerService and its observer.


In my last question in #8, I was wondering if there could be any situation in which active polling of the accounts status would be necessary (because I agree polling would be suboptimal). I don't see any reason right now, but the one reason I could think of was if we have unity in the future, that unity-dependent features (like UKM) would no longer check sync for the opt-in, but look at the Unity pref, and then combine that with the signin state to determine if they're allowed to run. In this case, those features may run longer than they should if GaiaCookieManagerService doesn't get the invalidation in a timely fashion.

I deleted my comment, let my do a better one.
We also chatted offline, so I can try to summarize here.

With unity, features like UKM will work as long as the user has given consent, regardless of token or cookie state.

I don't know on the top of my head if unity consent becomes invalid when the user signs out of Chrome, but even if it is the case it does not make a difference. Signing out of Chrome is not equivalent to having a valid token or a valid cookie (you could be signed into Chrome with and invalid token), so they would check the Chrome signin state and not the token or cookie state.

After discussion with Gaia, the only time when the cookie can become invalid is when the token is revoked. It seems that in particular the cookie cannot expire without the token expiring too.

Thus forcing a ListAccount call on authentication errors is enough.

I'm going to implement this solution.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f63247119f2d3fb6baa113f28663aa7db9e7de02

commit f63247119f2d3fb6baa113f28663aa7db9e7de02
Author: David Roger <droger@chromium.org>
Date: Tue Apr 10 12:23:32 2018

[Dice] Force ListAccount server call on authentication errors

This CL adds an API to OAuth2TokenServiceDelegate::Observer to
listen to changes in auth errors.

AccountReconcilor uses this API to trigger a ListAccount call
when a token becomes invalid.
This is necessary because in that case, the account can be invalid
in the cookie without the cookie changing. Chrome needs to make a
call to ListAccount to have an accurate view of the Gaia accounts in
the cookie.

Bug:  823714 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ie22226adfe6c425acaffe875a314c4a6b8de5e88
Reviewed-on: https://chromium-review.googlesource.com/986268
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549493}
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/chrome/browser/signin/oauth2_token_service_delegate_android.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/chrome/browser/signin/oauth2_token_service_delegate_android.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/core/browser/account_reconcilor.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/core/browser/account_reconcilor.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/core/browser/account_reconcilor_unittest.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/core/browser/fake_profile_oauth2_token_service.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/core/browser/fake_profile_oauth2_token_service.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate.mm
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/components/signin/ios/browser/profile_oauth2_token_service_ios_delegate_unittest.mm
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/fake_oauth2_token_service_delegate.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/fake_oauth2_token_service_delegate.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/oauth2_token_service.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/oauth2_token_service.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/oauth2_token_service_delegate.cc
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/google_apis/gaia/oauth2_token_service_delegate.h
[modify] https://crrev.com/f63247119f2d3fb6baa113f28663aa7db9e7de02/ios/chrome/browser/signin/authentication_service.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4b6058d8200e048e6b8ad9681b4e6af81b40711

commit d4b6058d8200e048e6b8ad9681b4e6af81b40711
Author: David Roger <droger@chromium.org>
Date: Tue Apr 10 16:54:54 2018

[signin] Remove unnecessary call to TriggerListAccounts()

The TODO was addressed in:
https://chromium-review.googlesource.com/c/chromium/src/+/986268

Bug:  823714 
Change-Id: I1201f14edb15a8f89921865aabf6f1326bb0612b
Reviewed-on: https://chromium-review.googlesource.com/995416
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549564}
[modify] https://crrev.com/d4b6058d8200e048e6b8ad9681b4e6af81b40711/chrome/browser/supervised_user/child_accounts/child_account_service.cc
[modify] https://crrev.com/d4b6058d8200e048e6b8ad9681b4e6af81b40711/chrome/browser/supervised_user/child_accounts/child_account_service.h