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

chrome/browser/password_manager/password_form_manager.cc

Index: chrome/browser/password_manager/password_form_manager.cc
diff --git a/chrome/browser/password_manager/password_form_manager.cc b/chrome/browser/password_manager/password_form_manager.cc
index f5c0f840779eda8b788e33b7ae0379b01a64366d..784bc0e30916de52889df7bbd9472a6ac1a3b03d 100644
--- a/chrome/browser/password_manager/password_form_manager.cc
+++ b/chrome/browser/password_manager/password_form_manager.cc
@@ -39,7 +39,6 @@ PasswordFormManager::PasswordFormManager(Profile* profile,
 }
 
 PasswordFormManager::~PasswordFormManager() {
-  CancelLoginsQuery();

[James Hawkins, 2011/03/16 22:39:35]

How does the PasswordStore know to not notify this object of
OnPasswordStoreRequestDone() after this object has been deleted?

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

Good question.  The cancelable_request recommends having a
CancelableRequestConsumer as a member of every consumer object.  Upon
destruction, that class cancels all of the pending requests managed by / pending
a response for it.  The framework has a thread-safe cancel flag and has several
checks for a request being canceled.  The one which answers your question is the
execution of the callback, which happens on the calling (UI) thread and here's a
code snippet:

class CancelableRequest : public CancelableRequestBase {
...
  void ExecuteCallback(const TupleType& param) {
    if (!canceled_.IsSet()) {
        ...
      // Execute the callback.
        ...
    }
  }

   UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTaken",
                             GetActionsTaken(),
                             kMaxNumActionsTaken);
@@ -306,7 +305,8 @@ void PasswordFormManager::OnRequestDone(int handle,
 }
 
 void PasswordFormManager::OnPasswordStoreRequestDone(
-    int handle, const std::vector<PasswordForm*>& result) {
+    PasswordStore::Handle handle,
+    const std::vector<webkit_glue::PasswordForm*>& result) {

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

What's the purpose of this change? This file has a using directive.

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

It was a global replace script that did it.  I'll revert and leave just the int
-> Handle change.

   DCHECK_EQ(state_, MATCHING_PHASE);
   DCHECK_EQ(pending_login_query_, handle);
 
@@ -316,6 +316,7 @@ void PasswordFormManager::OnPasswordStoreRequestDone(
   }
 
   OnRequestDone(handle, result);
+  pending_login_query_ = PasswordStore::Handle(0);

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

0 feels like a magic number. Can we have a constructor for making an invalid
handle instead (assuming that's what this is doing)

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

Handle is actually just an int.  Would the default constructor be better a la
the following?

    pending_login_query_ = PasswordStore::Handle();

 }
 
 bool PasswordFormManager::IgnoreResult(const PasswordForm& form) const {
@@ -420,16 +421,6 @@ void PasswordFormManager::UpdateLogin() {
   }
 }
 
-void PasswordFormManager::CancelLoginsQuery() {
-  PasswordStore* password_store =
-      profile_->GetPasswordStore(Profile::EXPLICIT_ACCESS);
-  if (!password_store) {
-    // Can be NULL in unit tests.
-    return;
-  }
-  password_store->CancelLoginsQuery(pending_login_query_);
-}
-
 int PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
   DCHECK_EQ(state_, MATCHING_PHASE);
   // For scoring of candidate login data: