Fix DCHECK, memory leak, and refactor PasswordStore to use CancelableRequest

chrome/browser/ui/webui/options/password_manager_handler.cc

 // Copyright (c) 2011 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/ui/webui/options/password_manager_handler.h"
 
 #include "base/callback.h"
-#include "base/stl_util-inl.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
 PasswordManagerHandler::PasswordManagerHandler()
     : ALLOW_THIS_IN_INITIALIZER_LIST(populater_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(exception_populater_(this)) {
 }
 
 PasswordManagerHandler::~PasswordManagerHandler() {
-  // TODO(scr): ScopedVector.
-  STLDeleteElements(&password_list_);
-  STLDeleteElements(&password_exception_list_);
   GetPasswordStore()->RemoveObserver(this);
 }
 
 void PasswordManagerHandler::GetLocalizedValues(
     DictionaryValue* localized_strings) {
   DCHECK(localized_strings);
 
   RegisterTitle(localized_strings, "passwordsPage",
                 IDS_PASSWORDS_EXCEPTIONS_WINDOW_TITLE);
   localized_strings->SetString("savedPasswordsTitle",
@@ skipping 43 matching lines @@
 void PasswordManagerHandler::OnLoginsChanged() {
   UpdatePasswordLists(NULL);
 }
 
 PasswordStore* PasswordManagerHandler::GetPasswordStore() {
   return web_ui_->GetProfile()->GetPasswordStore(Profile::EXPLICIT_ACCESS);
 }
 
 void PasswordManagerHandler::UpdatePasswordLists(const ListValue* args) {
   // Reset the current lists.
-  STLDeleteElements(&password_list_);
-  STLDeleteElements(&password_exception_list_);
+  password_list_.reset();
+  password_exception_list_.reset();
 
   languages_ =
       web_ui_->GetProfile()->GetPrefs()->GetString(prefs::kAcceptLanguages);
   populater_.Populate();
   exception_populater_.Populate();
 }
 
 void PasswordManagerHandler::RemoveSavedPassword(const ListValue* args) {
   std::string string_value = UTF16ToUTF8(ExtractStringValue(args));
   int index;
@@ skipping 45 matching lines @@
   ListValue entries;
   for (size_t i = 0; i < password_exception_list_.size(); ++i) {
     entries.Append(new StringValue(
         net::FormatUrl(password_exception_list_[i]->origin, languages_)));
   }
 
   web_ui_->CallJavascriptFunction("PasswordManager.setPasswordExceptionsList",
                                   entries);
 }
 
+PasswordManagerHandler::ListPopulater::~ListPopulater() {

[James Hawkins, 2011/03/21 01:45:06]

Source ordering: ~ListPopulater() implementation should go after the
ListPopulater() implementation.

[Sheridan Rawlins, 2011/03/21 05:20:17]

Done.

+}
+
 PasswordManagerHandler::ListPopulater::ListPopulater(
     PasswordManagerHandler* page) : page_(page),
                                     pending_login_query_(0) {

[James Hawkins, 2011/03/21 01:45:06]

|pending_login_query_| should be a PasswordStore::Handle, and that should be
opaque, so we're going to need another way to initialize this.

[Sheridan Rawlins, 2011/03/21 05:20:17]

Done.

 }
 
-PasswordManagerHandler::ListPopulater::~ListPopulater() {
-  PasswordStore* store = page_->GetPasswordStore();
-  if (store)
-    store->CancelLoginsQuery(pending_login_query_);
-}
-
 PasswordManagerHandler::PasswordListPopulater::PasswordListPopulater(
     PasswordManagerHandler* page) : ListPopulater(page) {
 }
 
 void PasswordManagerHandler::PasswordListPopulater::Populate() {
-  DCHECK(!pending_login_query_);
   PasswordStore* store = page_->GetPasswordStore();
-  if (store != NULL)
+  if (store != NULL) {
+    if (pending_login_query_)

[James Hawkins, 2011/03/21 01:45:06]

Isn't PasswordStore::Handle opaque?  We're assuming an unset Handle is the value
0/false.

[Sheridan Rawlins, 2011/03/21 05:20:17]

Done.

+      store->CancelRequest(pending_login_query_);
+
     pending_login_query_ = store->GetAutofillableLogins(this);
-  else
+  } else {
     LOG(ERROR) << "No password store! Cannot display passwords.";
+  }
 }
 
 void PasswordManagerHandler::PasswordListPopulater::
-    OnPasswordStoreRequestDone(int handle,
-    const std::vector<webkit_glue::PasswordForm*>& result) {
+    OnPasswordStoreRequestDone(
+        PasswordStore::Handle handle,
+        const std::vector<webkit_glue::PasswordForm*>& result) {
   DCHECK_EQ(pending_login_query_, handle);
   pending_login_query_ = 0;
-  page_->password_list_ = result;
+  page_->password_list_.reset();
+  page_->password_list_.insert(page_->password_list_.end(),
+                               result.begin(), result.end());
   page_->SetPasswordList();
 }
 
 PasswordManagerHandler::PasswordExceptionListPopulater::
     PasswordExceptionListPopulater(PasswordManagerHandler* page)
         : ListPopulater(page) {
 }
 
 void PasswordManagerHandler::PasswordExceptionListPopulater::Populate() {
-  DCHECK(!pending_login_query_);
   PasswordStore* store = page_->GetPasswordStore();
-  if (store != NULL)
+  if (store != NULL) {
+    if (pending_login_query_)
+      store->CancelRequest(pending_login_query_);
+
     pending_login_query_ = store->GetBlacklistLogins(this);
-  else
+  } else {
     LOG(ERROR) << "No password store! Cannot display exceptions.";
+  }
 }
 
 void PasswordManagerHandler::PasswordExceptionListPopulater::
-    OnPasswordStoreRequestDone(int handle,
-    const std::vector<webkit_glue::PasswordForm*>& result) {
+    OnPasswordStoreRequestDone(
+        PasswordStore::Handle handle,
+        const std::vector<webkit_glue::PasswordForm*>& result) {
   DCHECK_EQ(pending_login_query_, handle);
   pending_login_query_ = 0;
-  page_->password_exception_list_ = result;
+  page_->password_exception_list_.reset();
+  page_->password_exception_list_.insert(page_->password_exception_list_.end(),
+                                         result.begin(), result.end());
   page_->SetPasswordExceptionList();
 }