Fixing SSL error that occurs when underlying socket is blocked.

webrtc/base/ssladapter_unittest.cc

Index: webrtc/base/ssladapter_unittest.cc
diff --git a/webrtc/base/ssladapter_unittest.cc b/webrtc/base/ssladapter_unittest.cc
index ec39d949a2c764680ffc05990d3f06d2b3191a07..fdc5dd74144392fd984af6e6b26de15deeb34ed4 100644
--- a/webrtc/base/ssladapter_unittest.cc
+++ b/webrtc/base/ssladapter_unittest.cc
@@ -15,9 +15,10 @@
 #include "webrtc/base/ipaddress.h"
 #include "webrtc/base/socketstream.h"
 #include "webrtc/base/ssladapter.h"
-#include "webrtc/base/sslstreamadapter.h"
 #include "webrtc/base/sslidentity.h"
+#include "webrtc/base/sslstreamadapter.h"
 #include "webrtc/base/stream.h"
+#include "webrtc/base/stringencode.h"
 #include "webrtc/base/virtualsocketserver.h"
 
 static const int kTimeout = 5000;
@@ -209,22 +210,25 @@ class SSLAdapterTestDummyServer : public sigslot::has_slots<> {
 
   void OnSSLStreamAdapterEvent(rtc::StreamInterface* stream, int sig, int err) {
     if (sig & rtc::SE_READ) {
-      char buffer[4096] = "";
+      do {
+        char buffer[4096] = "";
+        size_t read;
+        int error;
+
+        // Read data received from the client and store it in our internal

[juberti1, 2017/06/02 05:35:44]

This shouldn't be necessary - we should trigger a new SE_READ even after a
partial read. Otherwise, looping here could cause us to become corebound trying
to read a busy stream.

[Taylor Brandstetter, 2017/06/02 06:49:14]

Hmm. So is this an issue with VirtualSocketServer? Should it be queueing a task
to call SignalReadEvent again if a partial read occurs? See updated patchset.

+        // buffer. Keep reading until Read returns something other than
+        // SR_SUCCESS, since multiple SSL records could be received in a single
+        // TCP packet.
+        rtc::StreamResult r =
+            stream->Read(buffer, sizeof(buffer) - 1, &read, &error);
+        if (r != rtc::SR_SUCCESS) {
+          break;
+        }
 
-      size_t read;
-      int error;
-
-      // Read data received from the client and store it in our internal
-      // buffer.
-      rtc::StreamResult r = stream->Read(buffer,
-          sizeof(buffer) - 1, &read, &error);
-      if (r == rtc::SR_SUCCESS) {
         buffer[read] = '\0';
-
         LOG(LS_INFO) << "Server received '" << buffer << "'";
-
         data_ += buffer;
-      }
+      } while (true);
     }
   }
 
@@ -336,7 +340,7 @@ class SSLAdapterTestBase : public testing::Test,
     LOG(LS_INFO) << "Transfer complete.";
   }
 
- private:
+ protected:
   const rtc::SSLMode ssl_mode_;
 
   std::unique_ptr<rtc::VirtualSocketServer> vss_;
@@ -389,6 +393,43 @@ TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransfer) {
   TestTransfer("Hello, world!");
 }
 
+TEST_F(SSLAdapterTestTLS_RSA, TestTLSTransferWithBlockedSocket) {

[juberti1, 2017/06/02 05:35:44]

Put this into a TestTransferWithBlockedSocket function in the fixture class,
perhaps?

[Taylor Brandstetter, 2017/06/02 06:49:14]

I don't think that helps, since I only want to run this test with TLS sockets
(and don't care about RSA vs. ECDSA).

[juberti1, 2017/06/02 17:47:22]

Sure, but I was thinking it would be easiest for future maintainers to have all
the test code that does real work arranged similarly.

+  TestHandshake(true);
+
+  // Tell the underlying socket to simulate being blocked.
+  vss_->SetSendingBlocked(true);
+
+  std::string expected;
+  int rv;
+  // Send messages until the SSL socket adapter starts applying backpressure.
+  // Note that this may not occur immediately since there may be some amount of
+  // intermediate buffering (either in our code or in BoringSSL).
+  for (int i = 0; i < 1024; ++i) {
+    std::string message = "Hello, world: " + rtc::ToString(i);
+    rv = client_->Send(message);
+    if (rv != static_cast<int>(message.size())) {
+      // This test assumes either the whole message or none of it is sent.
+      ASSERT_EQ(-1, rv);
+      break;
+    }
+    expected += message;
+  }
+  // Assert that the loop above exited due to Send returning -1.
+  ASSERT_EQ(-1, rv);

[pthatcher1, 2017/06/02 04:58:22]

Should we verify that sending the same thing while still blocked returns -1 and
doesn't cause double buffering or something broken?

[Taylor Brandstetter, 2017/06/02 06:49:14]

Done.

[pthatcher1, 2017/06/02 13:24:24]

I meant doing something like 


EXPECT_EQ(-1, client_->Send("Hello, world: 1023"));

[Taylor Brandstetter, 2017/06/02 17:10:53]

This already tests that the last message (for which -1 was returned) isn't sent.
Do you just want it to test 2 unsent messages instead of 1? If so, done.

+
+  // Unblock the underlying socket. All of the buffered messages should be sent
+  // without any further action.
+  vss_->SetSendingBlocked(false);
+  EXPECT_EQ_WAIT(expected, server_->GetReceivedData(), kTimeout);
+
+  // Send another message. This previously wasn't working
+  std::string final_message = "Fin.";
+  expected += final_message;
+  EXPECT_EQ(static_cast<int>(final_message.size()),
+            client_->Send(final_message));
+  EXPECT_EQ_WAIT(expected, server_->GetReceivedData(), kTimeout);
+}
+
 // Test transfer between client and server, using ECDSA
 TEST_F(SSLAdapterTestTLS_ECDSA, TestTLSTransfer) {
   TestHandshake(true);