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

chrome/browser/password_manager/password_store.cc

Index: chrome/browser/password_manager/password_store.cc
diff --git a/chrome/browser/password_manager/password_store.cc b/chrome/browser/password_manager/password_store.cc
index 80380928d18ad8c4f9ead4dc09ce2e7bc93911e6..e91806998e14ec89094a5e780d95ba3d75ba1717 100644
--- a/chrome/browser/password_manager/password_store.cc
+++ b/chrome/browser/password_manager/password_store.cc
@@ -6,13 +6,14 @@
 
 #include "base/message_loop.h"
 #include "base/scoped_ptr.h"
+#include "base/stl_util-inl.h"
 #include "base/task.h"
 #include "content/browser/browser_thread.h"
 
 using std::vector;
 using webkit_glue::PasswordForm;
 
-PasswordStore::PasswordStore() : handle_(0) {
+PasswordStore::PasswordStore() {
 }
 
 bool PasswordStore::Init() {
@@ -47,36 +48,24 @@ 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;
+PasswordStore::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;
+PasswordStore::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;
+PasswordStore::Handle PasswordStore::GetBlacklistLogins(
+    PasswordStoreConsumer* consumer) {
+  return Schedule(&PasswordStore::GetBlacklistLoginsImpl, consumer);
 }
 
-void PasswordStore::CancelLoginsQuery(int handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  pending_requests_.erase(handle);
+void PasswordStore::ForwardLoginsResult(GetLoginsRequest* request) {

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

Method ordering: ForwardLoginsResult() implementation should be  after
ScheduleTask() implementation.

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

Done.

+  request->ForwardResult(GetLoginsRequest::TupleType(request->handle(),
+                                                     request->value));
 }
 
 void PasswordStore::ReportMetrics() {
@@ -93,50 +82,41 @@ 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));
+template<typename BackendFunc>
+PasswordStore::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();
 }
 
-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);
+template<typename BackendFunc, typename ArgA>
+PasswordStore::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();
+}
 
-  consumer->OnPasswordStoreRequestDone(handle, forms);
+PasswordStore::GetLoginsRequest::GetLoginsRequest(GetLoginsCallback* callback)

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

The ordering of the methods in the source file should match the ordering in the
header.  These two methods should be placed above the PasswordStore constructor
implementation.

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

Done.

+    : CancelableRequest1<GetLoginsCallback,
+                         std::vector<webkit_glue::PasswordForm*> >(callback) {
 }
 
-int PasswordStore::GetNewRequestHandle() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  int handle = handle_++;
-  pending_requests_.insert(handle);
-  return handle;
+PasswordStore::GetLoginsRequest::~GetLoginsRequest() {
+  if (canceled()) {
+    STLDeleteElements(&value);
+  }
 }
 
 void PasswordStore::WrapModificationTask(Task* task) {