net: fix callbacks in DiskCacheBasedSSLHostInfo

net/http/disk_cache_based_ssl_host_info.h

Index: net/http/disk_cache_based_ssl_host_info.h
diff --git a/net/http/disk_cache_based_ssl_host_info.h b/net/http/disk_cache_based_ssl_host_info.h
index 1d53b909dffbc7ac822ea74b34bbc0cea20a87f1..b3c23bb13530197267ef735b5e1574554ad1ec27 100644
--- a/net/http/disk_cache_based_ssl_host_info.h
+++ b/net/http/disk_cache_based_ssl_host_info.h
@@ -10,6 +10,7 @@
 #include "base/lock.h"
 #include "base/non_thread_safe.h"
 #include "base/scoped_ptr.h"
+#include "base/weak_ptr.h"
 #include "net/base/completion_callback.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/socket/ssl_host_info.h"
@@ -58,6 +59,9 @@ class DiskCacheBasedSSLHostInfo : public SSLHostInfo,
   // SetDone is the terminal state of the write operation.
   int SetDone();
 
+  // IsCallbackPending returns true if we have a pending callback.
+  bool IsCallbackPending() const;
+
   enum State {
     GET_BACKEND,
     GET_BACKEND_COMPLETE,
@@ -74,15 +78,38 @@ class DiskCacheBasedSSLHostInfo : public SSLHostInfo,
     NONE,
   };
 
-  scoped_refptr<CancelableCompletionCallback<DiskCacheBasedSSLHostInfo> >
-      callback_;
+  class CallbackImpl : public CallbackRunner<Tuple1<int> > {

[willchan no longer on Chromium, 2010/12/15 00:03:14]

Nice solution!

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
says to move this to the top of the section.  You should probably move enum
State while you're at it.

[agl, 2010/12/15 16:28:08]

Done.

+   public:
+    CallbackImpl(const base::WeakPtr<DiskCacheBasedSSLHostInfo>& obj,
+                 void (DiskCacheBasedSSLHostInfo::*meth) (int))
+        : obj_(obj),
+          meth_(meth) {
+    }
+
+    virtual void RunWithParams(const Tuple1<int>& params) {
+      if (!obj_) {
+        delete this;
+      } else {
+        DispatchToMethod(obj_.get(), meth_, params);
+      }
+    }
+
+    void* user_data;

[willchan no longer on Chromium, 2010/12/15 00:03:14]

I see why you've done this, but this is also against the style guide and ugly
due to the casting.  I suggest you template your CallbackImpl, so you can
specify this user_data.  And have a |backend_callback_| and |entry_callback_|
for each type.  Make user_data private and provide an accessor to its address. 
I wish we could just cancel the request =/

[agl, 2010/12/15 16:28:08]

Rather than have the complexity of two callbacks (and figuring out which needs
to be deleted etc), I've added both a disk_cache::Entry and disk_cache::Backend
pointer, with accessors. Let me know if that works for you.

+
+   protected:
+    base::WeakPtr<DiskCacheBasedSSLHostInfo> obj_;
+    void (DiskCacheBasedSSLHostInfo::*meth_) (int);
+  };
+
+  base::WeakPtrFactory<DiskCacheBasedSSLHostInfo> weak_ptr_factory_;
+  CallbackImpl* callback_;
   State state_;
   bool ready_;
   std::string new_data_;
   const std::string hostname_;
   HttpCache* const http_cache_;
   disk_cache::Backend* backend_;

[rvargas (doing something else), 2010/12/15 02:00:16]

Interesting. How do we make sure that this pointer is still valid when we try to
use it (as the backend is owned by the http cache).

Also, how does one of these entries look from the about:cache page? I'm guessing
that hostname looks like "www.google.com", so that means that now we store
things that don't start with the protocol.

[willchan no longer on Chromium, 2010/12/15 02:14:25]

The HttpCache hands out SSLHostInfos to the HttpNetworkLayer and beyond.  When
~HttpCache runs, it'll kill HttpNetworkLayer which should transitively kill
everything else.  Of course, if something's getting leaked, then yeah, this is
problematic.  But theoretically we shouldn't have leaks.

I need to think at some point if there's a better way to establish ownership for
disk_cache::Backend*, so people don't acquire raw pointers to something that
they shouldn't.

-  disk_cache::Entry *entry_;
+  disk_cache::Entry* entry_;
   CompletionCallback* user_callback_;
   scoped_refptr<net::IOBuffer> read_buffer_;
   scoped_refptr<net::IOBuffer> write_buffer_;