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

chrome/browser/password_manager/password_store.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/password_manager/password_store.h"
 
 #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) {
+const PasswordStore::Handle PasswordStore::kInvalidHandle = 0;

[James Hawkins, 2011/03/21 17:53:11]

Isn't Handle defined by CancelableRequest?  It seems strange that we're making
assumptions about the validity of a handle in a derived class. This is fragile.

[Sheridan Rawlins, 2011/03/21 18:36:42]

Yep, was thinking the same thing, but too tired to implement last night.  I
moved the constants, validity check, and InvalidHandle factory method to
CancelableRequest.

+
+PasswordStoreConsumer::~PasswordStoreConsumer() {
+}
+
+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() {
   ReportMetrics();
   return true;
 }
 
 void PasswordStore::AddLogin(const PasswordForm& form) {
   Task* task = NewRunnableMethod(this, &PasswordStore::AddLoginImpl, form);
   ScheduleTask(
@@ skipping 14 matching lines @@
 
 void PasswordStore::RemoveLoginsCreatedBetween(const base::Time& delete_begin,
                                                const base::Time& delete_end) {
   Task* task = NewRunnableMethod(this,
                                  &PasswordStore::RemoveLoginsCreatedBetweenImpl,
                                  delete_begin, delete_end);
   ScheduleTask(
       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;
-}
-
-void PasswordStore::CancelLoginsQuery(int handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  pending_requests_.erase(handle);
+PasswordStore::Handle PasswordStore::GetBlacklistLogins(
+    PasswordStoreConsumer* consumer) {
+  return Schedule(&PasswordStore::GetBlacklistLoginsImpl, consumer);
 }
 
 void PasswordStore::ReportMetrics() {
   ScheduleTask(NewRunnableMethod(this, &PasswordStore::ReportMetricsImpl));
 }
 
 void PasswordStore::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void PasswordStore::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(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::ForwardLoginsResult(GetLoginsRequest* request) {
+  request->ForwardResult(GetLoginsRequest::TupleType(request->handle(),
+                                                     request->value));
 }
 
-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);
+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();
 }
 
-int PasswordStore::GetNewRequestHandle() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  int handle = handle_++;
-  pending_requests_.insert(handle);
-  return 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();
 }
 
 void PasswordStore::WrapModificationTask(Task* task) {
 #if !defined(OS_MACOSX)
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 #endif  // !defined(OS_MACOSX)
 
   DCHECK(task);
   task->Run();
   delete task;
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &PasswordStore::NotifyLoginsChanged));
 }
 
 void PasswordStore::NotifyLoginsChanged() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   FOR_EACH_OBSERVER(Observer, observers_, OnLoginsChanged());
 }