net: fix callbacks in DiskCacheBasedSSLHostInfo

net: fix callbacks in DiskCacheBasedSSLHostInfo

Previously, DiskCacheBasedSSLHostInfo had a couple of problems:
  * I had assumed that the disk cache was taking references to the
    callback. It wasn't.
  * Even if it were, I was passing pointers into
    DiskCacheBasedSSLHostInfo which needed to be live for the duration
    of the callback.

This change switches to a custom callback class which doesn't need
reference counting and which contains a |user_data| pointer member which
remains live for the duration of the callback.

BUG=63867
TEST=Navigate to an HTTPS page with Snap Start running and no network connection.

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

LGTM, although I have to insist you not use void* and casts and public data
members.  I'd much prefer templates and an accessor which returned the address. 
Thought I think we should simply fix disk_cache::Backend's interface.

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.cc (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.cc:97: switch (state_) {
indentation is off for these cases.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:81: class CallbackImpl : public
CallbackRunner<Tuple1<int> > {
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.

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:97: void* user_data;
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 =/

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

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:111: disk_cache::Backend* backend_;
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]

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:111: disk_cache::Backend* backend_;
On 2010/12/15 02:00:16, rvargas wrote:
> 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).

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.

> 
> 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.

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

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.cc (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.cc:97: switch (state_) {
On 2010/12/15 00:03:14, willchan wrote:
> indentation is off for these cases.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

Done.

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:81: class CallbackImpl : public
CallbackRunner<Tuple1<int> > {
On 2010/12/15 00:03:14, willchan wrote:
> 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.

Done.

http://codereview.chromium.org/5826001/diff/1/net/http/disk_cache_based_ssl_h...
net/http/disk_cache_based_ssl_host_info.h:97: void* user_data;
On 2010/12/15 00:03:14, willchan wrote:
> 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 =/

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.

[agl, 2010-12-15 16:29:58]

On Tue, Dec 14, 2010 at 9:00 PM,  <rvargas@chromium.org> wrote:
> 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.

The keys look like "sslhostinfo:www.google.com". If there's something
better that I should do, please let me know. This code isn't live yet
so I'd rather make changes now.

Maybe filter out sslhostinfo in about:cache?


AGL

[rvargas (doing something else), 2010-12-15 18:49:26]

On 2010/12/15 16:29:58, agl wrote:
> On Tue, Dec 14, 2010 at 9:00 PM,  <mailto:rvargas@chromium.org> wrote:
> > 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.
> 
> The keys look like "sslhostinfo:www.google.com". If there's something
> better that I should do, please let me know. This code isn't live yet
> so I'd rather make changes now.
> 
> Maybe filter out sslhostinfo in about:cache?
> 
> 
> AGL

On 2010/12/15 16:29:58, agl wrote:
> On Tue, Dec 14, 2010 at 9:00 PM,  <mailto:rvargas@chromium.org> wrote:
> > 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.
> 
> The keys look like "sslhostinfo:www.google.com". If there's something
> better that I should do, please let me know. This code isn't live yet
> so I'd rather make changes now.
> 
> Maybe filter out sslhostinfo in about:cache?
> 
> 
> AGL

No, that's good: there is a clear way to identify these entries, and I think it
is better to be able to see everything from about cache.

Regarding the raw pointers that are stored here, I'm happy with the explanation;
as long as we know that everything should be deleted synchronously with the
cache, everything's fine.

[willchan no longer on Chromium, 2010-12-16 00:12:26]

LGTM

http://codereview.chromium.org/5826001/diff/8001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/5826001/diff/8001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.h:79: protected:
private