net: fix callbacks in DiskCacheBasedSSLHostInfo

net/http/disk_cache_based_ssl_host_info.h

 // Copyright (c) 2010 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 NET_HTTP_DISK_CACHE_BASED_SSL_HOST_INFO_H
 #define NET_HTTP_DISK_CACHE_BASED_SSL_HOST_INFO_H
 
 #include <string>
 
 #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"
 
 namespace net {
 
 class HttpCache;
 class IOBuffer;
 struct SSLConfig;
 
@@ skipping 28 matching lines @@
   int DoOpen();
   int DoRead();
   int DoCreate();
   int DoWrite();
 
   // WaitForDataReadyDone is the terminal state of the read operation.
   int WaitForDataReadyDone();
   // 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,
     OPEN,
     OPEN_COMPLETE,
     READ,
     READ_COMPLETE,
     WAIT_FOR_DATA_READY_DONE,
     CREATE,
     CREATE_COMPLETE,
     WRITE,
     WRITE_COMPLETE,
     SET_DONE,
     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_;
   std::string data_;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_DISK_CACHE_BASED_SSL_HOST_INFO_H