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

chrome/browser/password_manager/password_store.h

Index: chrome/browser/password_manager/password_store.h
diff --git a/chrome/browser/password_manager/password_store.h b/chrome/browser/password_manager/password_store.h
index 32f9c81e4ca96bdc76c806a0e139e7af07c5c1fd..4360bb5bdc2d6d76b0875c0d48d22e11d753ea8b 100644
--- a/chrome/browser/password_manager/password_store.h
+++ b/chrome/browser/password_manager/password_store.h
@@ -6,15 +6,15 @@
 #define CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_H_
 #pragma once
 
-#include <set>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/ref_counted.h"
 #include "base/threading/thread.h"
 #include "base/time.h"
+#include "content/browser/cancelable_request.h"
 #include "webkit/glue/password_form.h"
 
-class Profile;
 class Task;
 
 namespace browser_sync {
@@ -29,14 +29,42 @@ class 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;
+      CancelableRequestProvider::Handle handle,
+      const std::vector<webkit_glue::PasswordForm*>& result) = 0;
+
+  CancelableRequestConsumerBase* cancelable_consumer() {

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

Comment?

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+    return &cancelable_consumer_;
+  }
+
+ private:
+  CancelableRequestConsumer cancelable_consumer_;
 };
 
 // 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;
+
+  // Destructor cleans up after canceled requests by deleting vector elements.

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

This doesn't seem like a description of the class...

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+  class GetLoginsRequest

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

This should probably be defined in the implementation file.

[brettw, 2011/03/17 21:40:34]

Why do you do this at all instead of just having a typedef. see
browser/history/history_marshaling.h

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

If I can change the type of |value| to a ScopedVector, then I can change this
_back_ to a typedef.  At the moment, as per the comment, the destructor is
needed to clean up after canceled requests.

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+      : public CancelableRequest1<GetLoginsCallback,
+                                  std::vector<webkit_glue::PasswordForm*> > {
+   public:
+    explicit GetLoginsRequest(GetLoginsCallback* callback)
+        : CancelableRequest1<GetLoginsCallback,
+        std::vector<webkit_glue::PasswordForm*> >(callback) {}

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

Fix indentation. Also, I believe having the empty constructor inline violates
our style (and may even be a clang error now).

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

Good point.  Again, if I change to typedef, this will become a non-issue.

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

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

I noticed the following addition was merged in recently:
    virtual ~Observer() {}

Are empty destructors allowed? 
http://www.chromium.org/developers/coding-style#TOC-Inline-functions would seem
to indicate otherwise...

+    virtual ~GetLoginsRequest();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(GetLoginsRequest);
+  };
+
   PasswordStore();
 
   // Reimplement this to add custom initialization. Always call this too.
@@ -56,24 +84,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 Consumer 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.
   virtual void ReportMetrics();
@@ -87,27 +112,34 @@ 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.
   virtual void ScheduleTask(Task* task);
 
+  template<typename BackendFunc>
+  Handle Schedule(

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

Don't add code to headers.

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

I referenced the HistoryService files when creating this.  If I can declare the
templated method and define it in the c++ class, I will.  If all the
BackendFunc's have the same signature, I can also do away with the
templatization.

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+      BackendFunc func, // Function to call on PasswordStore's thread.

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

I've never seen this style; please use the normal Chromium style to document
this method and its arguments.

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

I believe this also is referenced from HistoryService files.  I'll fix the style
in this file.

+      PasswordStoreConsumer* consumer) {
+    scoped_refptr<GetLoginsRequest> request(new GetLoginsRequest(

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

Why is the implementation inline?

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

Because of templating.  If this is defined in the implementation, then only the
implementation can use it.  I suppose that's fine as long as I ensure that its
private.

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+        NewCallback(consumer,
+                    &PasswordStoreConsumer::OnPasswordStoreRequestDone)));
+    AddRequest(request, consumer->cancelable_consumer());
+    ScheduleTask(NewRunnableMethod(this, func, request));
+    return request->handle();
+  }
+
+  template<typename BackendFunc, typename ArgA>
+  Handle Schedule(

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

Same concerns here.

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

Ditto above.  Will look into this.

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

+      BackendFunc func, // Function to call on PasswordStore's thread.
+      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();
+  }
+
   // These will be run in PasswordStore's own thread.
   // Synchronous implementation that reports usage metrics.
   virtual void ReportMetricsImpl() = 0;
@@ -137,29 +169,9 @@ 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);
+  virtual void ForwardLoginsResult(GetLoginsRequest* request);

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

Comment

[Sheridan Rawlins, 2011/03/20 08:13:11]

Done.

 
  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);
-
-  // Returns a new request handle tracked in pending_requests_.
-  int GetNewRequestHandle();
-
-  // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordStore);
 };