Fix NetLog thread safety issue

Fix NetLog thread safety issue introduced in
http://codereview.chromium.org/10539094/.

We weren't holding on to a reference for an
x509Certificate passed to another thread for logging.

BUG=126243
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141812

[Ryan Sleevi, 2012-06-12 20:22:09]

The only one that doesn't need base::Unretained() is SSLClientSocketNSS. So this
change adds additional AddRef/Release's to the X509Certificate* that aren't
strictly necessary.

I'm not sure that this is strictly better, and I'd rather have us come to a good
solution as far as thread-safety and NetLog and the "Right Way" to use it before
changing all the call sites.

As a temporary fix for the real race though, with the alternative being
reverting all of the changes, this LGTM, although my own view is it might be
'better' to limit the scope of the changes until we know the solution.

http://codereview.chromium.org/10534117/diff/7002/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.cc (left):

http://codereview.chromium.org/10534117/diff/7002/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.cc:269:
base::Unretained(worker_->certificate())));
This was safe as-is. Start() has not yet been called on |worker| (line 389), and
remains valid until HandleResult is called (line 201/205).

A temporary scoped_refptr<> would be created, since it's implicitly
constructable, so the change on 155 isn't necessary (and avoids one extra
AddRef/Release cycle)

http://codereview.chromium.org/10534117/diff/7002/net/base/x509_certificate_n...
File net/base/x509_certificate_net_log_param.h (right):

http://codereview.chromium.org/10534117/diff/7002/net/base/x509_certificate_n...
net/base/x509_certificate_net_log_param.h:19: scoped_refptr<const
X509Certificate> certificate);
const scoped_refptr<>&

Only scoped_ptr<>'s should be treated as movable - this avoids an extra
AddRef/Release on a temporary for the copy-by-value.

[mmenke, 2012-06-12 20:37:32]

Switched to just keeping a const ref where it's needed.  I'd switched all
instances to using refcounted because I was thinking having a single use case of
the function makes life a little simpler.

On 2012/06/12 20:22:09, Ryan Sleevi wrote:
> The only one that doesn't need base::Unretained() is SSLClientSocketNSS. So
this
> change adds additional AddRef/Release's to the X509Certificate* that aren't
> strictly necessary.
> 
> I'm not sure that this is strictly better, and I'd rather have us come to a
good
> solution as far as thread-safety and NetLog and the "Right Way" to use it
before
> changing all the call sites.
> 
> As a temporary fix for the real race though, with the alternative being
> reverting all of the changes, this LGTM, although my own view is it might be
> 'better' to limit the scope of the changes until we know the solution.
> 
>
http://codereview.chromium.org/10534117/diff/7002/net/base/multi_threaded_cer...
> File net/base/multi_threaded_cert_verifier.cc (left):
> 
>
http://codereview.chromium.org/10534117/diff/7002/net/base/multi_threaded_cer...
> net/base/multi_threaded_cert_verifier.cc:269:
> base::Unretained(worker_->certificate())));
> This was safe as-is. Start() has not yet been called on |worker| (line 389),
and
> remains valid until HandleResult is called (line 201/205).
> 
> A temporary scoped_refptr<> would be created, since it's implicitly
> constructable, so the change on 155 isn't necessary (and avoids one extra
> AddRef/Release cycle)
> 
>
http://codereview.chromium.org/10534117/diff/7002/net/base/x509_certificate_n...
> File net/base/x509_certificate_net_log_param.h (right):
> 
>
http://codereview.chromium.org/10534117/diff/7002/net/base/x509_certificate_n...
> net/base/x509_certificate_net_log_param.h:19: scoped_refptr<const
> X509Certificate> certificate);
> const scoped_refptr<>&
> 
> Only scoped_ptr<>'s should be treated as movable - this avoids an extra
> AddRef/Release on a temporary for the copy-by-value.

[Ryan Sleevi, 2012-06-12 21:33:26]

nack.

http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2536:
base::ConstRef(nss_handshake_state_.server_cert));
This is still wrong.

ConstRefWrapper stores a ConstRef, rather than taking a copy/ownership.

If you were to use base::ConstRef, then you would have kept the change as you
originally had it, added base::ConstRef to every other call, and kept this one
as a non-const-ref.

base::ConstRef is more or less base::Unretained(), but for ref-args rather than
pointer args.

[mmenke, 2012-06-12 21:39:27]

On 2012/06/12 21:33:26, Ryan Sleevi wrote:
> nack.
> 
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> File net/socket/ssl_client_socket_nss.cc (right):
> 
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> net/socket/ssl_client_socket_nss.cc:2536:
> base::ConstRef(nss_handshake_state_.server_cert));
> This is still wrong.
> 
> ConstRefWrapper stores a ConstRef, rather than taking a copy/ownership.
> 
> If you were to use base::ConstRef, then you would have kept the change as you
> originally had it, added base::ConstRef to every other call, and kept this one
> as a non-const-ref.
> 
> base::ConstRef is more or less base::Unretained(), but for ref-args rather
than
> pointer args.

Then what do I use?  Do I have to add a function wrapper?  I've never found the
documentation of the callback system very clear.

[mmenke, 2012-06-12 21:45:27]

On 2012/06/12 21:39:27, Matt Menke wrote:
> On 2012/06/12 21:33:26, Ryan Sleevi wrote:
> > nack.
> > 
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > File net/socket/ssl_client_socket_nss.cc (right):
> > 
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > net/socket/ssl_client_socket_nss.cc:2536:
> > base::ConstRef(nss_handshake_state_.server_cert));
> > This is still wrong.
> > 
> > ConstRefWrapper stores a ConstRef, rather than taking a copy/ownership.
> > 
> > If you were to use base::ConstRef, then you would have kept the change as
you
> > originally had it, added base::ConstRef to every other call, and kept this
one
> > as a non-const-ref.
> > 
> > base::ConstRef is more or less base::Unretained(), but for ref-args rather
> than
> > pointer args.
> 
> Then what do I use?  Do I have to add a function wrapper?  I've never found
the
> documentation of the callback system very clear.

I know that if it was a method, I could just use "Bind(&Blah::whatever, *this)"
for this file, and "Bind(&Blah::whatever, base::Unretained(this))" in the other.
 I had assumed there'd be some analog in the refcounted argument case.  I guess
there isn't?

[Ryan Sleevi, 2012-06-12 21:56:17]

On 2012/06/12 21:39:27, Matt Menke wrote:
> On 2012/06/12 21:33:26, Ryan Sleevi wrote:
> > nack.
> > 
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > File net/socket/ssl_client_socket_nss.cc (right):
> > 
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > net/socket/ssl_client_socket_nss.cc:2536:
> > base::ConstRef(nss_handshake_state_.server_cert));
> > This is still wrong.
> > 
> > ConstRefWrapper stores a ConstRef, rather than taking a copy/ownership.
> > 
> > If you were to use base::ConstRef, then you would have kept the change as
you
> > originally had it, added base::ConstRef to every other call, and kept this
one
> > as a non-const-ref.
> > 
> > base::ConstRef is more or less base::Unretained(), but for ref-args rather
> than
> > pointer args.
> 
> Then what do I use?  Do I have to add a function wrapper?  I've never found
the
> documentation of the callback system very clear.

Nothing.

Just

base::Bind(&NetLogX509CertificateChainCallback,
nss_handshake_state_.server_cert);

Assuming my template foo is correct (sorta hard, given the pump-generated nature
of the BindState), for P1 args, base::Bind will auto-refcount. However, even
more, since typeof (nss_handshake_state_.server_cert) is a scoped_refptr<>, when
base::Bind evalutes the base::internal::CallbackParamTraits<P1>::StorageType
(not to be confused with the base::internal::BoundFunctorTraits::A1Type), it'll
store a scoped_refptr<>.

AIUI, There's two types of introspection that happens - one based upon the
typenames you base to base::Bind, one based on the typenames extracted from the
Functor you've bound too. As long as the Bound typename is convertable to the
Functor typename, it should work.

The easiest way to test if I'm wrong in this is to compile on Linux. If
base::Bind (bind.h line 78) is evaluating P1 to be a "X509Certificate*" (the
Functor param trait) instead of a "const scoped_refptr<X509Certificate>&" (the
Bound template arg trait), then you should get a compile error on bind.h line
102.

If you don't get an error, then it works and you're safe.
If you do get an error then:
- Option 1) The function signature will need to be changed to "const
scoped_refptr<X509Certificate>&" and all the no-copy operations rewritten to use
base::ConstRef()
- Option 2) use base::Owned(new scoped_refptr<X509Certificate>(...)); to heap
allocate a scoped_refptr that will be destroyed post callback
[Larger design work: eg, not serious for this change, but presented for
completness]
- Option 3) Separate out the FunctorParamTraits and the CallbackParamTraits so
that callers can indicate whether or not they're intending to pass a pointer
(eg: what base::Unretained() is supposed to do)
- Option 4) Make scoped_refptr<> a pseudo-passable type, so that when you
actually do want Bind to store a scoped_refptr<> even when the signature takes a
naked pointer, then the caller passes base::Passed() instead (eg: the inverse of
Option 3)

But I think you should just get away with binding to the argument itself, with
no bind helpers involved :-)

FWIW, callback_internal.h, bind_internal.h, and bind_helpers.h are all pretty
good documentation. As callback.h mentions, all arguments are automatically
ref-counted UNLESS base::Unretained (documented in bind_helpers.h) is used

[mmenke, 2012-06-12 22:21:37]

On 2012/06/12 21:56:17, Ryan Sleevi wrote:
> On 2012/06/12 21:39:27, Matt Menke wrote:
> > On 2012/06/12 21:33:26, Ryan Sleevi wrote:
> > > nack.
> > > 
> > >
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > > File net/socket/ssl_client_socket_nss.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10534117/diff/4018/net/socket/ssl_client_socke...
> > > net/socket/ssl_client_socket_nss.cc:2536:
> > > base::ConstRef(nss_handshake_state_.server_cert));
> > > This is still wrong.
> > > 
> > > ConstRefWrapper stores a ConstRef, rather than taking a copy/ownership.
> > > 
> > > If you were to use base::ConstRef, then you would have kept the change as
> you
> > > originally had it, added base::ConstRef to every other call, and kept this
> one
> > > as a non-const-ref.
> > > 
> > > base::ConstRef is more or less base::Unretained(), but for ref-args rather
> > than
> > > pointer args.
> > 
> > Then what do I use?  Do I have to add a function wrapper?  I've never found
> the
> > documentation of the callback system very clear.
> 
> Nothing.
> 
> Just
> 
> base::Bind(&NetLogX509CertificateChainCallback,
> nss_handshake_state_.server_cert);
> 
> Assuming my template foo is correct (sorta hard, given the pump-generated
nature
> of the BindState), for P1 args, base::Bind will auto-refcount. However, even
> more, since typeof (nss_handshake_state_.server_cert) is a scoped_refptr<>,
when
> base::Bind evalutes the base::internal::CallbackParamTraits<P1>::StorageType
> (not to be confused with the base::internal::BoundFunctorTraits::A1Type),
it'll
> store a scoped_refptr<>.
> 
> AIUI, There's two types of introspection that happens - one based upon the
> typenames you base to base::Bind, one based on the typenames extracted from
the
> Functor you've bound too. As long as the Bound typename is convertable to the
> Functor typename, it should work.
> 
> The easiest way to test if I'm wrong in this is to compile on Linux. If
> base::Bind (bind.h line 78) is evaluating P1 to be a "X509Certificate*" (the
> Functor param trait) instead of a "const scoped_refptr<X509Certificate>&" (the
> Bound template arg trait), then you should get a compile error on bind.h line
> 102.
> 
> If you don't get an error, then it works and you're safe.
> If you do get an error then:
> - Option 1) The function signature will need to be changed to "const
> scoped_refptr<X509Certificate>&" and all the no-copy operations rewritten to
use
> base::ConstRef()
> - Option 2) use base::Owned(new scoped_refptr<X509Certificate>(...)); to heap
> allocate a scoped_refptr that will be destroyed post callback
> [Larger design work: eg, not serious for this change, but presented for
> completness]
> - Option 3) Separate out the FunctorParamTraits and the CallbackParamTraits so
> that callers can indicate whether or not they're intending to pass a pointer
> (eg: what base::Unretained() is supposed to do)
> - Option 4) Make scoped_refptr<> a pseudo-passable type, so that when you
> actually do want Bind to store a scoped_refptr<> even when the signature takes
a
> naked pointer, then the caller passes base::Passed() instead (eg: the inverse
of
> Option 3)
> 
> But I think you should just get away with binding to the argument itself, with
> no bind helpers involved :-)
> 
> FWIW, callback_internal.h, bind_internal.h, and bind_helpers.h are all pretty
> good documentation. As callback.h mentions, all arguments are automatically
> ref-counted UNLESS base::Unretained (documented in bind_helpers.h) is used

callback.h does not seem to say it refcounts by default, or if it does, I can't
find it.  All I can find is "By default Bind() will store copies of all bound
parameters, and attempt to refcount a target object if the function being bound
is a class method."  Most of the docs there talk about its guts, without talking
about behavior.

The issue in this case was I didn't realize that a refptr's * operator  would
let it be passed to a function that takes a pointer when using bind, or at least
I assume that's what's going on.  Going through 5+ levels of classes/templates
to deduce behaviors just makes my brain hurt.

On a somewhat related note, if I pass a pointer to an object that inherits from
refcounted, is it refcounted, or do I have to pass a scoped_refptr?  I believe
doing the former may Linux-error, but I'm not sure.

[Ryan Sleevi, 2012-06-12 22:31:06]

On 2012/06/12 22:21:37, Matt Menke wrote:
> callback.h does not seem to say it refcounts by default, or if it does, I
can't
> find it.  All I can find is "By default Bind() will store copies of all bound
> parameters, and attempt to refcount a target object if the function being
bound
> is a class method."  Most of the docs there talk about its guts, without
talking
> about behavior.

Yeah, perhaps that wording should be shored up.

Stores a copy of its argument + argument is a scoped_refptr<> = stores as a
scoped_refptr<> (meaning it'll AddRef/Release)

> 
> The issue in this case was I didn't realize that a refptr's * operator  would
> let it be passed to a function that takes a pointer when using bind, or at
least
> I assume that's what's going on.  Going through 5+ levels of classes/templates
> to deduce behaviors just makes my brain hurt.

Right. That's what is supposed to be happening. CallbackParamTraits<P1> deduces
it's a "const scoped_refptr<T>&", and FunctorTraits<Arg1> deduces that the
call-time param is a T*, and since UnwrapTraits<scoped_refptr<T>> returns a
scoped_refptr<T>, and scoped_refptr<T> is castable to T*, it all "just works"

I agree though, it's a crazy map of brain-hurt. I only learned it because of
base::PostTaskAndReplyWithResult being unable to take a scoped_ptr<>, hence got
to learn all of that fun.

> 
> On a somewhat related note, if I pass a pointer to an object that inherits
from
> refcounted, is it refcounted, or do I have to pass a scoped_refptr?  I believe
> doing the former may Linux-error, but I'm not sure.

If you Bind as a pointer T*, then you'll get a Linux compile error, and it will
not ref count.
If you Bind as a scoped_refptr<T>(T*), then it will.

This is why, for example, in SSLClientSocketNSS::Core, I was (am?) explicitly
specifying scoped_refptr<NetLog::EventParameters>(new T(...)). If I had just
said "new T(...)", it would not have ref-counted the EventParameters, and would
have compile errored on Linux.

Technically, we can make it a compile error on Windows as well, but the
compile-time performance hit, not to mention the readability hit, of the
template introspection is probably not worth it. I think it was something like
12-15% slower compiles for that extra indirection...

[commit-bot: I haz the power, 2012-06-12 22:56:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10534117/5003

[commit-bot: I haz the power, 2012-06-13 00:46:42]

Change committed as 141812