[Password Manager] Fix crash in reporting sync stats.

It is possible for password_sync_metrics::IsPasswordSyncUsername() to be
called during sync setup, which currently causes a stack overflow since
GetPasswordSyncUsername() will try to re-initialize sync. Fixed by not checking
to see if the user is syncing passwords, just if they have a saved username.
This is slightly less specific, but determining if password sync is occuring
during setup doesn't seem reasonable.

R=isherman@chromium.org
TBR=vabr@chromium.org
BUG=393626, 386403

Review URL: https://codereview.chromium.org/383083009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283254 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -102,9 +102,9 @@ bool ChromePasswordManagerClient::IsPasswordManagerEnabledForCurrentPage()
   return entry->GetURL().host() != chrome::kChromeUIChromeSigninHost;
 }
 
-bool ChromePasswordManagerClient::IsPasswordSyncAccountCredential(
+bool ChromePasswordManagerClient::IsSyncAccountCredential(
     const std::string& username, const std::string& origin) const {
-  return password_manager_sync_metrics::IsPasswordSyncAccountCredential(
+  return password_manager_sync_metrics::IsSyncAccountCredential(
       profile_, username, origin);
 }
 

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -40,7 +40,7 @@ class ChromePasswordManagerClient
   // PasswordManagerClient implementation.
   virtual bool IsAutomaticPasswordSavingEnabled() const OVERRIDE;
   virtual bool IsPasswordManagerEnabledForCurrentPage() const OVERRIDE;
-  virtual bool IsPasswordSyncAccountCredential(
+  virtual bool IsSyncAccountCredential(
       const std::string& username, const std::string& origin) const OVERRIDE;
   virtual void PromptUserToSavePassword(
       password_manager::PasswordFormManager* form_to_save) OVERRIDE;

chrome/browser/password_manager/password_store_factory.cc

@@ -228,7 +228,7 @@ KeyedService* PasswordStoreFactory::BuildServiceInstanceFor(
   NOTIMPLEMENTED();
 #endif
   std::string sync_username =
-      password_manager_sync_metrics::GetPasswordSyncUsername(profile);
+      password_manager_sync_metrics::GetSyncUsername(profile);
   if (!ps || !ps->Init(
           sync_start_util::GetFlareForSyncableService(profile->GetPath()),
           sync_username)) {

chrome/browser/password_manager/sync_metrics.cc

@@ -13,14 +13,7 @@
 
 namespace password_manager_sync_metrics {
 
-std::string GetPasswordSyncUsername(Profile* profile) {
-  ProfileSyncService* sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile);
-  if (!sync_service ||
-      !sync_service->HasSyncSetupCompleted() ||
-      !sync_service->GetActiveDataTypes().Has(syncer::PASSWORDS))
-    return "";
-
+std::string GetSyncUsername(Profile* profile) {
   SigninManagerBase* signin_manager =
       SigninManagerFactory::GetForProfile(profile);
   if (!signin_manager)
@@ -29,13 +22,13 @@ std::string GetPasswordSyncUsername(Profile* profile) {
   return signin_manager->GetAuthenticatedUsername();
 }
 
-bool IsPasswordSyncAccountCredential(Profile* profile,
-                                     const std::string& username,
-                                     const std::string& origin) {
+bool IsSyncAccountCredential(Profile* profile,
+                             const std::string& username,
+                             const std::string& origin) {
   if (origin != GaiaUrls::GetInstance()->gaia_url().GetOrigin().spec())
     return false;
 
-  return gaia::AreEmailsSame(username, GetPasswordSyncUsername(profile));
+  return gaia::AreEmailsSame(username, GetSyncUsername(profile));
 }
 
 }  // namespace password_manager_sync_metrics

chrome/browser/password_manager/sync_metrics.h

@@ -12,14 +12,16 @@ class Profile;
 namespace password_manager_sync_metrics {
 
 // Returns the sync username for |profile|. Returns an empty string if the
-// |profile| isn't syncing passwords.
-std::string GetPasswordSyncUsername(Profile* profile);
+// |profile| isn't syncing. It would be preferable to only return the username
+// if the user is syncing passwords, but that is not currently possible since
+// this function can be called during sync setup (http://crbug.com/393626).
+std::string GetSyncUsername(Profile* profile);
 
 // Returns true if |username| and |origin| correspond to the account which is
-// syncing passwords. Will return false if |profile| is not syncing passwords.
-bool IsPasswordSyncAccountCredential(Profile* profile,
-                                     const std::string& username,
-                                     const std::string& origin);
+// syncing. Will return false if |profile| is not syncing.
+bool IsSyncAccountCredential(Profile* profile,
+                             const std::string& username,
+                             const std::string& origin);
 
 }  // namespace password_manager_sync_metrics
 

chrome/browser/ui/passwords/password_manager_presenter.cc

@@ -129,7 +129,7 @@ void PasswordManagerPresenter::RequestShowPassword(size_t index) {
       return;
   }
 
-  if (password_manager_sync_metrics::IsPasswordSyncAccountCredential(
+  if (password_manager_sync_metrics::IsSyncAccountCredential(
           password_view_->GetProfile(),
           base::UTF16ToUTF8(password_list_[index]->username_value),
           password_list_[index]->signon_realm)) {

components/password_manager/core/browser/password_form_manager.cc

@@ -561,7 +561,7 @@ void PasswordFormManager::UpdateLogin() {
   // Update metadata.
   ++pending_credentials_.times_used;
 
-  if (client_->IsPasswordSyncAccountCredential(
+  if (client_->IsSyncAccountCredential(
           base::UTF16ToUTF8(pending_credentials_.username_value),
           pending_credentials_.signon_realm)) {
     base::RecordAction(

components/password_manager/core/browser/password_manager.cc

@@ -218,7 +218,7 @@ void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
 
   // Don't save credentials for the syncing account. See crbug.com/365832 for
   // background.
-  if (client_->IsPasswordSyncAccountCredential(
+  if (client_->IsSyncAccountCredential(
           base::UTF16ToUTF8(form.username_value), form.signon_realm)) {
     RecordFailure(SYNC_CREDENTIAL, form.origin.host(), logger.get());
     return;

components/password_manager/core/browser/password_manager_client.h

@@ -35,8 +35,8 @@ class PasswordManagerClient {
   virtual bool IsPasswordManagerEnabledForCurrentPage() const;
 
   // Returns true if |username| and |origin| correspond to the account which is
-  // syncing passwords.
-  virtual bool IsPasswordSyncAccountCredential(
+  // syncing.
+  virtual bool IsSyncAccountCredential(
       const std::string& username, const std::string& origin) const = 0;
 
   // Informs the embedder of a password form that can be saved if the user

components/password_manager/core/browser/password_manager_unittest.cc

@@ -43,7 +43,7 @@ namespace {
 class MockPasswordManagerClient : public StubPasswordManagerClient {
  public:
   MOCK_CONST_METHOD0(IsPasswordManagerEnabledForCurrentPage, bool());
-  MOCK_CONST_METHOD2(IsPasswordSyncAccountCredential,
+  MOCK_CONST_METHOD2(IsSyncAccountCredential,
                      bool(const std::string&, const std::string&));
   MOCK_METHOD1(PromptUserToSavePassword, void(PasswordFormManager*));
   MOCK_METHOD0(GetPasswordStore, PasswordStore*());
@@ -87,7 +87,7 @@ class PasswordManagerTest : public testing::Test {
 
     EXPECT_CALL(client_, IsPasswordManagerEnabledForCurrentPage())
         .WillRepeatedly(Return(true));
-    EXPECT_CALL(client_, IsPasswordSyncAccountCredential(_, _))
+    EXPECT_CALL(client_, IsSyncAccountCredential(_, _))
         .WillRepeatedly(Return(false));
     EXPECT_CALL(client_, GetPasswordStore()).WillRepeatedly(Return(store_));
     EXPECT_CALL(client_, GetPrefs()).WillRepeatedly(Return(&prefs_));
@@ -711,7 +711,7 @@ TEST_F(PasswordManagerTest, AutofillingDisabledIfManagerDisabled) {
 }
 
 TEST_F(PasswordManagerTest, SyncCredentialsNotSaved) {
-  EXPECT_CALL(client_, IsPasswordSyncAccountCredential(_, _))
+  EXPECT_CALL(client_, IsSyncAccountCredential(_, _))
       .WillRepeatedly(Return(true));
 
   // Simulate loading a simple form with no existing stored password.

components/password_manager/core/browser/stub_password_manager_client.cc

@@ -10,7 +10,7 @@ StubPasswordManagerClient::StubPasswordManagerClient() {}
 
 StubPasswordManagerClient::~StubPasswordManagerClient() {}
 
-bool StubPasswordManagerClient::IsPasswordSyncAccountCredential(
+bool StubPasswordManagerClient::IsSyncAccountCredential(
     const std::string& username, const std::string& origin) const {
   return false;
 }

components/password_manager/core/browser/stub_password_manager_client.h

@@ -18,7 +18,7 @@ class StubPasswordManagerClient : public PasswordManagerClient {
   virtual ~StubPasswordManagerClient();
 
   // PasswordManagerClient:
-  virtual bool IsPasswordSyncAccountCredential(
+  virtual bool IsSyncAccountCredential(
       const std::string& username, const std::string& origin) const OVERRIDE;
   virtual void PromptUserToSavePassword(PasswordFormManager* form_to_save)
       OVERRIDE;