PasswordStore: Use ScopedVector to express ownership of forms

chrome/browser/password_manager/password_store_x.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 "chrome/browser/password_manager/password_store_x.h"
 
 #include <algorithm>
 #include <map>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "components/password_manager/core/browser/password_store_change.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 
 using autofill::PasswordForm;
 using content::BrowserThread;
 using password_manager::PasswordStoreChange;
 using password_manager::PasswordStoreChangeList;
 using password_manager::PasswordStoreDefault;
-using std::vector;
 
 namespace {
 
 bool AddLoginToBackend(const scoped_ptr<PasswordStoreX::NativeBackend>& backend,
                        const PasswordForm& form,
                        PasswordStoreChangeList* changes) {
   *changes = backend->AddLogin(form);
   return (!changes->empty() &&
           changes->back().type() == PasswordStoreChange::ADD);
 }
@@ skipping 85 matching lines @@
 }
 
 namespace {
 struct LoginLessThan {
   bool operator()(const PasswordForm* a, const PasswordForm* b) {
     return a->origin < b->origin;
   }
 };
 }  // anonymous namespace
 
-void PasswordStoreX::SortLoginsByOrigin(NativeBackend::PasswordFormList* list) {
+void PasswordStoreX::SortLoginsByOrigin(
+    std::vector<autofill::PasswordForm*>* list) {
   // In login_database.cc, the query has ORDER BY origin_url. Simulate that.
   std::sort(list->begin(), list->end(), LoginLessThan());
 }
 
 void PasswordStoreX::GetLoginsImpl(
     const autofill::PasswordForm& form,
     AuthorizationPromptPolicy prompt_policy,
     const ConsumerCallbackRunner& callback_runner) {
   CheckMigration();
-  std::vector<autofill::PasswordForm*> matched_forms;
+  ScopedVector<autofill::PasswordForm> matched_forms;
   if (use_native_backend() && backend_->GetLogins(form, &matched_forms)) {
-    SortLoginsByOrigin(&matched_forms);
+    SortLoginsByOrigin(&matched_forms.get());
     // The native backend may succeed and return no data even while locked, if
     // the query did not match anything stored. So we continue to allow fallback
     // until we perform a write operation, or until a read returns actual data.
     if (matched_forms.size() > 0)
       allow_fallback_ = false;
   } else if (allow_default_store()) {
     DCHECK(matched_forms.empty());
     PasswordStoreDefault::GetLoginsImpl(form, prompt_policy, callback_runner);
     return;
   }
   // The consumer will be left hanging unless we reply.
-  callback_runner.Run(matched_forms);
+  callback_runner.Run(&matched_forms);
 }
 
 void PasswordStoreX::GetAutofillableLoginsImpl(GetLoginsRequest* request) {
   CheckMigration();
+  ScopedVector<autofill::PasswordForm> obtained_forms;
   if (use_native_backend() &&
-      backend_->GetAutofillableLogins(request->result())) {
+      backend_->GetAutofillableLogins(&obtained_forms)) {
+    request->result()->swap(obtained_forms.get());
+    obtained_forms.weak_clear();  // TODO(vabr): Change request to have a

[vasilii, 2015/01/27 20:45:51]

Why? request->result() was probably empty. However, if not then we leak here.

[vabr (Chromium), 2015/01/28 13:27:36]

You are correct.
Done.

+                                  // ScopedVector instead.
     SortLoginsByOrigin(request->result());
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (request->result()->size() > 0)
       allow_fallback_ = false;
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetAutofillableLoginsImpl(request);
     return;
   }
   // The consumer will be left hanging unless we reply.
   ForwardLoginsResult(request);
 }
 
 void PasswordStoreX::GetBlacklistLoginsImpl(GetLoginsRequest* request) {
   CheckMigration();
-  if (use_native_backend() &&
-      backend_->GetBlacklistLogins(request->result())) {
+  ScopedVector<autofill::PasswordForm> obtained_forms;
+  if (use_native_backend() && backend_->GetBlacklistLogins(&obtained_forms)) {
+    request->result()->swap(obtained_forms.get());
+    obtained_forms.weak_clear();  // TODO(vabr): Change request to have a

[vasilii, 2015/01/27 20:45:51]

Same as above.

[vabr (Chromium), 2015/01/28 13:27:36]

Done.

+                                  // ScopedVector instead.
     SortLoginsByOrigin(request->result());
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (request->result()->size() > 0)
       allow_fallback_ = false;
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetBlacklistLoginsImpl(request);
     return;
   }
   // The consumer will be left hanging unless we reply.
   ForwardLoginsResult(request);
 }
 
-bool PasswordStoreX::FillAutofillableLogins(vector<PasswordForm*>* forms) {
+bool PasswordStoreX::FillAutofillableLogins(
+    ScopedVector<autofill::PasswordForm>* forms) {
   CheckMigration();
   if (use_native_backend() && backend_->GetAutofillableLogins(forms)) {
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (forms->size() > 0)
       allow_fallback_ = false;
     return true;
   }
   if (allow_default_store())
     return PasswordStoreDefault::FillAutofillableLogins(forms);
   return false;
 }
 
-bool PasswordStoreX::FillBlacklistLogins(vector<PasswordForm*>* forms) {
+bool PasswordStoreX::FillBlacklistLogins(
+    ScopedVector<autofill::PasswordForm>* forms) {
   CheckMigration();
   if (use_native_backend() && backend_->GetBlacklistLogins(forms)) {
     // See GetLoginsImpl() for why we disallow fallback conditionally here.
     if (forms->size() > 0)
       allow_fallback_ = false;
     return true;
   }
   if (allow_default_store())
     return PasswordStoreDefault::FillBlacklistLogins(forms);
   return false;
@@ skipping 27 matching lines @@
                  "Falling back on default (unencrypted) store.";
     backend_.reset(NULL);
     // Don't warn again. We'll use the default store because backend_ is NULL.
     allow_fallback_ = false;
   }
   return !backend_.get();
 }
 
 ssize_t PasswordStoreX::MigrateLogins() {
   DCHECK(backend_.get());
-  vector<PasswordForm*> forms;
+  ScopedVector<autofill::PasswordForm> forms;
   bool ok = PasswordStoreDefault::FillAutofillableLogins(&forms) &&
       PasswordStoreDefault::FillBlacklistLogins(&forms);
   if (ok) {
     // We add all the passwords (and blacklist entries) to the native backend
     // before attempting to remove any from the login database, to make sure we
     // don't somehow end up with some of the passwords in one store and some in
     // another. We'll always have at least one intact store this way.
     for (size_t i = 0; i < forms.size(); ++i) {
       PasswordStoreChangeList changes;
       if (!AddLoginToBackend(backend_, *forms[i], &changes)) {
@@ skipping 12 matching lines @@
         PasswordStoreDefault::RemoveLoginImpl(*forms[i]);
       }
       // Finally, delete the database file itself. We remove the passwords from
       // it before deleting the file just in case there is some problem deleting
       // the file (e.g. directory is not writable, but file is), which would
       // otherwise cause passwords to re-migrate next (or maybe every) time.
       DeleteAndRecreateDatabaseFile();
     }
   }
   ssize_t result = ok ? forms.size() : -1;
-  STLDeleteElements(&forms);
   return result;
 }