GetLoginsRequest: Use ScopedVector to express ownership of forms

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 096257b8d1a3114a819949087cb7350fd5c5c466..b72fe1794eb156c90f59dc882eef45d2da71d9c1 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -384,8 +384,7 @@ void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
     client_->AutofillResultsComputed();
     // There is no point in looking for the credentials in the store when they
     // won't be autofilled, so pretend there were none.
-    std::vector<autofill::PasswordForm*> dummy_results;
-    OnGetPasswordStoreResults(dummy_results);
+    OnGetPasswordStoreResults(ScopedVector<autofill::PasswordForm>());
     return;
   }
 
@@ -411,10 +410,8 @@ bool PasswordFormManager::IsIgnorableChangePasswordForm() const {
 }
 
 void PasswordFormManager::OnRequestDone(
-    const std::vector<PasswordForm*>& logins_result) {
-  // Note that the result gets deleted after this call completes, but we own
-  // the PasswordForm objects pointed to by the result vector, thus we keep
-  // copies to a minimum here.
+    ScopedVector<PasswordForm> logins_result) {
+  const size_t logins_result_size = logins_result.size();
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
@@ -424,65 +421,63 @@ void PasswordFormManager::OnRequestDone(
 
   int best_score = 0;
   // These credentials will be in the final result regardless of score.
-  std::vector<PasswordForm> credentials_to_keep;
-  for (size_t i = 0; i < logins_result.size(); i++) {
-    if (ShouldIgnoreResult(*logins_result[i])) {
-      delete logins_result[i];
+  ScopedVector<PasswordForm> credentials_to_keep;
+  for (auto& login_ptr : logins_result) {
+    scoped_ptr<PasswordForm> login(login_ptr);
+    login_ptr = nullptr;
+    // A stable handle to the form while the owners change.
+    const PasswordForm& const_login(*login);
+
+    if (ShouldIgnoreResult(const_login))
       continue;
-    }
+
     // Score and update best matches.
-    int current_score = ScoreResult(*logins_result[i]);
-    // This check is here so we can append empty path matches in the event
-    // they don't score as high as others and aren't added to best_matches_.
-    // This is most commonly imported firefox logins. We skip blacklisted
-    // ones because clearly we don't want to autofill them, and secondly
-    // because they only mean something when we have no other matches already
-    // saved in Chrome - in which case they'll make it through the regular
-    // scoring flow below by design. Note signon_realm == origin implies empty
-    // path logins_result, since signon_realm is a prefix of origin for HTML
-    // password forms.
-    // TODO(timsteele): Bug 1269400. We probably should do something more
-    // elegant for any shorter-path match instead of explicitly handling empty
-    // path matches.
+    int current_score = ScoreResult(const_login);
     if ((observed_form_.scheme == PasswordForm::SCHEME_HTML) &&
-        (observed_form_.signon_realm == logins_result[i]->origin.spec()) &&
-        (current_score > 0) && (!logins_result[i]->blacklisted_by_user)) {
-      credentials_to_keep.push_back(*logins_result[i]);
+        (observed_form_.signon_realm == const_login.origin.spec()) &&
+        (current_score > 0) && (!const_login.blacklisted_by_user)) {
+      // This check is here so we can append empty path matches in the event
+      // they don't score as high as others and aren't added to best_matches_.
+      // This is most commonly imported firefox logins. We skip blacklisted
+      // ones because clearly we don't want to autofill them, and secondly
+      // because they only mean something when we have no other matches already
+      // saved in Chrome - in which case they'll make it through the regular
+      // scoring flow below by design. Note signon_realm == origin implies empty
+      // path logins_result, since signon_realm is a prefix of origin for HTML
+      // password forms.
+      // TODO(timsteele): Bug 1269400. We probably should do something more
+      // elegant for any shorter-path match instead of explicitly handling empty
+      // path matches.
+      credentials_to_keep.push_back(login.release());
+    } else if (const_login.type == PasswordForm::TYPE_GENERATED) {
+      // Always keep generated passwords as part of the result set. If a user
+      // generates a password on a signup form, it should show on a login form
+      // even if they have a previous login saved.
+      // TODO(gcasto): We don't want to cut credentials that were saved on
+      // signup forms even if they weren't generated, but currently it's hard to
+      // distinguish between those forms and two different login forms on the
+      // same domain. Filed http://crbug.com/294468 to look into this.
+      credentials_to_keep.push_back(login.release());
     }
 
-    // Always keep generated passwords as part of the result set. If a user
-    // generates a password on a signup form, it should show on a login form
-    // even if they have a previous login saved.
-    // TODO(gcasto): We don't want to cut credentials that were saved on signup
-    // forms even if they weren't generated, but currently it's hard to
-    // distinguish between those forms and two different login forms on the
-    // same domain. Filed http://crbug.com/294468 to look into this.
-    if (logins_result[i]->type == PasswordForm::TYPE_GENERATED)
-      credentials_to_keep.push_back(*logins_result[i]);
-
+    // End of exceptions, only interested in good-scoring candidates since now.
     if (current_score < best_score) {
-      delete logins_result[i];
       continue;
-    }
-    if (current_score == best_score) {
-      PasswordForm* old_form = best_matches_[logins_result[i]->username_value];
-      if (old_form) {
-        if (preferred_match_ == old_form)
-          preferred_match_ = NULL;
-        delete old_form;
-      }
-      best_matches_[logins_result[i]->username_value] = logins_result[i];
-    } else if (current_score > best_score) {
+    } else if (current_score == best_score) {
+      auto& best_match = best_matches_[const_login.username_value];
+      if (best_match == preferred_match_)
+        preferred_match_ = nullptr;
+      delete best_match;
+      best_match = login.release();
+    } else {  // current_score > best_score
       best_score = current_score;
-      // This new login has a better score than all those up to this point
-      // Note 'this' owns all the PasswordForms in best_matches_.
-      STLDeleteValues(&best_matches_);
-      preferred_match_ = NULL;  // Don't delete, its owned by best_matches_.
-      best_matches_[logins_result[i]->username_value] = logins_result[i];
+      preferred_match_ = nullptr;  // Don't delete, its owned by best_matches_.
+      STLDeleteValues(&best_matches_);  // |login| tops the previous matches.
+      best_matches_[const_login.username_value] = login.release();
     }
-    preferred_match_ =
-        logins_result[i]->preferred ? logins_result[i] : preferred_match_;
+    preferred_match_ = const_login.preferred ? &const_login : preferred_match_;

[vasilii, 2015/02/09 10:08:35]

What about DCHECK(!login) above because you save the permanent pointer to the
form.

[vabr (Chromium), 2015/02/09 11:36:06]

Done.

   }
+  logins_result.weak_clear();  // It contains just nulls, speed up destruction.
 
   client_->AutofillResultsComputed();
 
@@ -495,18 +490,19 @@ void PasswordFormManager::OnRequestDone(
     return;
   }
 
-  for (std::vector<PasswordForm>::const_iterator it =
-           credentials_to_keep.begin();
-       it != credentials_to_keep.end(); ++it) {
+  for (auto& form : credentials_to_keep) {
     // If we don't already have a result with the same username, add the
     // lower-scored match (if it had equal score it would already be in
     // best_matches_).
-    if (best_matches_.find(it->username_value) == best_matches_.end())
-      best_matches_[it->username_value] = new PasswordForm(*it);
+    auto& corresponding_best_match = best_matches_[form->username_value];
+    if (!corresponding_best_match) {
+      corresponding_best_match = form;
+      form = nullptr;
+    }
   }
 
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsNotShown",
-                       logins_result.size() - best_matches_.size());
+                       logins_result_size - best_matches_.size());
 
   // It is possible we have at least one match but have no preferred_match_,
   // because a user may have chosen to 'Forget' the preferred match. So we
@@ -556,7 +552,7 @@ void PasswordFormManager::ProcessFrame(
 }
 
 void PasswordFormManager::OnGetPasswordStoreResults(
-    const std::vector<autofill::PasswordForm*>& results) {
+    ScopedVector<PasswordForm> results) {
   DCHECK_EQ(state_, MATCHING_PHASE);
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
@@ -567,7 +563,7 @@ void PasswordFormManager::OnGetPasswordStoreResults(
   }
 
   if (!results.empty())
-    OnRequestDone(results);
+    OnRequestDone(results.Pass());
   state_ = POST_MATCHING_PHASE;
 
   if (manager_action_ != kManagerActionBlacklisted) {