Fix instability in SSL client/server sockets

net/socket/ssl_server_socket_unittest.cc

Index: net/socket/ssl_server_socket_unittest.cc
diff --git a/net/socket/ssl_server_socket_unittest.cc b/net/socket/ssl_server_socket_unittest.cc
index de89c207117b900a92f2f4b89a90a12ea692821f..457ee134708497b7301d5c373fee2d26890e3039 100644
--- a/net/socket/ssl_server_socket_unittest.cc
+++ b/net/socket/ssl_server_socket_unittest.cc
@@ -15,10 +15,13 @@
 
 #include "net/socket/ssl_server_socket.h"
 
+#include <stdlib.h>
+
 #include <queue>
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "crypto/nss_util.h"
 #include "crypto/rsa_private_key.h"
@@ -46,7 +49,10 @@ namespace {
 
 class FakeDataChannel {
  public:
-  FakeDataChannel() : read_callback_(NULL), read_buf_len_(0) {
+  FakeDataChannel()
+      : read_callback_(NULL),
+        read_buf_len_(0),
+        ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {

[willchan no longer on Chromium, 2011/07/21 08:16:06]

Need compiler_specific.h for this macro.

[Sergey Ulanov, 2011/07/21 17:11:12]

Done.

   }
 
   virtual int Read(IOBuffer* buf, int buf_len,
@@ -63,13 +69,15 @@ class FakeDataChannel {
   virtual int Write(IOBuffer* buf, int buf_len,
                     CompletionCallback* callback) {
     data_.push(new net::DrainableIOBuffer(buf, buf_len));
-    DoReadCallback();
+    MessageLoop::current()->PostTask(
+        FROM_HERE, task_factory_.NewRunnableMethod(
+            &FakeDataChannel::DoReadCallback));

[wtc, 2011/07/21 00:27:12]

willchan: can you review the use of ScopedRunnableMethodFactory
in this class?

sergeyu: why do you need to post a task to run DoReadCallback()?

[Sergey Ulanov, 2011/07/21 00:51:15]

Because DoReadCallback() calls callback on the socket on the other end, which
then may then send some data back which would invoke callback on the first
socket. I.e. SSL socket calls Write() on the fake socket, and receives read
callback before Write() returns - this is not expected by SSL sockets code, so
it crashes.

[willchan no longer on Chromium, 2011/07/21 08:16:06]

Looks reasonable to me.

     return buf_len;
   }
 
  private:
   void DoReadCallback() {
-    if (!read_callback_)
+    if (!read_callback_ || !data_.size())

[wtc, 2011/07/21 00:27:12]

Nit: some people prefer data_.empty().

Why do you need to check if data_ is empty?
It seems to imply that you are calling DoReadCallback
more times than you should.

[Sergey Ulanov, 2011/07/21 00:51:15]

Done.

       return;
 
     int copied = PropogateData(read_buf_, read_buf_len_);
@@ -97,6 +105,8 @@ class FakeDataChannel {
 
   std::queue<scoped_refptr<net::DrainableIOBuffer> > data_;
 
+  ScopedRunnableMethodFactory<FakeDataChannel> task_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeDataChannel);
 };
 
@@ -114,11 +124,15 @@ class FakeSocket : public StreamSocket {
 
   virtual int Read(IOBuffer* buf, int buf_len,
                    CompletionCallback* callback) {
+    // Read random number of bytes.
+    buf_len = rand() % buf_len + 1;
     return incoming_->Read(buf, buf_len, callback);
   }
 
   virtual int Write(IOBuffer* buf, int buf_len,
                     CompletionCallback* callback) {
+    // Write random number of bytes.
+    buf_len = rand() % buf_len + 1;

[wtc, 2011/07/21 00:27:12]

Are you using rand() instead of base::RandInt() because
you need a repeatable sequence of pseudorandom numbers?

     return outgoing_->Write(buf, buf_len, callback);
   }
 
@@ -204,17 +218,28 @@ TEST(FakeSocketTest, DataTransfer) {
   scoped_refptr<net::IOBuffer> read_buf = new net::IOBuffer(kReadBufSize);
 
   // Write then read.
-  EXPECT_EQ(kTestDataSize, server.Write(write_buf, kTestDataSize, NULL));
-  EXPECT_EQ(kTestDataSize, client.Read(read_buf, kReadBufSize, NULL));
-  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), kTestDataSize));
+  int written = server.Write(write_buf, kTestDataSize, NULL);
+  EXPECT_GT(written, 0);
+  EXPECT_LE(written, kTestDataSize);
+
+  int read = client.Read(read_buf, kReadBufSize, NULL);

[wtc, 2011/07/21 00:27:12]

Nit: name these local variables nwritten and nread
or bytes_written and bytes_read.  "read" is the name of
a Unix system call.

+  EXPECT_GT(read, 0);
+  EXPECT_LE(read, written);
+  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 
   // Read then write.
   TestCompletionCallback callback;
   EXPECT_EQ(net::ERR_IO_PENDING,
             server.Read(read_buf, kReadBufSize, &callback));
-  EXPECT_EQ(kTestDataSize, client.Write(write_buf, kTestDataSize, NULL));
-  EXPECT_EQ(kTestDataSize, callback.WaitForResult());
-  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), kTestDataSize));
+
+  written = client.Write(write_buf, kTestDataSize, NULL);
+  EXPECT_GT(written, 0);
+  EXPECT_LE(written, kTestDataSize);
+
+  read = callback.WaitForResult();
+  EXPECT_GT(read, 0);
+  EXPECT_LE(read, written);
+  EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 }
 
 class SSLServerSocketTest : public PlatformTest {