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 d5212fae5107ec0e9948479184ddc13ba6073581..3f3e4665544a4e0c020e3c8541c4fc2a870cbcbb 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/observer_list.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;

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

Nice catch.

 class Task;
 
 namespace browser_sync {
@@ -24,20 +24,60 @@ class PasswordModelAssociator;
 class PasswordModelWorker;
 };
 
+// This abstract base class for consumers of PasswordStore calls that need

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

nit: This is quite a long sentence. Consider breaking it up for clarity.

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

Done.

+// asynchronous responses provides the CancelableRequestConsumer object which
+// takes care of canceling any outstanding requests, should the consumer be
+// destroyed before the response calls back.
 class PasswordStoreConsumer {

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

Move this class to its own header.

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

This is fairly tricky, due to PasswordStore providing PasswordStore::Handle.  As
I think about it, I'm wondering if the validity check should move into
CancelableRequestProvider::handle_is_valid (since the provider has the Handle
typedef anyway...  Could also fix its int next_handle_ to be Handle
next_handle_...

  public:
   virtual ~PasswordStoreConsumer() {}

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

Make the destructor protected.

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

Done.

   // 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;
+
+  // 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_;
+  }
+
+ 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;
+
+  // PasswordForm vector elements are meant to be owned (i.e. deleted) by the

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

nit: You don't need to clarify this: "i.e. deleted".

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

Done.

+  // PasswordStoreConsumer.  However, if the request is canceled after the

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

nit: Single space between sentences.

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

Done.

+  // 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 +109,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

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

Why have you made this ambiguous? Isn't it actually the PasswordStoreConsumer?

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

Nice catch - that was a search/replace artifact - I had this as
PasswordStore::Consumer at one point, but ended up preferring
PasswordStoreConsumer, and this comment stuck.  No semantic changes were
intended.

+  // 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 +143,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 +175,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 correct thread so

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

Which thread is the correct thread?

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

The original caller's thread (UI, in our case).

+  // the callback can be executed there using
+  // CancelableRequestProvider::ForwardResult.
+  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 +205,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_;