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"
@@ skipping 88 matching lines @@
   delete password_exception_list_[index];
   password_exception_list_.erase(password_exception_list_.begin() + index);
   SetPasswordExceptionList();
 }
 
 void PasswordManagerHandler::RemoveAllSavedPasswords(
     const ListValue* args) {
   PasswordStore* store = GetPasswordStore();
   for (size_t i = 0; i < password_list_.size(); ++i)
     store->RemoveLogin(*password_list_[i]);
-  STLDeleteElements(&password_list_);
+  password_list_.reset();
   SetPasswordList();
 }
 
 void PasswordManagerHandler::RemoveAllPasswordExceptions(
     const ListValue* args) {
   PasswordStore* store = GetPasswordStore();
   for (size_t i = 0; i < password_exception_list_.size(); ++i)
     store->RemoveLogin(*password_exception_list_[i]);
-  STLDeleteElements(&password_exception_list_);
+  password_exception_list_.reset();
   SetPasswordExceptionList();
 }
 
 void PasswordManagerHandler::SetPasswordList() {
   ListValue entries;
   for (size_t i = 0; i < password_list_.size(); ++i) {
     ListValue* entry = new ListValue();
     entry->Append(new StringValue(net::FormatUrl(password_list_[i]->origin,
                                                  languages_)));
     entry->Append(new StringValue(password_list_[i]->username_value));
@@ skipping 14 matching lines @@
 
   web_ui_->CallJavascriptFunction("PasswordManager.setPasswordExceptionsList",
                                   entries);
 }
 
 PasswordManagerHandler::ListPopulater::ListPopulater(
     PasswordManagerHandler* page) : page_(page),
                                     pending_login_query_(0) {
 }
 
-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_)
+      store->CancelRequest(pending_login_query_);

[stuartmorgan, 2011/03/16 23:25:20]

Why the change from DCHECK to handling gracefully?

[Sheridan Rawlins, 2011/03/18 15:45:48]

My thought was that Populate should just "make it so" rather than requiring the
client to need to cancel first and then call.  I can change the commenting in
header if necessary - do you agree with this change, with explanation?

+
     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(result);
   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(result);
   page_->SetPasswordExceptionList();
 }