Stop attempting to write to transport sockets in NSS on failure.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 3a8b39e678c2785ea4d6fd7c60fea762fb599883..34bdf1b8fb5e120c9a095fb5dec52f2b56ac168f 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -890,6 +890,7 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe<Core> {
   bool transport_recv_busy_;
   bool transport_recv_eof_;
   bool transport_send_busy_;
+  int transport_send_error_;
 
   // Used by Read function.
   scoped_refptr<IOBuffer> user_read_buf_;
@@ -956,6 +957,7 @@ SSLClientSocketNSS::Core::Core(
       transport_recv_busy_(false),
       transport_recv_eof_(false),
       transport_send_busy_(false),
+      transport_send_error_(0),

[wtc, 2014/06/16 19:57:21]

I assume transport_send_error_ is a net:: error code. If so, it should be
initialized to OK (same value but more descriptive).

Make the same change on line 2144.

[davidben, 2014/06/16 23:02:13]

Done.

       user_read_buf_len_(0),
       user_write_buf_len_(0),
       network_task_runner_(network_task_runner),
@@ -2139,6 +2141,8 @@ int SSLClientSocketNSS::Core::BufferSend() {
 
   if (transport_send_busy_)
     return ERR_IO_PENDING;
+  if (transport_send_error_ != 0)
+    return transport_send_error_;

[Ryan Sleevi, 2014/06/16 22:07:07]

Why introduce another member, when we already have the memio_PutWriteResult
tracking the write error?

Ultimately, the exact error doesn't matter - you can see that the BufferSend rv
gets swallowed in DoTransportIO, so the need for the exact error isn't even
needed. Heck, ERR_ABORTED would be sufficient to break the loop.

This also seems to ensure greater consistency with the existing implementation
(eg: it avoids the need to special-note on 2711-2714), and also avoids the issue
Wan-Teh noted where you're not storing the result in case of sync failure in
DoBufferSend (since we handle that below, on line 2172)

[davidben, 2014/06/16 23:02:13]

Good point. That's much cleaner. Done.

 
   const char* buf1;
   const char* buf2;
@@ -2704,6 +2708,12 @@ void SSLClientSocketNSS::Core::BufferSendComplete(int result) {
 
   memio_PutWriteResult(nss_bufs_, MapErrorToNSS(result));
   transport_send_busy_ = false;
+  // If there was an error, save the result on the NSS task runner. Future calls
+  // to BufferSend will return the error synchronously rather than calling
+  // DoBufferSend on the network task runner. This is important to prevent
+  // spinning infinitely. See https://crbug.com/381160.

[wtc, 2014/06/16 19:57:21]

Nit: it may be better to move this comment (starting with the second sentence
"Future calls to BufferSend...") to SSLClientSocketNSS::Core::BufferSend (line
2144), where there is more context to understand this comment.

[davidben, 2014/06/16 23:02:13]

Done.

+  if (result < 0)
+    transport_send_error_ = result;
   OnSendComplete(result);
 }