NetLogEventParameter to Callback refactoring 1,

net/socket/ssl_client_socket_nss.cc

 // Copyright (c) 2012 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.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 367 matching lines @@
 }
 
 void DestroyCertificates(CERTCertificate** certs, size_t len) {
   for (size_t i = 0; i < len; i++)
     CERT_DestroyCertificate(certs[i]);
 }
 
 // Helper function to make it possible to log events from within the
 // SSLClientSocketNSS::Core.  Can't use Bind with BoundNetLog::AddEntry directly
 // on Windows because it is overloaded.
+// TODO(mmenke):  Other than shutdown, NetLog is threadsafe.  Figure out if this
+//                is needed.

[Ryan Sleevi, 2012/06/12 17:14:38]

This was needed because existing consumers of NetLog were only filtering for
IOThread events.

The intent of the change to SSLClientSocketNSS::Core was meant to keep the same
(public) interface, hence all events continuing to be logged on the IOThread. In
particular, the chrome/browser/net/load_timing_observer only looked at IO thread
events.

[mmenke, 2012/06/12 17:37:24]

Thanks for the explanation.

+void AddLogEventWithCallback(BoundNetLog* net_log,
+                             NetLog::EventType event_type,
+                             const NetLog::ParametersCallback& callback) {
+  if (!net_log)
+    return;
+  net_log->AddEvent(event_type, callback);
+}
+
+// Helper functions to make it possible to log events from within the
+// SSLClientSocketNSS::Core.  Can't use Bind with BoundNetLog::AddEntry directly
+// on Windows because it is overloaded.
+// TODO(mmenke):  This function is deprecated, delete it.
 void AddLogEvent(BoundNetLog* net_log,
                  NetLog::EventType event_type,
                  const scoped_refptr<NetLog::EventParameters>& event_params) {
   if (!net_log)
     return;
   net_log->AddEvent(event_type, event_params);
 }
 
 // Helper function to make it easier to call BoundNetLog::AddByteTransferEvent
 // from within the SSLClientSocketNSS::Core.
@@ skipping 2110 matching lines @@
   }
 
   return OK;
 }
 
 void SSLClientSocketNSS::Core::UpdateServerCert() {
   nss_handshake_state_.server_cert_chain.Reset(nss_fd_);
   nss_handshake_state_.server_cert = X509Certificate::CreateFromDERCertChain(
       nss_handshake_state_.server_cert_chain.AsStringPieceVector());
   if (nss_handshake_state_.server_cert) {
+    NetLog::ParametersCallback net_log_callback =
+        base::Bind(&NetLogX509CertificateCallback,
+                   base::Unretained(nss_handshake_state_.server_cert.get()));

[Ryan Sleevi, 2012/06/12 17:14:38]

I believe this is wrong.

nss_handshake_state_ should only be accessed on the NSS thread, but you're
binding it to a callback that executes on the I/O thread.

If you were using a retained (refcounted) copy it would be safe, but as it
presently stands, it's possible for this event to be posted, and then
subsequently an SSL record read that fails the handshake and potentially
invalidates this on the Core thread.

note: It's also not "safe" to bind the cert to the |network_handshake_state_|,
since that structure may be getting written on the I/O thread when this is read
on the NSS thread.

[eroman, 2012/06/12 17:30:11]

Thanks Ryan! I hadn't read this carefully, you are right there are some
threading issues.

I'm not sure that I much care for the posting between threads. I think the right
fix would be to make the observer able to consume these off the IO thread.

[mmenke, 2012/06/12 17:37:24]

Thanks for catching this, a clear bug.

Options:

1)  Keep a reference.  This depends on the certs not being mutable from the
point we post the event onwards.

2)  Go back to walking through it on the current thread, at least for calls from
this file.

3)  Log events on this thread.  Have LoadTimingObserver have special handling
for events it cares about that occur on other threads (Looks like all it cares
about on this thread is the SSL_CONNECTION event).

If we have a strong guarantee of non-mutability, 1) would be fine, and is the
easiest to do.

I'm not a huge fan of 2) or 3), but think 2)'s preferable, just in terms of
keeping life simple.

[eroman, 2012/06/12 17:47:52]

It doesn't look like load_timing_observer actually consumes this event.

In which case there is a simpler solution: simply emit this directly from the
current thread!

... load timing observer really needs to go die
(http://code.google.com/p/chromium/issues/detail?id=77446)

     PostOrRunCallback(
         FROM_HERE,
-        base::Bind(&AddLogEvent, weak_net_log_,
+        base::Bind(&AddLogEventWithCallback, weak_net_log_,
                    NetLog::TYPE_SSL_CERTIFICATES_RECEIVED,
-                   make_scoped_refptr(
-                       new X509CertificateNetLogParam(
-                           nss_handshake_state_.server_cert))));
+                   net_log_callback));
   }
 }
 
 void SSLClientSocketNSS::Core::UpdateConnectionStatus() {
   SSLChannelInfo channel_info;
   SECStatus ok = SSL_GetChannelInfo(nss_fd_,
                                     &channel_info, sizeof(channel_info));
   if (ok == SECSuccess &&
       channel_info.length == sizeof(channel_info) &&
       channel_info.cipherSuite) {
@@ skipping 1146 matching lines @@
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 ServerBoundCertService* SSLClientSocketNSS::GetServerBoundCertService() const {
   return server_bound_cert_service_;
 }
 
 }  // namespace net