Note that one complexity here is that this flow currently does not set the refresh token. It's likely that our mainstream Identity Service signin API will require setting the refresh token along with the gaia ID/email address. Thus, before making the transition of this flow to use that signin API, we would likely need to add the setting of the refresh token to this flow.

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

commit 523c662a78133425ffead1c689acfb3058762023
Author: Marc Treib <treib@chromium.org>
Date: Mon Apr 23 11:03:31 2018

Migrate chromeos::UserSessionManager to IdentityManager

identity::IdentityManager is the new API that replaces
SigninManager[Base].

Bug:  825190 , 796544, 814787
Change-Id: If6dff8e3d7e93c21bbc7392bf434dfe73da9f92d
Reviewed-on: https://chromium-review.googlesource.com/1013575
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552667}
[modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/chrome/browser/chromeos/login/session/user_session_manager.cc
[modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/services/identity/public/cpp/identity_manager.h

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

commit b218f3e0f3d8d80733e8391274eda6cc17e17dd8
Author: Marc Treib <treib@chromium.org>
Date: Tue Apr 24 11:39:42 2018

Update ProfileSyncService unit tests to use IdentityManager

Bug:  825190 , 814787
Change-Id: I733c0dc3baca101b9ebe1b2448a31fadf815ae58
Reviewed-on: https://chromium-review.googlesource.com/1024835
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553051}
[modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/BUILD.gn
[modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/profile_sync_service_autofill_unittest.cc
[modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/services/identity/public/cpp/identity_manager.h

Bumping to P2, because this has bitten me repeatedly. What happens now is:
Sync is initialized, queries the primary account (which is empty at this point), registers an IdentityManager::Observer, and then assumes that it always has up-to-date info on the primary account.
Now on ChromeOS, the SetPrimaryAccountSynchronously call happens and changes the primary account out from under us, without any notification.

Can we maybe make SetPrimaryAccountSynchronously also notify any observers? (Assuming it's not possible to get rid of it entirely.)

Hi Marc,

Thanks for the report!

To clarify, is this biting you in a production context, a testing context, or both?

What has changed here from when Sync was using SigninManagerBase directly, where there is similarly no notification sent on update of the account?

It's in a testing context. In particular, it's this SetPrimaryAccountSynchronously call that's causing trouble:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/chrome_session_manager.cc?rcl=e9f2be5afc79b27735c85b04493e12d4fe9c78b5&l=244

I'm actually not sure if this was different in the days of SigninManagerBase. I've just been running into it quite regularly when changing Sync code or tests.

To give the full context (apologies if you're already aware of some/all of this):

- SigninManagerBase *never* sends the GoogleSigninSucceeded/GoogleSignedOut observer callbacks.
- On ChromeOS in production, the primary account info is set very early and never changed. The assumed invariant is that it is set in SigninManagerBase before any consumers query SigninManagerBase.

Given that context, the ideal solution from my POV would be to ensure that ChromeSessionManager::Initialize() is called before ProfileSyncService queries SigninManagerBase in testing contexts (as it presumably is in production contexts). Do you happen to have an example test wherein the inverse is the case? I'd be happy to dig a bit into how feasible it would be to ensure this invariant.

I think ChromeSessionManager::Initialize (which under some circumstances calls SetPrimaryAccountSynchronously) is actually *always* called after Sync initialization. So I think it actually affects more or less all browser tests, though of course most of them don't care about sync and so run fine. (If we were to add more aggressive DCHECKs though, lots of tests might start failing.)
I just noticed that it also affects linux-chromeos [1], so it's not strictly a test-only problem.

I've made a CL [2] that removes some workaround code in Sync's auth logic. Turns out that currently only two SyncInternals tests fail due to this (much fewer than I expected - I remember quite some pain in making ChromeOS tests work...)

[1] https://chromium.googlesource.com/chromium/src/+/master/docs/chromeos_build_instructions.md
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1206491

I see. Thank you for the further information! As I wrote in c#8, I think that the bug here is in ChromeSessionManager's late mutation of SigninManagerBase's info. Since SigninManagerBase doesn't send the OnGoogleSigninSucceeded notification, ChromeOS-specific consumers of SigninManagerBase presumably don't listen for it. So even if we were to send the notification from SetPrimaryAccountSynchronously, that wouldn't generically fix the problem case here.

When I have time I'll dig into the ordering flow here. It won't be immediate though.