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

On failure, future transport writes should synchronously return. This is
important on Chrome OS and Linux where we have a separate NSS task runner.
If we query the transport each time (in hopes that it will return the error
code) it becomes an asynchronous error and so the state machine keeps pumping
itself in response to the state change. (It alternates between "write pending"
and "write failed".)

Add a test that asserts we do not keep trying to write to the transport in a
loop.

This fixes one of the infinite loops in bug #381160, but not the other.

BUG=381160

Review URL: https://codereview.chromium.org/337823002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278014 0039d316-1c4b-4281-b951-d872f2087c98

net/base/nss_memio.c

@@ -442,17 +442,21 @@ void memio_PutReadResult(memio_Private *secret, int bytes_read)
     }
 }
 
-void memio_GetWriteParams(memio_Private *secret,
-                          const char **buf1, unsigned int *len1,
-                          const char **buf2, unsigned int *len2)
+int memio_GetWriteParams(memio_Private *secret,
+                         const char **buf1, unsigned int *len1,
+                         const char **buf2, unsigned int *len2)
 {
     struct memio_buffer* mb = &((PRFilePrivate *)secret)->writebuf;
     PR_ASSERT(mb->bufsize);
 
+    if (mb->last_err)
+        return mb->last_err;
+
     *buf1 = &mb->buf[mb->head];
     *len1 = memio_buffer_used_contiguous(mb);
     *buf2 = mb->buf;
     *len2 = memio_buffer_wrapped_bytes(mb);
+    return 0;
 }
 
 void memio_PutWriteResult(memio_Private *secret, int bytes_written)

net/base/nss_memio.h

@@ -79,12 +79,13 @@ int memio_GetReadableBufferSize(memio_Private *secret);
 void memio_PutReadResult(memio_Private *secret, int bytes_read);
 
 /* Ask memio what data it has to send to the network.
- * Returns up to two buffers of data by writing the positions and lengths into
- * |buf1|, |len1| and |buf2|, |len2|.
+ * If there was previous a write error, the NSPR error code is returned.
+ * Otherwise, it returns 0 and provides up to two buffers of data by
+ * writing the positions and lengths into |buf1|, |len1| and |buf2|, |len2|.
  */
-void memio_GetWriteParams(memio_Private *secret,
-                          const char **buf1, unsigned int *len1,
-                          const char **buf2, unsigned int *len2);
+int memio_GetWriteParams(memio_Private *secret,
+                         const char **buf1, unsigned int *len1,
+                         const char **buf2, unsigned int *len2);
 
 /* Tell memio how many bytes were sent to the network.
  * If bytes_written is < 0, it is treated as an NSPR error code.

net/socket/ssl_client_socket_nss.cc

@@ -2143,7 +2143,12 @@ int SSLClientSocketNSS::Core::BufferSend() {
   const char* buf1;
   const char* buf2;
   unsigned int len1, len2;
-  memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2);
+  if (memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2)) {
+    // It is important this return synchronously to prevent spinning infinitely
+    // in the off-thread NSS case. The error code itself is ignored, so just
+    // return ERR_ABORTED. See https://crbug.com/381160.
+    return ERR_ABORTED;
+  }
   const unsigned int len = len1 + len2;
 
   int rv = 0;

net/socket/ssl_client_socket_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
+#include "base/time/time.h"
 #include "net/base/address_list.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -529,6 +530,38 @@ void FakeBlockingStreamSocket::OnReadCompleted(int result) {
   }
 }
 
+// CountingStreamSocket wraps an existing StreamSocket and maintains a count of
+// reads and writes on the socket.
+class CountingStreamSocket : public WrappedStreamSocket {
+ public:
+  explicit CountingStreamSocket(scoped_ptr<StreamSocket> transport)
+      : WrappedStreamSocket(transport.Pass()),
+        read_count_(0),
+        write_count_(0) {}
+  virtual ~CountingStreamSocket() {}
+
+  // Socket implementation:
+  virtual int Read(IOBuffer* buf,
+                   int buf_len,
+                   const CompletionCallback& callback) OVERRIDE {
+    read_count_++;
+    return transport_->Read(buf, buf_len, callback);
+  }
+  virtual int Write(IOBuffer* buf,
+                    int buf_len,
+                    const CompletionCallback& callback) OVERRIDE {
+    write_count_++;
+    return transport_->Write(buf, buf_len, callback);
+  }
+
+  int read_count() const { return read_count_; }
+  int write_count() const { return write_count_; }
+
+ private:
+  int read_count_;
+  int write_count_;
+};
+
 // CompletionCallback that will delete the associated StreamSocket when
 // the callback is invoked.
 class DeleteSocketCallback : public TestCompletionCallbackBase {
@@ -1319,6 +1352,75 @@ TEST_F(SSLClientSocketTest, Write_WithSynchronousError) {
 #endif
 }
 
+// If there is a Write failure at the transport with no follow-up Read, although
+// the write error will not be returned to the client until a future Read or
+// Write operation, SSLClientSocket should not spin attempting to re-write on
+// the socket. This is a regression test for part of https://crbug.com/381160.
+TEST_F(SSLClientSocketTest, Write_WithSynchronousErrorNoRead) {
+  SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
+                                SpawnedTestServer::kLocalhost,
+                                base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  // Note: intermediate sockets' ownership are handed to |sock|, but a pointer
+  // is retained in order to query them.
+  scoped_ptr<SynchronousErrorStreamSocket> error_socket(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  SynchronousErrorStreamSocket* raw_error_socket = error_socket.get();
+  scoped_ptr<CountingStreamSocket> counting_socket(
+      new CountingStreamSocket(error_socket.PassAs<StreamSocket>()));
+  CountingStreamSocket* raw_counting_socket = counting_socket.get();
+  int rv = callback.GetResult(counting_socket->Connect(callback.callback()));
+  ASSERT_EQ(OK, rv);
+
+  // Disable TLS False Start to avoid handshake non-determinism.
+  SSLConfig ssl_config;
+  ssl_config.false_start_enabled = false;
+
+  scoped_ptr<SSLClientSocket> sock(
+      CreateSSLClientSocket(counting_socket.PassAs<StreamSocket>(),
+                            test_server.host_port_pair(),
+                            ssl_config));
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+  ASSERT_EQ(OK, rv);
+  ASSERT_TRUE(sock->IsConnected());
+
+  // Simulate an unclean/forcible shutdown on the underlying socket.
+  raw_error_socket->SetNextWriteError(ERR_CONNECTION_RESET);
+
+  const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
+  static const int kRequestTextSize =
+      static_cast<int>(arraysize(request_text) - 1);
+  scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
+  memcpy(request_buffer->data(), request_text, kRequestTextSize);
+
+  // This write should complete synchronously, because the TLS ciphertext
+  // can be created and placed into the outgoing buffers independent of the
+  // underlying transport.
+  rv = callback.GetResult(
+      sock->Write(request_buffer.get(), kRequestTextSize, callback.callback()));
+  ASSERT_EQ(kRequestTextSize, rv);
+
+  // Let the event loop spin for a little bit of time. Even on platforms where
+  // pumping the state machine involve thread hops, there should be no further
+  // writes on the transport socket.
+  //
+  // TODO(davidben): Avoid the arbitrary timeout?
+  int old_write_count = raw_counting_socket->write_count();
+  base::RunLoop loop;
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, loop.QuitClosure(), base::TimeDelta::FromMilliseconds(100));
+  loop.Run();
+  EXPECT_EQ(old_write_count, raw_counting_socket->write_count());
+}
+
 // Test the full duplex mode, with Read and Write pending at the same time.
 // This test also serves as a regression test for http://crbug.com/29815.
 TEST_F(SSLClientSocketTest, Read_FullDuplex) {

net/socket/ssl_server_socket_nss.cc

@@ -553,7 +553,10 @@ int SSLServerSocketNSS::BufferSend(void) {
   const char* buf1;
   const char* buf2;
   unsigned int len1, len2;
-  memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2);
+  if (memio_GetWriteParams(nss_bufs_, &buf1, &len1, &buf2, &len2)) {
+    // The error code itself is ignored, so just return ERR_ABORTED.
+    return ERR_ABORTED;
+  }
   const unsigned int len = len1 + len2;
 
   int rv = 0;