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..5d694dad8943cc99ede804bb0aba88ab7871a45a 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();
     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,14 +421,18 @@ 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]);
+    int current_score = ScoreResult(const_login);
     // 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
@@ -445,9 +446,9 @@ void PasswordFormManager::OnRequestDone(
     // elegant for any shorter-path match instead of explicitly handling empty
     // path matches.
     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)) {
+      credentials_to_keep.push_back(login.release());
     }
 
     // Always keep generated passwords as part of the result set. If a user
@@ -457,32 +458,27 @@ void PasswordFormManager::OnRequestDone(
     // 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]);
+    if (const_login.type == PasswordForm::TYPE_GENERATED)

[vasilii, 2015/02/03 19:22:16]

what about "else if" here so we don't save |login| in |credentials_to_keep|
twice?

[vabr (Chromium), 2015/02/04 16:13:44]

Good catch! Done.

Amusing though, that this double-saving did neither cause use-after free before
(due to the copy-by-value) nor after (due to |login| being already a scoped_ptr
and giving out nulls on subsequent releases).

+      credentials_to_keep.push_back(login.release());
 
+    // 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_;
   }
+  logins_result.weak_clear();  // It contains just nulls, speed up destruction.
 
   client_->AutofillResultsComputed();
 
@@ -495,18 +491,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
@@ -555,19 +552,18 @@ void PasswordFormManager::ProcessFrame(
                               *preferred_match_, wait_for_username);
 }
 
-void PasswordFormManager::OnGetPasswordStoreResults(
-    const std::vector<autofill::PasswordForm*>& results) {
+void PasswordFormManager::OnGetPasswordStoreResults() {
   DCHECK_EQ(state_, MATCHING_PHASE);
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_ON_GET_STORE_RESULTS_METHOD);
-    logger->LogNumber(Logger::STRING_NUMBER_RESULTS, results.size());
+    logger->LogNumber(Logger::STRING_NUMBER_RESULTS, results()->size());
   }
 
-  if (!results.empty())
-    OnRequestDone(results);
+  if (!results()->empty())
+    OnRequestDone(results()->Pass());
   state_ = POST_MATCHING_PHASE;
 
   if (manager_action_ != kManagerActionBlacklisted) {