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..029b5abbbd0032454525a51d4b8d32e448c5a863 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;
 class Task;
 
 namespace browser_sync {
@@ -24,20 +24,65 @@ class PasswordModelAssociator;
 class PasswordModelWorker;
 };
 
+// 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:
-  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;
+      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_;
 };
 
 // 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:
+  static const Handle kInvalidHandle;

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

nit: Document this constant.

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

I'm not going to ask you to change this because it's extremely unlikely, but
consider the scenario where Handle changes to some object with state in the
future. You'd have to change all instances of kInvalidHandle to a Factory
method. If you create the factory method in the beginning, you won't have to
change the call sites for this case.

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

I created a factory method in CancelableRequestProvider.

Does the constant still need documentation if it's protected and there's a
factory method?

+
+  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 +114,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 17:53:11]

s/Consumer/PasswordStoreConsumer/

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

Done.

+  // 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();
@@ -97,6 +139,11 @@ class PasswordStore : public base::RefCountedThreadSafe<PasswordStore> {
   // Removes |observer| from the observer list.
   void RemoveObserver(Observer* observer);
 
+  // Since Handle is supposed to be opaque, provide validity method.
+  static bool handle_is_valid(Handle handle) {

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

This is not an accessor method; as such, the method should be CamelCase and
defined in the implementation file.

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

Done.

+    return handle != kInvalidHandle;
+  }
+
  protected:
   friend class base::RefCountedThreadSafe<PasswordStore>;
   friend class browser_sync::PasswordDataTypeController;
@@ -106,25 +153,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 +185,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 +215,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_;