IdentityManager: Consider reintroducing delayed handling of observer callbacks from internal signin classes to mimic interaction with Identity Service |
||
Issue descriptionWhen we first built out the IdentityManager C++ class and backed it via //components/signin classes, we wanted its interaction with those classes to mimic eventual interaction with the Identity Service as much as possible. As part of this design goal, we implemented it to handle callbacks from SigninManager by posting tasks to set its internal state and notify observers; this mimics the fact of receiving these notifications asynchronously from the Identity Service. This mimicking has proven useful, as it has ferreted out issues with tests assuming that signin state can be changed and have those changes observed synchronously. However, it has also proven problematic: 1. By delaying the change to IdentityManager's internal state, we could end up with clients getting inconsistent views of the user's signin state from SigninManager and IdentityManager (cf. crbug.com/804410 ). 2. We then changed IdentityManager to change its internal state before SigninManager sends its callbacks, but still delay notifying its (IdentityManager's) observers by posting a task. However, we have gotten feedback from developers of clients of IdentityManager that this behavior is problematic semantically: these developers want to be able to make changes to internal state in response to IdentityManager callbacks and have the invariant that those changes to internal state are made atomically with the change in IdentityManager's internal state. By decoupling the change in IdentityManager's internal state from the notification of its observers, these developers are forced to handle additional intermediate states in their code (e.g., "IdentityManager has changed to have the user be signed in but hasn't yet notified its observers." In consequence of (1) and (2), we are abandoning the asynchronous notification at this time. However, once the codebase is converted away from direct usage of SigninManager entirely to IdentityManager, it could be a good idea to reintroduce the implementation described in the top paragraph above; reintroducing that implementation would be a good intermediate move to switching in the Identity Service-backed implementation.
,
May 22 2018
We are also changing IdentityManager's interactions with ProfileOAuth2TokenService to be synchronous for similar reasons. As with SigninManager, once the codebase is completely converted away from direct usage of ProfileOAuth2TokenService, we should consider reintroducing asynchronous interaction to mimic IdentityManager's interaction with the Identity Service.
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1 commit 824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1 Author: Colin Blundell <blundell@chromium.org> Date: Thu May 24 13:12:05 2018 Make IdentityTestEnvironment agnostic to sync/async access token fetches IdentityTestEnvironment::WaitForAccessTokenRequest* currently assume that access token requests are handled asynchronously in the production code. Specifically, they assume that in the following test flow (wherein all steps are invoked in the same iteration of the runloop): 1. Invoke production method that makes an access token request 2. Invoke WaitForAccessTokenRequest* 3. WaitForAccessTokenRequest* runs a runloop waiting for the callback that an access token request was received the callback being waited for in 3 will not occur as part of 1. We are about to change PrimaryAccountAccessTokenFetcher to handle access token requests synchronously as part of easing the migration of the codebase from SigninManager/ProfileOAuth2TokenService to IdentityManager (see crbug.com/843510 for details). To enable that change, this CL makes IdentityTestEnvironment::WaitForAccessTokenRequest* agnostic as to whether the production code handles access token requests by posting a task to query ProfileOAuth2TokenService or by calling into ProfileOAuth2TokenService synchronously. We do so by changing IdentityTestEnvironment to handle the callback that an access token request was received via a posted task, thus allowing for the above test flow to continue to work as expected even if that callback is received in step 1. We also rename the relevant methods to WaitForAccessTokenRequestIfNecessary* to reflect the new semantics. Bug: 843510 Change-Id: Iae9785ce1bdaa9d8cb64958c27e22831c3369a5c Reviewed-on: https://chromium-review.googlesource.com/1070206 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#561479} [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/chromeos/services/device_sync/cryptauth_token_fetcher_impl_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/components/autofill/core/browser/payments/payments_client_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/components/feed/core/feed_networking_host_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/components/ntp_snippets/breaking_news/subscription_manager_impl_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/components/suggestions/suggestions_service_impl_unittest.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/services/identity/public/cpp/identity_test_environment.cc [modify] https://crrev.com/824f7e1b4e28fdfc13f3b03bf5164d84eadd3ed1/services/identity/public/cpp/identity_test_environment.h
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/962dce8d967d5a42b43cb084d96ef092a0ab64d1 commit 962dce8d967d5a42b43cb084d96ef092a0ab64d1 Author: Colin Blundell <blundell@chromium.org> Date: Fri May 25 07:16:27 2018 Have IdentityManager interact w/ ProfileOAuth2TokenService synchronously The reasoning for this change is analogous to that described in https://chromium-review.googlesource.com/1061494 for IdentityManager's interactions with SigninManager. As with that change, once the codebase is converted away from direct usage of PO2TS entirely to IdentityManager, it could be a good idea to reintroduce the asynchronous interaction; reintroducing that implementation would be a good intermediate move to switching in the Identity Service-backed implementation. As part of this change, we remove a test of PrimaryAccessTokenFetcher that assumes that the implementation handles access token requests asynchronously. Bug: 843510 Change-Id: Ie9a9b06460044fd12cd63299f21c7e24660d8cc1 Reviewed-on: https://chromium-review.googlesource.com/1068669 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#561796} [modify] https://crrev.com/962dce8d967d5a42b43cb084d96ef092a0ab64d1/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/962dce8d967d5a42b43cb084d96ef092a0ab64d1/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/962dce8d967d5a42b43cb084d96ef092a0ab64d1/services/identity/public/cpp/primary_account_access_token_fetcher.cc [modify] https://crrev.com/962dce8d967d5a42b43cb084d96ef092a0ab64d1/services/identity/public/cpp/primary_account_access_token_fetcher.h [modify] https://crrev.com/962dce8d967d5a42b43cb084d96ef092a0ab64d1/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc
,
Aug 2
|
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, May 16 2018