Convert DiskCacheBasedSSLHostInfo to base::Bind().

This CL introduces a small hack to create a DataShim that allows the target of the object to be deleted while an in-flight callback is still outstanding for it.

BUG=none
TEST=existing

Review URL: http://codereview.chromium.org/9025001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@115600 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 49 insertions, 83 deletions

diff --git a/net/http/disk_cache_based_ssl_host_info.cc b/net/http/disk_cache_based_ssl_host_info.cc
index 46a557f..e99c9b0 100644
--- a/net/http/disk_cache_based_ssl_host_info.cc
+++ b/net/http/disk_cache_based_ssl_host_info.cc
@@ -15,25 +15,32 @@
 
 namespace net {
 
-DiskCacheBasedSSLHostInfo::CallbackImpl::CallbackImpl(
-    const base::WeakPtr<DiskCacheBasedSSLHostInfo>& obj,
-    void (DiskCacheBasedSSLHostInfo::*meth)(int))
-    : obj_(obj),
-      meth_(meth),
-      backend_(NULL),
-      entry_(NULL) {
-}
-
-DiskCacheBasedSSLHostInfo::CallbackImpl::~CallbackImpl() {}
-
-void DiskCacheBasedSSLHostInfo::CallbackImpl::RunWithParams(
-    const Tuple1<int>& params) {
-  if (!obj_) {
-    delete this;
-  } else {
-    DispatchToMethod(obj_.get(), meth_, params);
-  }
-}
+// Some APIs inside disk_cache take a handle that the caller must keep alive
+// until the API has finished its asynchronous execution.
+//
+// Unfortunately, DiskCacheBasedSSLHostInfo may be deleted before the
+// operation completes causing a use-after-free.
+//
+// This data shim struct is meant to provide a location for the disk_cache
+// APIs to write into even if the originating DiskCacheBasedSSLHostInfo
+// object has been deleted.  The lifetime for instances of this struct
+// should be bound to the CompletionCallback that is passed to the disk_cache
+// API.  We do this by binding an instance of this struct to an unused
+// parameter for OnIOComplete() using base::Owned().
+//
+// This is a hack. A better fix is to make it so that the disk_cache APIs
+// take a Callback to a mutator for setting the output value rather than
+// writing into a raw handle. Then the caller can just pass in a Callback
+// bound to WeakPtr for itself. This callback would correct "no-op" itself
+// when the DiskCacheBasedSSLHostInfo object is deleted.
+//
+// TODO(ajwong): Change disk_cache's API to return results via Callback.
+struct DiskCacheBasedSSLHostInfo::CacheOperationDataShim {
+  CacheOperationDataShim() : backend(NULL), entry(NULL) {}
+
+  disk_cache::Backend* backend;
+  disk_cache::Entry* entry;
+};
 
 DiskCacheBasedSSLHostInfo::DiskCacheBasedSSLHostInfo(
     const std::string& hostname,
@@ -42,8 +49,11 @@ DiskCacheBasedSSLHostInfo::DiskCacheBasedSSLHostInfo(
     HttpCache* http_cache)
     : SSLHostInfo(hostname, ssl_config, cert_verifier),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      callback_(new CallbackImpl(weak_factory_.GetWeakPtr(),
-                                 &DiskCacheBasedSSLHostInfo::OnIOComplete)),
+      data_shim_(new CacheOperationDataShim()),
+      io_callback_(
+          base::Bind(&DiskCacheBasedSSLHostInfo::OnIOComplete,
+                     weak_factory_.GetWeakPtr(),
+                     base::Owned(data_shim_))),  // Ownership assigned.
       state_(GET_BACKEND),
       ready_(false),
       found_entry_(false),
@@ -95,15 +105,14 @@ DiskCacheBasedSSLHostInfo::~DiskCacheBasedSSLHostInfo() {
   DCHECK(user_callback_.is_null());
   if (entry_)
     entry_->Close();
-  if (!IsCallbackPending())
-    delete callback_;
 }
 
 std::string DiskCacheBasedSSLHostInfo::key() const {
   return "sslhostinfo:" + hostname_;
 }
 
-void DiskCacheBasedSSLHostInfo::OnIOComplete(int rv) {
+void DiskCacheBasedSSLHostInfo::OnIOComplete(
+    CacheOperationDataShim* unused, int rv) {
   rv = DoLoop(rv);
   if (rv != ERR_IO_PENDING && !user_callback_.is_null()) {
     CompletionCallback callback = user_callback_;
@@ -162,7 +171,7 @@ int DiskCacheBasedSSLHostInfo::DoLoop(int rv) {
 
 int DiskCacheBasedSSLHostInfo::DoGetBackendComplete(int rv) {
   if (rv == OK) {
-    backend_ = callback_->backend();
+    backend_ = data_shim_->backend,
     state_ = OPEN;
   } else {
     state_ = WAIT_FOR_DATA_READY_DONE;
@@ -172,7 +181,7 @@ int DiskCacheBasedSSLHostInfo::DoGetBackendComplete(int rv) {
 
 int DiskCacheBasedSSLHostInfo::DoOpenComplete(int rv) {
   if (rv == OK) {
-    entry_ = callback_->entry();
+    entry_ = data_shim_->entry;
     state_ = READ;
     found_entry_ = true;
   } else {
@@ -199,7 +208,7 @@ int DiskCacheBasedSSLHostInfo::DoCreateOrOpenComplete(int rv) {
   if (rv != OK) {
     state_ = SET_DONE;
   } else {
-    entry_ = callback_->entry();
+    entry_ = data_shim_->entry;
     state_ = WRITE;
   }
   return OK;
@@ -207,16 +216,12 @@ int DiskCacheBasedSSLHostInfo::DoCreateOrOpenComplete(int rv) {
 
 int DiskCacheBasedSSLHostInfo::DoGetBackend() {
   state_ = GET_BACKEND_COMPLETE;
-  return http_cache_->GetBackend(
-      callback_->backend_pointer(),
-      base::Bind(&net::OldCompletionCallbackAdapter, callback_));
+  return http_cache_->GetBackend(&data_shim_->backend, io_callback_);
 }
 
 int DiskCacheBasedSSLHostInfo::DoOpen() {
   state_ = OPEN_COMPLETE;
-  return backend_->OpenEntry(
-      key(), callback_->entry_pointer(),
-      base::Bind(&net::OldCompletionCallbackAdapter, callback_));
+  return backend_->OpenEntry(key(), &data_shim_->entry, io_callback_);
 }
 
 int DiskCacheBasedSSLHostInfo::DoRead() {
@@ -229,8 +234,7 @@ int DiskCacheBasedSSLHostInfo::DoRead() {
   read_buffer_ = new IOBuffer(size);
   state_ = READ_COMPLETE;
   return entry_->ReadData(
-      0 /* index */, 0 /* offset */, read_buffer_, size,
-      base::Bind(&net::OldCompletionCallbackAdapter, callback_));
+      0 /* index */, 0 /* offset */, read_buffer_, size, io_callback_);
 }
 
 int DiskCacheBasedSSLHostInfo::DoWrite() {
@@ -240,22 +244,17 @@ int DiskCacheBasedSSLHostInfo::DoWrite() {
 
   return entry_->WriteData(
       0 /* index */, 0 /* offset */, write_buffer_, new_data_.size(),
-      base::Bind(&net::OldCompletionCallbackAdapter, callback_),
-      true /* truncate */);
+      io_callback_, true /* truncate */);
 }
 
 int DiskCacheBasedSSLHostInfo::DoCreateOrOpen() {
   DCHECK(entry_ == NULL);
   state_ = CREATE_OR_OPEN_COMPLETE;
   if (found_entry_) {
-    return backend_->OpenEntry(
-        key(), callback_->entry_pointer(),
-        base::Bind(&net::OldCompletionCallbackAdapter, callback_));
+    return backend_->OpenEntry(key(), &data_shim_->entry, io_callback_);
   }
 
-  return backend_->CreateEntry(
-      key(), callback_->entry_pointer(),
-      base::Bind(&net::OldCompletionCallbackAdapter, callback_));
+  return backend_->CreateEntry(key(), &data_shim_->entry, io_callback_);
 }
 
 int DiskCacheBasedSSLHostInfo::DoWaitForDataReadyDone() {
@@ -279,17 +278,4 @@ int DiskCacheBasedSSLHostInfo::DoSetDone() {
   return OK;
 }
 
-bool DiskCacheBasedSSLHostInfo::IsCallbackPending() const {
-  switch (state_) {
-    case GET_BACKEND_COMPLETE:
-    case OPEN_COMPLETE:
-    case READ_COMPLETE:
-    case CREATE_OR_OPEN_COMPLETE:
-    case WRITE_COMPLETE:
-      return true;
-    default:
-      return false;
-  }
-}
-
 }  // namespace net
diff --git a/net/http/disk_cache_based_ssl_host_info.h b/net/http/disk_cache_based_ssl_host_info.h
index 5fd2be80..72c256f 100644
--- a/net/http/disk_cache_based_ssl_host_info.h
+++ b/net/http/disk_cache_based_ssl_host_info.h
@@ -38,6 +38,7 @@ class NET_EXPORT_PRIVATE DiskCacheBasedSSLHostInfo
   virtual void Persist() OVERRIDE;
 
  private:
+  struct CacheOperationDataShim;
   enum State {
     GET_BACKEND,
     GET_BACKEND_COMPLETE,
@@ -54,33 +55,14 @@ class NET_EXPORT_PRIVATE DiskCacheBasedSSLHostInfo
     NONE,
   };
 
-  class CallbackImpl : public CallbackRunner<Tuple1<int> > {
-   public:
-    CallbackImpl(const base::WeakPtr<DiskCacheBasedSSLHostInfo>& obj,
-                 void (DiskCacheBasedSSLHostInfo::*meth)(int));
-    virtual ~CallbackImpl();
-
-    disk_cache::Backend** backend_pointer() { return &backend_; }
-    disk_cache::Entry** entry_pointer() { return &entry_; }
-    disk_cache::Backend* backend() const { return backend_; }
-    disk_cache::Entry* entry() const { return entry_; }
-
-    // CallbackRunner<Tuple1<int> >:
-    virtual void RunWithParams(const Tuple1<int>& params) OVERRIDE;
-
-   private:
-    base::WeakPtr<DiskCacheBasedSSLHostInfo> obj_;
-    void (DiskCacheBasedSSLHostInfo::*meth_)(int);
-
-    disk_cache::Backend* backend_;
-    disk_cache::Entry* entry_;
-  };
-
   virtual ~DiskCacheBasedSSLHostInfo();
 
   std::string key() const;
 
-  void OnIOComplete(int rv);
+  // The |unused| parameter is a small hack so that we can have the
+  // CacheOperationDataShim object owned by the Callback that is created for
+  // this method.  See comment above CacheOperationDataShim for details.
+  void OnIOComplete(CacheOperationDataShim* unused, int rv);
 
   int DoLoop(int rv);
 
@@ -102,11 +84,9 @@ class NET_EXPORT_PRIVATE DiskCacheBasedSSLHostInfo
   // DoSetDone is the terminal state of the write operation.
   int DoSetDone();
 
-  // IsCallbackPending returns true if we have a pending callback.
-  bool IsCallbackPending() const;
-
   base::WeakPtrFactory<DiskCacheBasedSSLHostInfo> weak_factory_;
-  CallbackImpl* callback_;
+  CacheOperationDataShim* data_shim_;  // Owned by |io_callback_|.
+  CompletionCallback io_callback_;
   State state_;
   bool ready_;
   bool found_entry_;  // Controls the behavior of DoCreateOrOpen.