When looking at this bug, I found at least the following issues:

1) PasswordManagerHandler was not canceling requests if new requests were made (by double-clicking the "Manage Saved Passwords..." button with a slow database - simulated with long Sleep).
2) PasswordStore starts its handle numbering at 0 meaning that the very first request is non-true and could hit DCHECK(s).
3) PasswordManagerHandler doesn't free the results.

When talking with the DOMUI TL (jhawkins) and an author of PasswordStore (stuartmorgan), I learned that it was modeled after HistoryService, which had since been refactored into a reusable suite of classes in content/browser/cancelable_request.h

So the CL will include fixes for the 3 issues, as well as a refactor to use the shared code in cancelable_requst.h

BUG=71466
TEST=chrome://settings/personal, then click "Manage Saved Passwords".  Put
PlatformThread::Sleep(10000), then try double-clicking the button to see no error.

Review URL: http://codereview.chromium.org/6646051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@79625 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 207 insertions, 197 deletions

diff --git a/chrome/browser/password_manager/password_form_manager.cc b/chrome/browser/password_manager/password_form_manager.cc
index 11efdd2..c0a6724 100644
--- a/chrome/browser/password_manager/password_form_manager.cc
+++ b/chrome/browser/password_manager/password_form_manager.cc
@@ -10,6 +10,7 @@
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "chrome/browser/password_manager/password_manager.h"
+#include "chrome/browser/password_manager/password_store.h"
 #include "chrome/browser/profiles/profile.h"
 #include "webkit/glue/password_form_dom_manager.h"
 
@@ -39,7 +40,6 @@ PasswordFormManager::PasswordFormManager(Profile* profile,
 }
 
 PasswordFormManager::~PasswordFormManager() {
-  CancelLoginsQuery();
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTaken",
                             GetActionsTaken(),
                             kMaxNumActionsTaken);
@@ -306,7 +306,8 @@ void PasswordFormManager::OnRequestDone(int handle,
 }
 
 void PasswordFormManager::OnPasswordStoreRequestDone(
-    int handle, const std::vector<PasswordForm*>& result) {
+    CancelableRequestProvider::Handle handle,
+    const std::vector<PasswordForm*>& result) {
   DCHECK_EQ(state_, MATCHING_PHASE);
   DCHECK_EQ(pending_login_query_, handle);
 
@@ -316,6 +317,7 @@ void PasswordFormManager::OnPasswordStoreRequestDone(
   }
 
   OnRequestDone(handle, result);
+  pending_login_query_ = 0;
 }
 
 bool PasswordFormManager::IgnoreResult(const PasswordForm& form) const {
@@ -420,16 +422,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:
diff --git a/chrome/browser/password_manager/password_form_manager.h b/chrome/browser/password_manager/password_form_manager.h
index 698c3e2..f1c48bc 100644
--- a/chrome/browser/password_manager/password_form_manager.h
+++ b/chrome/browser/password_manager/password_form_manager.h
@@ -12,10 +12,11 @@
 #include "build/build_config.h"
 
 #include "base/stl_util-inl.h"
-#include "chrome/browser/password_manager/password_store.h"
+#include "chrome/browser/password_manager/password_store_consumer.h"
 #include "webkit/glue/password_form.h"
 
 class PasswordManager;
+class PasswordStore;
 class Profile;
 
 // Per-password-form-{on-page, dialog} class responsible for interactions
@@ -72,7 +73,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
 
   // PasswordStoreConsumer implementation.
   virtual void OnPasswordStoreRequestDone(
-      int handle, const std::vector<webkit_glue::PasswordForm*>& result);
+      CancelableRequestProvider::Handle handle,
+      const std::vector<webkit_glue::PasswordForm*>& result);
 
   // A user opted to 'never remember' passwords for this form.
   // Blacklist it so that from now on when it is seen we ignore it.
@@ -136,10 +138,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
   static const int kMaxNumActionsTaken = kManagerActionMax * kUserActionMax *
                                          kSubmitResultMax;
 
-  // Called by destructor to ensure if this object is deleted, no potential
-  // outstanding callbacks can call OnPasswordStoreRequestDone.
-  void CancelLoginsQuery();
-
   // Helper for OnPasswordStoreRequestDone to determine whether or not
   // the given result form is worth scoring.
   bool IgnoreResult(const webkit_glue::PasswordForm& form) const;
@@ -197,7 +195,7 @@ class PasswordFormManager : public PasswordStoreConsumer {
   const PasswordManager* const password_manager_;
 
   // Handle to any pending PasswordStore::GetLogins query.
-  int pending_login_query_;
+  CancelableRequestProvider::Handle pending_login_query_;
 
   // Convenience pointer to entry in best_matches_ that is marked
   // as preferred. This is only allowed to be null if there are no best matches
diff --git a/chrome/browser/password_manager/password_store.cc b/chrome/browser/password_manager/password_store.cc
index 1bab90f..7436a54 100644
--- a/chrome/browser/password_manager/password_store.cc
+++ b/chrome/browser/password_manager/password_store.cc
@@ -6,13 +6,27 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/stl_util-inl.h"
 #include "base/task.h"
+#include "chrome/browser/password_manager/password_store_consumer.h"
 #include "content/browser/browser_thread.h"
+#include "webkit/glue/password_form.h"
 
 using std::vector;
 using webkit_glue::PasswordForm;
 
-PasswordStore::PasswordStore() : handle_(0) {
+PasswordStore::GetLoginsRequest::GetLoginsRequest(GetLoginsCallback* callback)
+    : CancelableRequest1<GetLoginsCallback,
+                         std::vector<webkit_glue::PasswordForm*> >(callback) {
+}
+
+PasswordStore::GetLoginsRequest::~GetLoginsRequest() {
+  if (canceled()) {
+    STLDeleteElements(&value);
+  }
+}
+
+PasswordStore::PasswordStore() {
 }
 
 bool PasswordStore::Init() {
@@ -47,36 +61,19 @@ void PasswordStore::RemoveLoginsCreatedBetween(const base::Time& delete_begin,
       NewRunnableMethod(this, &PasswordStore::WrapModificationTask, task));
 }
 
-int PasswordStore::GetLogins(const PasswordForm& form,
-                             PasswordStoreConsumer* consumer) {
-  int handle = GetNewRequestHandle();
-  GetLoginsRequest* request = new GetLoginsRequest(consumer, handle);
-  ScheduleTask(NewRunnableMethod(this, &PasswordStore::GetLoginsImpl, request,
-                                 form));
-  return handle;
+CancelableRequestProvider::Handle PasswordStore::GetLogins(
+    const PasswordForm& form, PasswordStoreConsumer* consumer) {
+  return Schedule(&PasswordStore::GetLoginsImpl, consumer, form);
 }
 
-int PasswordStore::GetAutofillableLogins(PasswordStoreConsumer* consumer) {
-  int handle = GetNewRequestHandle();
-  GetLoginsRequest* request = new GetLoginsRequest(consumer, handle);
-  ScheduleTask(NewRunnableMethod(this,
-                                 &PasswordStore::GetAutofillableLoginsImpl,
-                                 request));
-  return handle;
+CancelableRequestProvider::Handle PasswordStore::GetAutofillableLogins(
+    PasswordStoreConsumer* consumer) {
+  return Schedule(&PasswordStore::GetAutofillableLoginsImpl, consumer);
 }
 
-int PasswordStore::GetBlacklistLogins(PasswordStoreConsumer* consumer) {
-  int handle = GetNewRequestHandle();
-  GetLoginsRequest* request = new GetLoginsRequest(consumer, handle);
-  ScheduleTask(NewRunnableMethod(this,
-                                 &PasswordStore::GetBlacklistLoginsImpl,
-                                 request));
-  return handle;
-}
-
-void PasswordStore::CancelLoginsQuery(int handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  pending_requests_.erase(handle);
+CancelableRequestProvider::Handle PasswordStore::GetBlacklistLogins(
+    PasswordStoreConsumer* consumer) {
+  return Schedule(&PasswordStore::GetBlacklistLoginsImpl, consumer);
 }
 
 void PasswordStore::ReportMetrics() {
@@ -93,50 +90,34 @@ void PasswordStore::RemoveObserver(Observer* observer) {
 
 PasswordStore::~PasswordStore() {}
 
-PasswordStore::GetLoginsRequest::GetLoginsRequest(
-    PasswordStoreConsumer* consumer, int handle)
-    : consumer(consumer), handle(handle), message_loop(MessageLoop::current()) {
-}
-
 void PasswordStore::ScheduleTask(Task* task) {
   BrowserThread::PostTask(BrowserThread::DB, FROM_HERE, task);
 }
-
-void PasswordStore::NotifyConsumer(GetLoginsRequest* request,
-                                   const vector<PasswordForm*>& forms) {
-  scoped_ptr<GetLoginsRequest> request_ptr(request);
-
-#if !defined(OS_MACOSX)
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
-#endif
-  request->message_loop->PostTask(
-      FROM_HERE,
-      NewRunnableMethod(this,
-                        &PasswordStore::NotifyConsumerImpl,
-                        request->consumer, request->handle, forms));
-}
-
-void PasswordStore::NotifyConsumerImpl(PasswordStoreConsumer* consumer,
-                                       int handle,
-                                       const vector<PasswordForm*>& forms) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // Don't notify the consumer if the request was canceled.
-  if (pending_requests_.find(handle) == pending_requests_.end()) {
-    // |forms| is const so we iterate rather than use STLDeleteElements().
-    for (size_t i = 0; i < forms.size(); ++i)
-      delete forms[i];
-    return;
-  }
-  pending_requests_.erase(handle);
-
-  consumer->OnPasswordStoreRequestDone(handle, forms);
-}
-
-int PasswordStore::GetNewRequestHandle() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  int handle = handle_++;
-  pending_requests_.insert(handle);
-  return handle;
+void PasswordStore::ForwardLoginsResult(GetLoginsRequest* request) {
+  request->ForwardResult(GetLoginsRequest::TupleType(request->handle(),
+                                                     request->value));
+}
+
+template<typename BackendFunc>
+CancelableRequestProvider::Handle PasswordStore::Schedule(
+    BackendFunc func, PasswordStoreConsumer* consumer) {
+  scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(
+      NewCallback(consumer,
+                  &PasswordStoreConsumer::OnPasswordStoreRequestDone)));
+  AddRequest(request, consumer->cancelable_consumer());
+  ScheduleTask(NewRunnableMethod(this, func, request));
+  return request->handle();
+}
+
+template<typename BackendFunc, typename ArgA>
+CancelableRequestProvider::Handle PasswordStore::Schedule(
+    BackendFunc func, PasswordStoreConsumer* consumer, const ArgA& a) {
+  scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(
+      NewCallback(consumer,
+                  &PasswordStoreConsumer::OnPasswordStoreRequestDone)));
+  AddRequest(request, consumer->cancelable_consumer());
+  ScheduleTask(NewRunnableMethod(this, func, request, a));
+  return request->handle();
 }
 
 void PasswordStore::WrapModificationTask(Task* task) {
diff --git a/chrome/browser/password_manager/password_store.h b/chrome/browser/password_manager/password_store.h
index 4b8b435..0212790 100644
--- a/chrome/browser/password_manager/password_store.h
+++ b/chrome/browser/password_manager/password_store.h
@@ -6,16 +6,16 @@
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_H_
 #pragma once
 
-#include <set>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
 #include "base/threading/thread.h"
 #include "base/time.h"
-#include "webkit/glue/password_form.h"
+#include "content/browser/cancelable_request.h"
 
-class Profile;
+class PasswordStoreConsumer;
 class Task;
 
 namespace browser_sync {
@@ -24,20 +24,39 @@ class PasswordModelAssociator;
 class PasswordModelWorker;
 };
 
-class PasswordStoreConsumer {
- public:
-  virtual ~PasswordStoreConsumer() {}
-  // Call this when the request is finished. If there are no results, call it
-  // anyway with an empty vector.
-  virtual void OnPasswordStoreRequestDone(
-      int handle, const std::vector<webkit_glue::PasswordForm*>& result) = 0;
+namespace webkit_glue {
+struct PasswordForm;
 };
 
 // Interface for storing form passwords in a platform-specific secure way.
 // The login request/manipulation API is not threadsafe and must be used
 // from the UI thread.
-class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
+class PasswordStore
+    : public base::RefCountedThreadSafe<PasswordStore>,
+      public CancelableRequestProvider {
  public:
+  typedef Callback2<Handle,
+                    const std::vector<webkit_glue::PasswordForm*>&>::Type
+      GetLoginsCallback;
+
+  // PasswordForm vector elements are meant to be owned by the
+  // PasswordStoreConsumer. However, if the request is canceled after the
+  // allocation, then the request must take care of the deletion.
+  // TODO(scr) If we can convert vector<PasswordForm*> to
+  // ScopedVector<PasswordForm>, then we can move the following class to merely
+  // a typedef. At the moment, a subclass of CancelableRequest1 is required to
+  // provide a destructor, which cleans up after canceled requests by deleting
+  // vector elements.
+  class GetLoginsRequest : public CancelableRequest1<
+    GetLoginsCallback, std::vector<webkit_glue::PasswordForm*> > {
+   public:
+    explicit GetLoginsRequest(GetLoginsCallback* callback);
+    virtual ~GetLoginsRequest();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(GetLoginsRequest);
+  };
+
   // An interface used to notify clients (observers) of this object that data in
   // the password store has changed. Register the observer via
   // PasswordStore::SetObserver.
@@ -69,24 +88,21 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
                                   const base::Time& delete_end);
 
   // Searches for a matching PasswordForm and returns a handle so the async
-  // request can be tracked. Implement the PasswordStoreConsumer interface to
-  // be notified on completion.
-  virtual int GetLogins(const webkit_glue::PasswordForm& form,
-                        PasswordStoreConsumer* consumer);
+  // request can be tracked. Implement the PasswordStoreConsumer interface to be
+  // notified on completion.
+  virtual Handle GetLogins(const webkit_glue::PasswordForm& form,
+                           PasswordStoreConsumer* consumer);
 
   // Gets the complete list of PasswordForms that are not blacklist entries--and
   // are thus auto-fillable--and returns a handle so the async request can be
-  // tracked. Implement the PasswordStoreConsumer interface to be notified
-  // on completion.
-  int GetAutofillableLogins(PasswordStoreConsumer* consumer);
+  // tracked. Implement the PasswordStoreConsumer interface to be notified on
+  // completion.
+  Handle GetAutofillableLogins(PasswordStoreConsumer* consumer);
 
   // Gets the complete list of PasswordForms that are blacklist entries, and
   // returns a handle so the async request can be tracked. Implement the
   // PasswordStoreConsumer interface to be notified on completion.
-  int GetBlacklistLogins(PasswordStoreConsumer* consumer);
-
-  // Cancels a previous Get*Logins query (async)
-  void CancelLoginsQuery(int handle);
+  Handle GetBlacklistLogins(PasswordStoreConsumer* consumer);
 
   // Reports usage metrics for the database.
   void ReportMetrics();
@@ -106,25 +122,7 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
 
   virtual ~PasswordStore();
 
-  // Simple container class that represents a login lookup request.
-  class GetLoginsRequest {
-   public:
-    GetLoginsRequest(PasswordStoreConsumer* c,
-                     int handle);
-
-    // The consumer to notify when this request is complete.
-    PasswordStoreConsumer* consumer;
-    // A unique handle for the request
-    int handle;
-    // The message loop that the request was made from.  We send the result
-    // back to the consumer in this same message loop.
-    MessageLoop* message_loop;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(GetLoginsRequest);
-  };
-
-  // Schedule the given task to be run in the PasswordStore's own thread.
+  // Schedule the given |task| to be run in the PasswordStore's own thread.
   virtual void ScheduleTask(Task* task);
 
   // These will be run in PasswordStore's own thread.
@@ -156,22 +154,23 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
   virtual bool FillBlacklistLogins(
       std::vector<webkit_glue::PasswordForm*>* forms) = 0;
 
-  // Notifies the consumer that a Get*Logins() request is complete.
-  virtual void NotifyConsumer(
-      GetLoginsRequest* request,
-      const std::vector<webkit_glue::PasswordForm*>& forms);
+  // Dispatches the result to the PasswordStoreConsumer on the original caller's
+  // thread so the callback can be executed there.  This should be the UI
+  // thread.
+  virtual void ForwardLoginsResult(GetLoginsRequest* request);
 
- private:
-  // Called by NotifyConsumer, but runs in the consumer's thread.  Will not
-  // call the consumer if the request was canceled.  This extra layer is here so
-  // that PasswordStoreConsumer doesn't have to be reference counted (we assume
-  // consumers will cancel their requests before they are destroyed).
-  void NotifyConsumerImpl(PasswordStoreConsumer* consumer, int handle,
-                          const std::vector<webkit_glue::PasswordForm*>& forms);
+  // Schedule the given |func| to be run in the PasswordStore's own thread with
+  // responses delivered to |consumer| on the current thread.
+  template<typename BackendFunc>
+  Handle Schedule(BackendFunc func, PasswordStoreConsumer* consumer);
 
-  // Returns a new request handle tracked in pending_requests_.
-  int GetNewRequestHandle();
+  // Schedule the given |func| to be run in the PasswordStore's own thread with
+  // argument |a| and responses delivered to |consumer| on the current thread.
+  template<typename BackendFunc, typename ArgA>
+  Handle Schedule(BackendFunc func, PasswordStoreConsumer* consumer,
+                  const ArgA& a);
 
+ private:
   // Wrapper method called on the destination thread (DB for non-mac) that calls
   // the method specified in |task| and then calls back into the source thread
   // to notify observers that the password store may have been modified via
@@ -185,14 +184,6 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
   // may have been changed.
   void NotifyLoginsChanged();
 
-  // Next handle to return from Get*Logins() to allow callers to track
-  // their request.
-  int handle_;
-
-  // List of pending request handles.  Handles are removed from the set when
-  // they finish or are canceled.
-  std::set<int> pending_requests_;
-
   // The observers.
   ObserverList<Observer> observers_;
 
diff --git a/chrome/browser/password_manager/password_store_consumer.cc b/chrome/browser/password_manager/password_store_consumer.cc
new file mode 100644
index 0000000..f0756ec
--- /dev/null
+++ b/chrome/browser/password_manager/password_store_consumer.cc
@@ -0,0 +1,8 @@
+// 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/password_manager/password_store_consumer.h"
+
+PasswordStoreConsumer::~PasswordStoreConsumer() {
+}
diff --git a/chrome/browser/password_manager/password_store_consumer.h b/chrome/browser/password_manager/password_store_consumer.h
new file mode 100644
index 0000000..ad399d7
--- /dev/null
+++ b/chrome/browser/password_manager/password_store_consumer.h
@@ -0,0 +1,43 @@
+// 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.
+
+#ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_CONSUMER_H_
+#define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_CONSUMER_H_
+#pragma once
+
+#include "content/browser/cancelable_request.h"
+
+namespace webkit_glue {
+struct PasswordForm;
+};
+
+// Reads from the PasswordStore are done asynchrously on a separate
+// thread. PasswordStoreConsumer provides the virtual callback method, which is
+// guaranteed to be executed on this (the UI) thread. It also provides the
+// CancelableRequestConsumer member, which cancels any outstanding requests upon
+// destruction.
+class PasswordStoreConsumer {
+ public:
+  // Call this when the request is finished. If there are no results, call it
+  // anyway with an empty vector.
+  virtual void OnPasswordStoreRequestDone(
+      CancelableRequestProvider::Handle handle,
+      const std::vector<webkit_glue::PasswordForm*>& result) = 0;
+
+  // The CancelableRequest framework needs both a callback (the
+  // PasswordStoreConsumer::OnPasswordStoreRequestDone) as well as a
+  // CancelableRequestConsumer.  This accessor makes the API simpler for callers
+  // as PasswordStore can get both from the PasswordStoreConsumer.
+  CancelableRequestConsumerBase* cancelable_consumer() {
+    return &cancelable_consumer_;
+  }
+
+ protected:
+  virtual ~PasswordStoreConsumer();
+
+ private:
+  CancelableRequestConsumer cancelable_consumer_;
+};
+
+#endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_CONSUMER_H_
diff --git a/chrome/browser/password_manager/password_store_default.cc b/chrome/browser/password_manager/password_store_default.cc
index c029e2c..2be5207 100644
--- a/chrome/browser/password_manager/password_store_default.cc
+++ b/chrome/browser/password_manager/password_store_default.cc
@@ -93,23 +93,20 @@ void PasswordStoreDefault::RemoveLoginsCreatedBetweenImpl(
 
 void PasswordStoreDefault::GetLoginsImpl(
     GetLoginsRequest* request, const webkit_glue::PasswordForm& form) {
-  std::vector<PasswordForm*> forms;
-  login_db_->GetLogins(form, &forms);
-  NotifyConsumer(request, forms);
+  login_db_->GetLogins(form, &request->value);
+  ForwardLoginsResult(request);
 }
 
 void PasswordStoreDefault::GetAutofillableLoginsImpl(
     GetLoginsRequest* request) {
-  std::vector<PasswordForm*> forms;
-  FillAutofillableLogins(&forms);
-  NotifyConsumer(request, forms);
+  FillAutofillableLogins(&request->value);
+  ForwardLoginsResult(request);
 }
 
 void PasswordStoreDefault::GetBlacklistLoginsImpl(
     GetLoginsRequest* request) {
-  std::vector<PasswordForm*> forms;
-  FillBlacklistLogins(&forms);
-  NotifyConsumer(request, forms);
+  FillBlacklistLogins(&request->value);
+  ForwardLoginsResult(request);
 }
 
 bool PasswordStoreDefault::FillAutofillableLogins(
diff --git a/chrome/browser/password_manager/password_store_default_unittest.cc b/chrome/browser/password_manager/password_store_default_unittest.cc
index 97f67b3..19b70a1 100644
--- a/chrome/browser/password_manager/password_store_default_unittest.cc
+++ b/chrome/browser/password_manager/password_store_default_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/password_manager/password_store_change.h"
+#include "chrome/browser/password_manager/password_store_consumer.h"
 #include "chrome/browser/password_manager/password_store_default.h"
 #include "chrome/browser/password_manager/password_form_data.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -38,7 +39,8 @@ namespace {
 class MockPasswordStoreConsumer : public PasswordStoreConsumer {
  public:
   MOCK_METHOD2(OnPasswordStoreRequestDone,
-               void(int, const std::vector<webkit_glue::PasswordForm*>&));
+               void(CancelableRequestProvider::Handle,
+                    const std::vector<webkit_glue::PasswordForm*>&));
 };
 
 class MockWebDataServiceConsumer : public WebDataServiceConsumer {
diff --git a/chrome/browser/password_manager/password_store_mac.cc b/chrome/browser/password_manager/password_store_mac.cc
index 2945032..ee1222c 100644
--- a/chrome/browser/password_manager/password_store_mac.cc
+++ b/chrome/browser/password_manager/password_store_mac.cc
@@ -886,7 +886,7 @@ void PasswordStoreMac::GetLoginsImpl(GetLoginsRequest* request,
   std::vector<PasswordForm*> database_forms;
   login_metadata_db_->GetLogins(form, &database_forms);
 
-  std::vector<PasswordForm*> merged_forms;
+  std::vector<PasswordForm*>& merged_forms = request->value;
   internal_keychain_helpers::MergePasswordForms(&keychain_forms,
                                                 &database_forms,
                                                 &merged_forms);
@@ -904,19 +904,17 @@ void PasswordStoreMac::GetLoginsImpl(GetLoginsRequest* request,
   RemoveDatabaseForms(database_forms);
   STLDeleteElements(&database_forms);
 
-  NotifyConsumer(request, merged_forms);
+  ForwardLoginsResult(request);
 }
 
 void PasswordStoreMac::GetBlacklistLoginsImpl(GetLoginsRequest* request) {
-  std::vector<PasswordForm*> database_forms;
-  FillBlacklistLogins(&database_forms);
-  NotifyConsumer(request, database_forms);
+  FillBlacklistLogins(&request->value);
+  ForwardLoginsResult(request);
 }
 
 void PasswordStoreMac::GetAutofillableLoginsImpl(GetLoginsRequest* request) {
-  std::vector<PasswordForm*> database_forms;
-  FillAutofillableLogins(&database_forms);
-  NotifyConsumer(request, database_forms);
+  FillAutofillableLogins(&request->value);
+  ForwardLoginsResult(request);
 }
 
 bool PasswordStoreMac::FillAutofillableLogins(
diff --git a/chrome/browser/password_manager/password_store_mac_unittest.cc b/chrome/browser/password_manager/password_store_mac_unittest.cc
index 1d2690a..3deabb3 100644
--- a/chrome/browser/password_manager/password_store_mac_unittest.cc
+++ b/chrome/browser/password_manager/password_store_mac_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/utf_string_conversions.h"
 #include "content/browser/browser_thread.h"
 #include "chrome/browser/keychain_mock_mac.h"
+#include "chrome/browser/password_manager/password_store_consumer.h"
 #include "chrome/browser/password_manager/password_store_mac.h"
 #include "chrome/browser/password_manager/password_store_mac_internal.h"
 #include "chrome/common/chrome_paths.h"
@@ -28,7 +29,8 @@ namespace {
 class MockPasswordStoreConsumer : public PasswordStoreConsumer {
 public:
   MOCK_METHOD2(OnPasswordStoreRequestDone,
-               void(int, const std::vector<webkit_glue::PasswordForm*>&));
+               void(CancelableRequestProvider::Handle,
+                    const std::vector<webkit_glue::PasswordForm*>&));
 };
 
 ACTION(STLDeleteElements0) {
diff --git a/chrome/browser/password_manager/password_store_win.cc b/chrome/browser/password_manager/password_store_win.cc
index 350ce8c..9adf0ab 100644
--- a/chrome/browser/password_manager/password_store_win.cc
+++ b/chrome/browser/password_manager/password_store_win.cc
@@ -36,14 +36,13 @@ int PasswordStoreWin::GetLogins(const webkit_glue::PasswordForm& form,
   return request_handle;
 }
 
-void PasswordStoreWin::NotifyConsumer(GetLoginsRequest* request,
-                                      const std::vector<PasswordForm*> forms) {
-  if (!forms.empty()) {
-    pending_request_forms_.erase(request->handle);
-    PasswordStore::NotifyConsumer(request, forms);
+void PasswordStoreWin::ForwardLoginsResult(GetLoginsRequest* request) {
+  if (!request->value.empty()) {
+    pending_request_forms_.erase(request->handle());
+    PasswordStore::ForwardLoginsResult(request);
   } else {
     PendingRequestFormMap::iterator it(pending_request_forms_.find(
-        request->handle));
+        request->handle()));
     if (it != pending_request_forms_.end()) {
       IE7PasswordInfo info;
       std::wstring url = ASCIIToWide(it->second.origin.spec());
@@ -69,16 +68,15 @@ void PasswordStoreWin::OnWebDataServiceRequestDone(
 
     // This is a response from WebDataService::GetIE7Login.
     PendingRequestFormMap::iterator it(pending_request_forms_.find(
-        request->handle));
+        request->handle()));
     DCHECK(pending_request_forms_.end() != it);
     PasswordForm* ie7_form = GetIE7Result(result, it->second);
 
-    std::vector<PasswordForm*> forms;
     if (ie7_form)
-      forms.push_back(ie7_form);
+      request->value.push_back(ie7_form);
 
     pending_request_forms_.erase(it);
-    PasswordStore::NotifyConsumer(request.release(), forms);
+    PasswordStore::ForwardLoginsResult(request.release());
   } else {
     PasswordStoreDefault::OnWebDataServiceRequestDone(handle, result);
   }
diff --git a/chrome/browser/password_manager/password_store_win.h b/chrome/browser/password_manager/password_store_win.h
index e58f717..cdde4a2 100644
--- a/chrome/browser/password_manager/password_store_win.h
+++ b/chrome/browser/password_manager/password_store_win.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -34,9 +34,7 @@ class PasswordStoreWin : public PasswordStoreDefault {
   void OnWebDataServiceRequestDone(WebDataService::Handle h,
                                    const WDTypedResult* result);
 
-  virtual void NotifyConsumer(
-      GetLoginsRequest* request,
-      const std::vector<webkit_glue::PasswordForm*> forms);
+  virtual void ForwardLoginsResult(GetLoginsRequest* request);
 
   // Takes ownership of |request| and tracks it under |handle|.
   void TrackRequest(WebDataService::Handle handle, GetLoginsRequest* request);
diff --git a/chrome/browser/password_manager/password_store_win_unittest.cc b/chrome/browser/password_manager/password_store_win_unittest.cc
index 42e7dc4..4031e7f 100644
--- a/chrome/browser/password_manager/password_store_win_unittest.cc
+++ b/chrome/browser/password_manager/password_store_win_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/time.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/browser/password_manager/password_form_data.h"
+#include "chrome/browser/password_manager/password_store_consumer.h"
 #include "chrome/browser/password_manager/password_store_win.h"
 #include "chrome/browser/password_manager/ie7_password.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -35,7 +36,8 @@ namespace {
 class MockPasswordStoreConsumer : public PasswordStoreConsumer {
  public:
   MOCK_METHOD2(OnPasswordStoreRequestDone,
-               void(int, const std::vector<webkit_glue::PasswordForm*>&));
+               void(CancelableRequestProvider::Handle,
+                    const std::vector<webkit_glue::PasswordForm*>&));
 };
 
 class MockWebDataServiceConsumer : public WebDataServiceConsumer {
diff --git a/chrome/browser/password_manager/password_store_x.cc b/chrome/browser/password_manager/password_store_x.cc
index 13ab8fb..7c3559f 100644
--- a/chrome/browser/password_manager/password_store_x.cc
+++ b/chrome/browser/password_manager/password_store_x.cc
@@ -99,45 +99,44 @@ void PasswordStoreX::RemoveLoginsCreatedBetweenImpl(
 }
 
 void PasswordStoreX::GetLoginsImpl(GetLoginsRequest* request,
-                                     const PasswordForm& form) {
+                                   const PasswordForm& form) {
   CheckMigration();
-  vector<PasswordForm*> forms;
-  if (use_native_backend() && backend_->GetLogins(form, &forms)) {
-    NotifyConsumer(request, forms);
+  if (use_native_backend() && backend_->GetLogins(form, &request->value)) {
+    ForwardLoginsResult(request);
     allow_fallback_ = false;
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetLoginsImpl(request, form);
   } else {
     // The consumer will be left hanging unless we reply.
-    NotifyConsumer(request, forms);
+    ForwardLoginsResult(request);
   }
 }
 
 void PasswordStoreX::GetAutofillableLoginsImpl(GetLoginsRequest* request) {
   CheckMigration();
-  vector<PasswordForm*> forms;
-  if (use_native_backend() && backend_->GetAutofillableLogins(&forms)) {
-    NotifyConsumer(request, forms);
+  if (use_native_backend() &&
+      backend_->GetAutofillableLogins(&request->value)) {
+    ForwardLoginsResult(request);
     allow_fallback_ = false;
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetAutofillableLoginsImpl(request);
   } else {
     // The consumer will be left hanging unless we reply.
-    NotifyConsumer(request, forms);
+    ForwardLoginsResult(request);
   }
 }
 
 void PasswordStoreX::GetBlacklistLoginsImpl(GetLoginsRequest* request) {
   CheckMigration();
-  vector<PasswordForm*> forms;
-  if (use_native_backend() && backend_->GetBlacklistLogins(&forms)) {
-    NotifyConsumer(request, forms);
+  if (use_native_backend() &&
+      backend_->GetBlacklistLogins(&request->value)) {
+    ForwardLoginsResult(request);
     allow_fallback_ = false;
   } else if (allow_default_store()) {
     PasswordStoreDefault::GetBlacklistLoginsImpl(request);
   } else {
     // The consumer will be left hanging unless we reply.
-    NotifyConsumer(request, forms);
+    ForwardLoginsResult(request);
   }
 }
 
diff --git a/chrome/browser/password_manager/password_store_x_unittest.cc b/chrome/browser/password_manager/password_store_x_unittest.cc
index df0778b..80eaa6a 100644
--- a/chrome/browser/password_manager/password_store_x_unittest.cc
+++ b/chrome/browser/password_manager/password_store_x_unittest.cc
@@ -36,7 +36,8 @@ namespace {
 class MockPasswordStoreConsumer : public PasswordStoreConsumer {
  public:
   MOCK_METHOD2(OnPasswordStoreRequestDone,
-               void(int, const std::vector<PasswordForm*>&));
+               void(CancelableRequestProvider::Handle,
+                    const std::vector<PasswordForm*>&));
 };
 
 class MockWebDataServiceConsumer : public WebDataServiceConsumer {