Fix crash in about://signin-internals when adding account in auth error

The crash was caused by SigninErrorController being called before the
AccountStatus was added to the map. Then when the about-internals page
tried to handle the error, it would try to get the account, but it would
crash because it was not completely added in the AccountStatusMap.

This CL also converts link_ptr to unique_ptr as a cleanup.

Bug: 823707
Change-Id: I0019da48de6e281d812538633a90f519f6b24d3a
Reviewed-on: https://chromium-review.googlesource.com/973370Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545044}

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc

@@ -252,6 +252,9 @@ MutableProfileOAuth2TokenServiceDelegate::AccountStatus::AccountStatus(
       last_auth_error_(GoogleServiceAuthError::NONE) {
   DCHECK(signin_error_controller_);
   DCHECK(!account_id_.empty());
+}
+
+void MutableProfileOAuth2TokenServiceDelegate::AccountStatus::Initialize() {
   signin_error_controller_->AddProvider(this);
 }
 
@@ -495,8 +498,7 @@ void MutableProfileOAuth2TokenServiceDelegate::OnWebDataServiceRequestDone(
       load_credentials_state_ =
           LOAD_CREDENTIALS_FINISHED_WITH_NO_TOKEN_FOR_PRIMARY_ACCOUNT;
     }
-    refresh_tokens_[loading_primary_account_id_].reset(new AccountStatus(
-        signin_error_controller_, loading_primary_account_id_, std::string()));
+    AddAccountStatus(loading_primary_account_id_, std::string());
   }
 
   // If we don't have a refresh token for a known account, signal an error.
@@ -688,8 +690,7 @@ void MutableProfileOAuth2TokenServiceDelegate::UpdateCredentialsInMemory(
   } else {
     VLOG(1) << "MutablePO2TS::UpdateCredentials; Refresh Token was absent. "
             << "account_id=" << account_id;
-    refresh_tokens_[account_id].reset(
-        new AccountStatus(signin_error_controller_, account_id, refresh_token));
+    AddAccountStatus(account_id, refresh_token);
   }
 
   UpdateAuthError(account_id,
@@ -719,9 +720,13 @@ void MutableProfileOAuth2TokenServiceDelegate::RevokeAllCredentials() {
 
   VLOG(1) << "MutablePO2TS::RevokeAllCredentials";
   CancelWebTokenFetch();
-  AccountStatusMap tokens = refresh_tokens_;
-  for (auto& token : tokens)
-    RevokeCredentials(token.first);
+
+  // Make a temporary copy of the account ids.
+  std::vector<std::string> accounts;
+  for (const auto& token : refresh_tokens_)
+    accounts.push_back(token.first);
+  for (const std::string& account : accounts)
+    RevokeCredentials(account);
 
   DCHECK_EQ(0u, refresh_tokens_.size());
 
@@ -796,3 +801,13 @@ const net::BackoffEntry*
     MutableProfileOAuth2TokenServiceDelegate::BackoffEntry() const {
   return &backoff_entry_;
 }
+
+void MutableProfileOAuth2TokenServiceDelegate::AddAccountStatus(
+    const std::string& account_id,
+    const std::string& refresh_token) {
+  DCHECK_EQ(0u, refresh_tokens_.count(account_id));
+  AccountStatus* status =
+      new AccountStatus(signin_error_controller_, account_id, refresh_token);
+  refresh_tokens_[account_id].reset(status);
+  status->Initialize();
+}

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h

@@ -92,6 +92,9 @@ class MutableProfileOAuth2TokenServiceDelegate
                   const std::string& refresh_token);
     ~AccountStatus() override;
 
+    // Must be called after the account has been added to the AccountStatusMap.
+    void Initialize();
+
     const std::string& refresh_token() const { return refresh_token_; }
     void set_refresh_token(const std::string& token) { refresh_token_ = token; }
 
@@ -166,9 +169,15 @@ class MutableProfileOAuth2TokenServiceDelegate
 
   std::string GetRefreshToken(const std::string& account_id) const;
 
+  // Creates a new AccountStatus and adds it to the AccountStatusMap.
+  // The account must not be already in the map.
+  void AddAccountStatus(const std::string& account_id,
+                        const std::string& refresh_token);
+
   // Maps the |account_id| of accounts known to ProfileOAuth2TokenService
   // to information about the account.
-  typedef std::map<std::string, linked_ptr<AccountStatus>> AccountStatusMap;
+  typedef std::map<std::string, std::unique_ptr<AccountStatus>>
+      AccountStatusMap;
   // In memory refresh token store mapping account_id to refresh_token.
   AccountStatusMap refresh_tokens_;
 

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -1090,3 +1091,41 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
   EXPECT_TRUE(
       oauth2_service_delegate_->RefreshTokenIsAvailable(secondary_account));
 }
+
+// Regression test for https://crbug.com/823707
+// Checks that OnErrorChanged() is called during UpdateCredentials(), and that
+// RefreshTokenIsAvailable() can be used at this time.
+TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, OnErrorChanged) {
+  class ErrorObserver : public SigninErrorController::Observer {
+   public:
+    explicit ErrorObserver(MutableProfileOAuth2TokenServiceDelegate* delegate)
+        : delegate_(delegate) {}
+
+    void OnErrorChanged() override {
+      error_changed_ = true;
+      EXPECT_TRUE(delegate_->RefreshTokenIsAvailable("account_id"));
+    }
+
+    MutableProfileOAuth2TokenServiceDelegate* delegate_;
+    bool error_changed_ = false;
+
+    DISALLOW_COPY_AND_ASSIGN(ErrorObserver);
+  };
+
+  CreateOAuth2ServiceDelegate(signin::AccountConsistencyMethod::kDisabled);
+
+  // Start with the SigninErrorController in error state, so that it calls
+  // OnErrorChanged() from AddProvider().
+  oauth2_service_delegate_->UpdateCredentials(
+      "error_account_id",
+      MutableProfileOAuth2TokenServiceDelegate::kInvalidRefreshToken);
+
+  ErrorObserver observer(oauth2_service_delegate_.get());
+  signin_error_controller_.AddObserver(&observer);
+
+  ASSERT_FALSE(observer.error_changed_);
+  oauth2_service_delegate_->UpdateCredentials("account_id", "token");
+  EXPECT_TRUE(observer.error_changed_);
+
+  signin_error_controller_.RemoveObserver(&observer);
+}