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

chrome/browser/password_manager/password_store.h

 // 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.
 
 #ifndef CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_H_
 #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 {
 class PasswordDataTypeController;
 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.
   class Observer {
    public:
     // Notifies the observer that password data changed in some way.
     virtual void OnLoginsChanged() = 0;
 
    protected:
     virtual ~Observer() {}
@@ skipping 11 matching lines @@
   void UpdateLogin(const webkit_glue::PasswordForm& form);
 
   // Removes the matching PasswordForm from the secure password store (async).
   void RemoveLogin(const webkit_glue::PasswordForm& form);
 
   // Removes all logins created in the given date range.
   void RemoveLoginsCreatedBetween(const base::Time& delete_begin,
                                   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();
 
   // Adds an observer to be notified when the password store data changes.
   void AddObserver(Observer* observer);
 
   // 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;
   friend class browser_sync::PasswordModelAssociator;
   friend class browser_sync::PasswordModelWorker;
   friend class LivePasswordsSyncTest;
 
   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.
   // Synchronous implementation that reports usage metrics.
   virtual void ReportMetricsImpl() = 0;
   // Synchronous implementation to add the given login.
   virtual void AddLoginImpl(const webkit_glue::PasswordForm& form) = 0;
   // Synchronous implementation to update the given login.
   virtual void UpdateLoginImpl(const webkit_glue::PasswordForm& form) = 0;
   // Synchronous implementation to remove the given login.
@@ skipping 11 matching lines @@
   // Finds all blacklist PasswordForms, and notifies the consumer.
   virtual void GetBlacklistLoginsImpl(GetLoginsRequest* request) = 0;
 
   // Finds all non-blacklist PasswordForms, and fills the vector.
   virtual bool FillAutofillableLogins(
       std::vector<webkit_glue::PasswordForm*>* forms) = 0;
   // Finds all blacklist PasswordForms, and fills the vector.
   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);
+
+  // 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);
+
+  // 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:
-  // 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();
-
   // 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
   // NotifyLoginsChanged(). Note that there is no guarantee that the called
   // method will actually modify the password store data. |task| may not be
   // NULL. This method owns and will delete |task|.
   void WrapModificationTask(Task* task);
 
   // Called by WrapModificationTask() once the underlying data-modifying
   // operation has been performed. Notifies observers that password store data
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordStore);
 };
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE_H_