GetLoginsRequest: Use ScopedVector to express ownership of forms

components/password_manager/core/browser/password_form_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/password_form_manager.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/metrics/histogram.h"
@@ skipping 366 matching lines @@
   }
 
   // Do not autofill on sign-up or change password forms (until we have some
   // working change password functionality).
   if (!observed_form_.new_password_element.empty()) {
     if (logger)
       logger->LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED);
     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;
   }
 
   PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     if (logger)
       logger->LogMessage(Logger::STRING_NO_STORE);
     NOTREACHED();
     return;
   }
   password_store->GetLogins(observed_form_, prompt_policy, this);
 }
 
 bool PasswordFormManager::HasCompletedMatching() const {
   return state_ == POST_MATCHING_PHASE;
 }
 
 bool PasswordFormManager::IsIgnorableChangePasswordForm() const {
   bool is_change_password_form = !observed_form_.new_password_element.empty() &&
                                  !observed_form_.password_element.empty();
   bool is_username_certainly_correct = observed_form_.username_marked_by_site;
   return is_change_password_form && !is_username_certainly_correct;
 }
 
 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()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_ON_REQUEST_DONE_METHOD);
   }
 
   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;
+    } 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;
+      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();
     }
-    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) {
-      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_ =
-        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();
 
   // TODO(gcasto): Change this to check that best_matches_ is empty. This should
   // be equivalent for the moment, but it's less clear and may not be
   // equivalent in the future.
   if (best_score <= 0) {
     if (logger)
       logger->LogNumber(Logger::STRING_BEST_SCORE, best_score);
     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
   // just pick the first one and whichever the user selects for submit will
   // be saved as preferred.
   DCHECK(!best_matches_.empty());
   if (!preferred_match_)
     preferred_match_ = best_matches_.begin()->second;
 
   // Check to see if the user told us to ignore this site in the past.
@@ skipping 29 matching lines @@
                            preferred_match_->IsPublicSuffixMatch();
   if (wait_for_username)
     manager_action_ = kManagerActionNone;
   else
     manager_action_ = kManagerActionAutofilled;
   password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
                               *preferred_match_, wait_for_username);
 }
 
 void PasswordFormManager::OnGetPasswordStoreResults(
-    const std::vector<autofill::PasswordForm*>& results) {
+    ScopedVector<PasswordForm> results) {
   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());
   }
 
   if (!results.empty())
-    OnRequestDone(results);
+    OnRequestDone(results.Pass());
   state_ = POST_MATCHING_PHASE;
 
   if (manager_action_ != kManagerActionBlacklisted) {
     for (auto const& driver : drivers_)
       ProcessFrame(driver);
   }
   drivers_.clear();
 }
 
 bool PasswordFormManager::ShouldIgnoreResult(const PasswordForm& form) const {
@@ skipping 281 matching lines @@
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMITTED);
 }
 
 void PasswordFormManager::SubmitFailed() {
   submit_result_ = kSubmitResultFailed;
   if (has_generated_password_)
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMISSION_FAILED);
 }
 
 }  // namespace password_manager