NetLogEventParameter to Callback refactoring 1,

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
===================================================================
--- net/socket/ssl_client_socket_nss.cc	(revision 141407)
+++ net/socket/ssl_client_socket_nss.cc	(working copy)
@@ -385,6 +385,20 @@
 // 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) {
@@ -2515,13 +2529,14 @@
   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));
   }
 }