SigninManagerBase: Change AddObserver to SetObserver.
IdentityManager is intended to be the sole observer of
SigninManagerBase, so we shouldn't allow multiple observers.
Bug: 939372
Change-Id: I81454f4174e19c1dad8fe38df4191878fa9512e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1515573
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#640289}diff --git a/components/signin/core/browser/signin_manager.cc b/components/signin/core/browser/signin_manager.cc
index 79b00b7..ad3a6f5 100644
--- a/components/signin/core/browser/signin_manager.cc
+++ b/components/signin/core/browser/signin_manager.cc
@@ -39,8 +39,9 @@
SigninManager::~SigninManager() {}
void SigninManager::HandleAuthError(const GoogleServiceAuthError& error) {
- for (auto& observer : observer_list_)
- observer.GoogleSigninFailed(error);
+ if (observer_ != nullptr) {
+ observer_->GoogleSigninFailed(error);
+ }
}
void SigninManager::SignOut(
@@ -253,13 +254,14 @@
void SigninManager::FireGoogleSigninSucceeded() {
const AccountInfo account_info = GetAuthenticatedAccountInfo();
- for (auto& observer : observer_list_)
- observer.GoogleSigninSucceeded(account_info);
+ if (observer_ != nullptr) {
+ observer_->GoogleSigninSucceeded(account_info);
+ }
}
void SigninManager::FireGoogleSignedOut(const AccountInfo& account_info) {
- for (auto& observer : observer_list_) {
- observer.GoogleSignedOut(account_info);
+ if (observer_ != nullptr) {
+ observer_->GoogleSignedOut(account_info);
}
}
diff --git a/components/signin/core/browser/signin_manager_base.cc b/components/signin/core/browser/signin_manager_base.cc
index a6538b3..b3210dea 100644
--- a/components/signin/core/browser/signin_manager_base.cc
+++ b/components/signin/core/browser/signin_manager_base.cc
@@ -37,7 +37,9 @@
DCHECK(account_tracker_service_);
}
-SigninManagerBase::~SigninManagerBase() {}
+SigninManagerBase::~SigninManagerBase() {
+ DCHECK(!observer_);
+}
// static
void SigninManagerBase::RegisterProfilePrefs(PrefRegistrySimple* registry) {
@@ -154,7 +156,9 @@
void SigninManagerBase::FinalizeInitBeforeLoadingRefreshTokens(
PrefService* local_state) {}
-bool SigninManagerBase::IsInitialized() const { return initialized_; }
+bool SigninManagerBase::IsInitialized() const {
+ return initialized_;
+}
bool SigninManagerBase::IsSigninAllowed() const {
return client_->GetPrefs()->GetBoolean(prefs::kSigninAllowed);
@@ -192,8 +196,7 @@
client_->GetPrefs()->GetString(prefs::kGoogleServicesAccountId);
DCHECK(pref_account_id.empty() || pref_account_id == account_id)
- << "account_id=" << account_id
- << " pref_account_id=" << pref_account_id;
+ << "account_id=" << account_id << " pref_account_id=" << pref_account_id;
authenticated_account_id_ = account_id;
client_->GetPrefs()->SetString(prefs::kGoogleServicesAccountId, account_id);
@@ -229,10 +232,12 @@
return !authenticated_account_id_.empty();
}
-void SigninManagerBase::AddObserver(Observer* observer) {
- observer_list_.AddObserver(observer);
+void SigninManagerBase::SetObserver(Observer* observer) {
+ DCHECK(!observer_) << "SetObserver shouldn't be called multiple times.";
+ observer_ = observer;
}
-void SigninManagerBase::RemoveObserver(Observer* observer) {
- observer_list_.RemoveObserver(observer);
+void SigninManagerBase::ClearObserver() {
+ DCHECK(observer_);
+ observer_ = nullptr;
}
diff --git a/components/signin/core/browser/signin_manager_base.h b/components/signin/core/browser/signin_manager_base.h
index b3857e3..fdd331aa 100644
--- a/components/signin/core/browser/signin_manager_base.h
+++ b/components/signin/core/browser/signin_manager_base.h
@@ -125,9 +125,12 @@
// Returns true if there is an authenticated user.
bool IsAuthenticated() const;
- // Methods to register or remove observers of signin.
- void AddObserver(Observer* observer);
- void RemoveObserver(Observer* observer);
+ // Methods to set or clear the observer of signin.
+ // In practice these should only be used by IdentityManager.
+ // NOTE: If SetObserver is called, ClearObserver should be called before
+ // the destruction of SigninManagerBase.
+ void SetObserver(Observer* observer);
+ void ClearObserver();
protected:
SigninClient* signin_client() const { return client_; }
@@ -157,9 +160,9 @@
// call this.
void ClearAuthenticatedAccountId();
- // List of observers to notify on signin events.
- // Makes sure list is empty on destruction.
- base::ObserverList<Observer, true>::Unchecked observer_list_;
+ // Observer to notify on signin events.
+ // There is a DCHECK on destruction that this has been cleared.
+ Observer* observer_ = nullptr;
private:
// Added only to allow SigninManager to call the SigninManagerBase
diff --git a/components/signin/core/browser/signin_manager_unittest.cc b/components/signin/core/browser/signin_manager_unittest.cc
index 255182d..f63322d 100644
--- a/components/signin/core/browser/signin_manager_unittest.cc
+++ b/components/signin/core/browser/signin_manager_unittest.cc
@@ -39,9 +39,7 @@
class TestSigninManagerObserver : public SigninManagerBase::Observer {
public:
TestSigninManagerObserver()
- : num_failed_signins_(0),
- num_successful_signins_(0),
- num_signouts_(0) {}
+ : num_failed_signins_(0), num_successful_signins_(0), num_signouts_(0) {}
~TestSigninManagerObserver() override {}
@@ -87,7 +85,7 @@
~SigninManagerTest() override {
if (manager_) {
- manager_->RemoveObserver(&test_observer_);
+ manager_->ClearObserver();
manager_->Shutdown();
}
token_service_.Shutdown();
@@ -119,13 +117,13 @@
&test_signin_client_, &token_service_, &account_tracker_,
&cookie_manager_service_, account_consistency_);
manager_->Initialize(&local_state_);
- manager_->AddObserver(&test_observer_);
+ manager_->SetObserver(&test_observer_);
}
// Shuts down |manager_|.
void ShutDownManager() {
DCHECK(manager_);
- manager_->RemoveObserver(&test_observer_);
+ manager_->ClearObserver();
manager_->Shutdown();
manager_.reset();
}
diff --git a/services/identity/public/cpp/identity_manager.cc b/services/identity/public/cpp/identity_manager.cc
index 264a32c..a1383631 100644
--- a/services/identity/public/cpp/identity_manager.cc
+++ b/services/identity/public/cpp/identity_manager.cc
@@ -61,7 +61,7 @@
DCHECK(account_fetcher_service_);
DCHECK(accounts_cookie_mutator_);
DCHECK(diagnostics_provider_);
- signin_manager_->AddObserver(this);
+ signin_manager_->SetObserver(this);
token_service_->AddDiagnosticsObserver(this);
token_service_->AddObserver(this);
account_tracker_service_->AddObserver(this);
@@ -69,7 +69,7 @@
}
IdentityManager::~IdentityManager() {
- signin_manager_->RemoveObserver(this);
+ signin_manager_->ClearObserver();
token_service_->RemoveObserver(this);
token_service_->RemoveDiagnosticsObserver(this);
account_tracker_service_->RemoveObserver(this);
diff --git a/services/identity/public/cpp/identity_manager_unittest.cc b/services/identity/public/cpp/identity_manager_unittest.cc
index 01dc48f..b99b658 100644
--- a/services/identity/public/cpp/identity_manager_unittest.cc
+++ b/services/identity/public/cpp/identity_manager_unittest.cc
@@ -110,64 +110,6 @@
base::OnceClosure on_access_token_invalidated_callback_;
};
-class TestSigninManagerObserver : public SigninManagerBase::Observer {
- public:
- explicit TestSigninManagerObserver(SigninManagerBase* signin_manager)
- : signin_manager_(signin_manager) {
- signin_manager_->AddObserver(this);
- }
- ~TestSigninManagerObserver() override {
- signin_manager_->RemoveObserver(this);
- }
-
- void set_identity_manager(IdentityManager* identity_manager) {
- identity_manager_ = identity_manager;
- }
-
- void set_on_google_signin_succeeded_callback(base::OnceClosure callback) {
- on_google_signin_succeeded_callback_ = std::move(callback);
- }
- void set_on_google_signed_out_callback(base::OnceClosure callback) {
- on_google_signed_out_callback_ = std::move(callback);
- }
-
- const CoreAccountInfo& primary_account_from_signin_callback() const {
- return primary_account_from_signin_callback_;
- }
- const CoreAccountInfo& primary_account_from_signout_callback() const {
- return primary_account_from_signout_callback_;
- }
-
- private:
- // SigninManager::Observer:
- void GoogleSigninSucceeded(const AccountInfo&) override {
- // Fetch the primary account from IdentityManager. The goal is to check
- // that the account from IdentityManager has correct values even if other
- // SigninManager::Observer are notified.
- primary_account_from_signin_callback_ =
- identity_manager_->GetPrimaryAccountInfo();
- if (on_google_signin_succeeded_callback_)
- std::move(on_google_signin_succeeded_callback_).Run();
- }
- void GoogleSignedOut(const AccountInfo&) override {
- // Fetch the primary account from IdentityManager. The goal is to check
- // that the account from IdentityManager has correct values even if other
- // SigninManager::Observer are notified.
- primary_account_from_signout_callback_ =
- identity_manager_->GetPrimaryAccountInfo();
- if (on_google_signed_out_callback_)
- std::move(on_google_signed_out_callback_).Run();
- }
-
- SigninManagerBase* signin_manager_;
- IdentityManager* identity_manager_;
- base::OnceClosure on_google_signin_succeeded_callback_;
- base::OnceClosure on_google_signin_failed_callback_;
- base::OnceClosure on_google_signed_out_callback_;
- CoreAccountInfo primary_account_from_signin_callback_;
- CoreAccountInfo primary_account_from_signout_callback_;
-};
-
// Class that observes updates from ProfileOAuth2TokenService and and verifies
// thereby that IdentityManager receives updates before direct observers of
// ProfileOAuth2TokenService.
@@ -1440,62 +1382,6 @@
}
#endif
-#if !defined(OS_CHROMEOS)
-TEST_F(
- IdentityManagerTest,
- IdentityManagerGivesConsistentValuesFromSigninManagerObserverNotificationOfSignIn) {
- ClearPrimaryAccount(identity_manager(), ClearPrimaryAccountPolicy::DEFAULT);
-
- base::RunLoop run_loop;
- TestSigninManagerObserver signin_manager_observer(signin_manager());
- signin_manager_observer.set_on_google_signin_succeeded_callback(
- run_loop.QuitClosure());
-
- // NOTE: For this test to be meaningful, TestSigninManagerObserver
- // needs to be created before the IdentityManager instance that it's
- // interacting with. Otherwise, even an implementation where they're
- // both SigninManager::Observers would work as IdentityManager would
- // get notified first during the observer callbacks.
- RecreateIdentityManager();
- signin_manager_observer.set_identity_manager(identity_manager());
-
- SigninManager::FromSigninManagerBase(signin_manager())
- ->OnExternalSigninCompleted(kTestEmail);
-
- run_loop.Run();
-
- CoreAccountInfo primary_account_from_signin_callback =
- signin_manager_observer.primary_account_from_signin_callback();
- EXPECT_EQ(kTestGaiaId, primary_account_from_signin_callback.gaia);
- EXPECT_EQ(kTestEmail, primary_account_from_signin_callback.email);
-}
-
-TEST_F(
- IdentityManagerTest,
- IdentityManagerGivesConsistentValuesFromSigninManagerObserverNotificationOfSignOut) {
- base::RunLoop run_loop;
- TestSigninManagerObserver signin_manager_observer(signin_manager());
- signin_manager_observer.set_on_google_signed_out_callback(
- run_loop.QuitClosure());
-
- // NOTE: For this test to be meaningful, TestSigninManagerObserver
- // needs to be created before the IdentityManager instance that it's
- // interacting with. Otherwise, even an implementation where they're
- // both SigninManager::Observers would work as IdentityManager would
- // get notified first during the observer callbacks.
- RecreateIdentityManager();
- signin_manager_observer.set_identity_manager(identity_manager());
-
- ClearPrimaryAccount(identity_manager(), ClearPrimaryAccountPolicy::DEFAULT);
- run_loop.Run();
-
- CoreAccountInfo primary_account_from_signout_callback =
- signin_manager_observer.primary_account_from_signout_callback();
- EXPECT_EQ(std::string(), primary_account_from_signout_callback.gaia);
- EXPECT_EQ(std::string(), primary_account_from_signout_callback.email);
-}
-#endif
-
#if defined(OS_CHROMEOS)
// On ChromeOS, AccountTrackerService first receives the normalized email
// address from GAIA and then later has it updated with the user's
