Fix data races in QUIC end-to-end-tests.

Provide notifications for pausing/resuming the QuicServer, as
well as handshake confirmation in ServerThread to avoid data races.

Merge internal change: 56303744

Change end-to-end tests from using racy/too small packet size
to instead use large requests.

Merge internal change: 56174630

BUG=313733

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

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

5 files changed, 128 insertions, 159 deletions

diff --git a/net/quic/quic_end_to_end_unittest.cc b/net/quic/quic_end_to_end_unittest.cc
index 99a040d..90c670b 100644
--- a/net/quic/quic_end_to_end_unittest.cc
+++ b/net/quic/quic_end_to_end_unittest.cc
@@ -141,7 +141,7 @@ class QuicEndToEndTest : public PlatformTest {
                                           QuicSupportedVersions(),
                                           strike_register_no_startup_period_));
     server_thread_->Start();
-    server_thread_->listening()->Wait();
+    server_thread_->WaitForServerStartup();
     server_address_ = IPEndPoint(server_address_.address(),
                                  server_thread_->GetPort());
     server_started_ = true;
@@ -153,7 +153,7 @@ class QuicEndToEndTest : public PlatformTest {
       return;
     }
     if (server_thread_.get()) {
-      server_thread_->quit()->Signal();
+      server_thread_->Quit();
       server_thread_->Join();
     }
   }
diff --git a/net/tools/quic/end_to_end_test.cc b/net/tools/quic/end_to_end_test.cc
index 220e4f7..597d1867 100644
--- a/net/tools/quic/end_to_end_test.cc
+++ b/net/tools/quic/end_to_end_test.cc
@@ -54,11 +54,6 @@ namespace {
 
 const char* kFooResponseBody = "Artichoke hearts make me happy.";
 const char* kBarResponseBody = "Palm hearts are pretty delicious, also.";
-const size_t kCongestionFeedbackFrameSize = 25;
-// If kCongestionFeedbackFrameSize increase we need to expand this string
-// accordingly.
-const char* kLargeRequest =
-    "https://www.google.com/foo/test/a/request/string/longer/than/25/bytes";
 
 void GenerateBody(string* body, int length) {
   body->clear();
@@ -150,7 +145,6 @@ class EndToEndTest : public ::testing::TestWithParam<TestParams> {
                                                   0);
 
     QuicInMemoryCachePeer::ResetForTests();
-    AddToCache("GET", kLargeRequest, "HTTP/1.1", "200", "OK", kFooResponseBody);
     AddToCache("GET", "https://www.google.com/foo",
                "HTTP/1.1", "200", "OK", kFooResponseBody);
     AddToCache("GET", "https://www.google.com/bar",
@@ -215,7 +209,7 @@ class EndToEndTest : public ::testing::TestWithParam<TestParams> {
                                           server_supported_versions_,
                                           strike_register_no_startup_period_));
     server_thread_->Start();
-    server_thread_->listening()->Wait();
+    server_thread_->WaitForServerStartup();
     server_address_ = IPEndPoint(server_address_.address(),
                                  server_thread_->GetPort());
     QuicDispatcher* dispatcher =
@@ -230,7 +224,7 @@ class EndToEndTest : public ::testing::TestWithParam<TestParams> {
     if (!server_started_)
       return;
     if (server_thread_.get()) {
-      server_thread_->quit()->Signal();
+      server_thread_->Quit();
       server_thread_->Join();
     }
   }
@@ -362,62 +356,28 @@ TEST_P(EndToEndTest, MultipleClients) {
 }
 
 TEST_P(EndToEndTest, RequestOverMultiplePackets) {
+  // Send a large enough request to guarantee fragmentation.
+  string huge_request =
+      "https://www.google.com/some/path?query=" + string(kMaxPacketSize, '.');
+  AddToCache("GET", huge_request, "HTTP/1.1", "200", "OK", kBarResponseBody);
+
   ASSERT_TRUE(Initialize());
-  // Set things up so we have a small payload, to guarantee fragmentation.
-  // A congestion feedback frame can't be split into multiple packets, make sure
-  // that our packet have room for at least this amount after the normal headers
-  // are added.
-
-  // TODO(rch) handle this better when we have different encryption options.
-  const size_t kStreamDataLength = 3;
-  const QuicStreamId kStreamId = 1u;
-  const QuicStreamOffset kStreamOffset = 0u;
-  size_t stream_payload_size = QuicFramer::GetMinStreamFrameSize(
-      negotiated_version_, kStreamId, kStreamOffset, true) + kStreamDataLength;
-  size_t min_payload_size =
-      std::max(kCongestionFeedbackFrameSize, stream_payload_size);
-  size_t ciphertext_size =
-      NullEncrypter(false).GetCiphertextSize(min_payload_size);
-  // TODO(satyashekhar): Fix this when versioning is implemented.
-  client_->options()->max_packet_length =
-      GetPacketHeaderSize(PACKET_8BYTE_GUID, !kIncludeVersion,
-                          PACKET_6BYTE_SEQUENCE_NUMBER, NOT_IN_FEC_GROUP) +
-      ciphertext_size;
-
-  // Make sure our request is too large to fit in one packet.
-  EXPECT_GT(strlen(kLargeRequest), min_payload_size);
-  EXPECT_EQ(kFooResponseBody, client_->SendSynchronousRequest(kLargeRequest));
+
+  EXPECT_EQ(kBarResponseBody, client_->SendSynchronousRequest(huge_request));
   EXPECT_EQ(200u, client_->response_headers()->parsed_response_code());
 }
 
 TEST_P(EndToEndTest, MultipleFramesRandomOrder) {
+  // Send a large enough request to guarantee fragmentation.
+  string huge_request =
+      "https://www.google.com/some/path?query=" + string(kMaxPacketSize, '.');
+  AddToCache("GET", huge_request, "HTTP/1.1", "200", "OK", kBarResponseBody);
+
   ASSERT_TRUE(Initialize());
   SetPacketSendDelay(QuicTime::Delta::FromMilliseconds(2));
   SetReorderPercentage(50);
-  // Set things up so we have a small payload, to guarantee fragmentation.
-  // A congestion feedback frame can't be split into multiple packets, make sure
-  // that our packet have room for at least this amount after the normal headers
-  // are added.
-
-  // TODO(rch) handle this better when we have different encryption options.
-  const size_t kStreamDataLength = 3;
-  const QuicStreamId kStreamId = 1u;
-  const QuicStreamOffset kStreamOffset = 0u;
-  size_t stream_payload_size = QuicFramer::GetMinStreamFrameSize(
-      negotiated_version_, kStreamId, kStreamOffset, true) + kStreamDataLength;
-  size_t min_payload_size =
-      std::max(kCongestionFeedbackFrameSize, stream_payload_size);
-  size_t ciphertext_size =
-      NullEncrypter(false).GetCiphertextSize(min_payload_size);
-  // TODO(satyashekhar): Fix this when versioning is implemented.
-  client_->options()->max_packet_length =
-      GetPacketHeaderSize(PACKET_8BYTE_GUID, !kIncludeVersion,
-                          PACKET_6BYTE_SEQUENCE_NUMBER, NOT_IN_FEC_GROUP) +
-      ciphertext_size;
-
-  // Make sure our request is too large to fit in one packet.
-  EXPECT_GT(strlen(kLargeRequest), min_payload_size);
-  EXPECT_EQ(kFooResponseBody, client_->SendSynchronousRequest(kLargeRequest));
+
+  EXPECT_EQ(kBarResponseBody, client_->SendSynchronousRequest(huge_request));
   EXPECT_EQ(200u, client_->response_headers()->parsed_response_code());
 }
 
@@ -573,10 +533,15 @@ TEST_P(EndToEndTest, DISABLED_LargePostFEC) {
   EXPECT_EQ(kFooResponseBody, client_->SendCustomSynchronousRequest(request));
 }
 
-/*TEST_P(EndToEndTest, PacketTooLarge) {
-  FLAGS_quic_allow_oversized_packets_for_test = true;
+// Enable once FLAGS_quic_allow_oversized_packets_for_test is added.
+TEST_P(EndToEndTest, DISABLED_PacketTooLarge) {
+  //FLAGS_quic_allow_oversized_packets_for_test = true;
   ASSERT_TRUE(Initialize());
 
+  // Wait for the handshake to be confirmed so that the negotiated
+  // max packet size does not overwrite our increased packet size.
+  client_->client()->WaitForCryptoHandshakeConfirmed();
+
   // If we use packet padding, then the CHLO is padded to such a large
   // size that it is rejected by the server before the handshake can complete
   // which results in a test timeout.
@@ -595,7 +560,7 @@ TEST_P(EndToEndTest, DISABLED_LargePostFEC) {
   EXPECT_EQ("", client_->SendCustomSynchronousRequest(request));
   EXPECT_EQ(QUIC_STREAM_CONNECTION_ERROR, client_->stream_error());
   EXPECT_EQ(QUIC_PACKET_TOO_LARGE, client_->connection_error());
-}*/
+}
 
 TEST_P(EndToEndTest, InvalidStream) {
   ASSERT_TRUE(Initialize());
@@ -692,14 +657,14 @@ TEST_P(EndToEndTest, LimitMaxPacketSizeAndCongestionWindowAndRTT) {
 
   ASSERT_TRUE(Initialize());
   client_->client()->WaitForCryptoHandshakeConfirmed();
+  server_thread_->WaitForCryptoHandshakeConfirmed();
+
+  // Pause the server so we can access the server's internals without races.
+  server_thread_->Pause();
   QuicDispatcher* dispatcher =
       QuicServerPeer::GetDispatcher(server_thread_->server());
   ASSERT_EQ(1u, dispatcher->session_map().size());
   QuicSession* session = dispatcher->session_map().begin()->second;
-  while (!session->IsCryptoHandshakeConfirmed()) {
-    server_thread_->server()->WaitForEvents();
-  }
-
   QuicConfig* client_negotiated_config = client_->client()->session()->config();
   QuicConfig* server_negotiated_config = session->config();
   QuicCongestionManager* client_congestion_manager =
@@ -743,6 +708,8 @@ TEST_P(EndToEndTest, LimitMaxPacketSizeAndCongestionWindowAndRTT) {
                       HttpConstants::POST, "/foo");
   request.AddBody(body, true);
 
+  server_thread_->Resume();
+
   EXPECT_EQ(kFooResponseBody, client_->SendCustomSynchronousRequest(request));
 }
 
@@ -754,14 +721,14 @@ TEST_P(EndToEndTest, InitialRTT) {
 
   ASSERT_TRUE(Initialize());
   client_->client()->WaitForCryptoHandshakeConfirmed();
+  server_thread_->WaitForCryptoHandshakeConfirmed();
+
+  // Pause the server so we can access the server's internals without races.
+  server_thread_->Pause();
   QuicDispatcher* dispatcher =
       QuicServerPeer::GetDispatcher(server_thread_->server());
   ASSERT_EQ(1u, dispatcher->session_map().size());
   QuicSession* session = dispatcher->session_map().begin()->second;
-  while (!session->IsCryptoHandshakeConfirmed()) {
-    server_thread_->server()->WaitForEvents();
-  }
-
   QuicConfig* client_negotiated_config = client_->client()->session()->config();
   QuicConfig* server_negotiated_config = session->config();
   QuicCongestionManager* client_congestion_manager =
@@ -847,9 +814,8 @@ TEST_P(EndToEndTest, ConnectionMigration) {
 
   writer->set_writer(new QuicDefaultPacketWriter(
       QuicClientPeer::GetFd(client_->client())));
-  QuicConnectionPeer::SetWriter(
-      client_->client()->session()->connection(),
-      reinterpret_cast<QuicTestWriter*>(writer.get()));
+  QuicConnectionPeer::SetWriter(client_->client()->session()->connection(),
+                                writer.get());
 
   client_->SendSynchronousRequest("/bar");
 
diff --git a/net/tools/quic/test_tools/server_thread.cc b/net/tools/quic/test_tools/server_thread.cc
index dc41e09e..a1bebc7 100644
--- a/net/tools/quic/test_tools/server_thread.cc
+++ b/net/tools/quic/test_tools/server_thread.cc
@@ -4,6 +4,8 @@
 
 #include "net/tools/quic/test_tools/server_thread.h"
 
+#include "net/tools/quic/test_tools/quic_server_peer.h"
+
 namespace net {
 namespace tools {
 namespace test {
@@ -14,6 +16,10 @@ ServerThread::ServerThread(IPEndPoint address,
                            bool strike_register_no_startup_period)
     : SimpleThread("server_thread"),
       listening_(true, false),
+      confirmed_(true, false),
+      pause_(true, false),
+      paused_(true, false),
+      resume_(true, false),
       quit_(true, false),
       server_(config, supported_versions),
       address_(address),
@@ -27,7 +33,6 @@ ServerThread::~ServerThread() {
 }
 
 void ServerThread::Run() {
-  LOG(INFO) << "listening";
   server_.Listen(address_);
 
   port_lock_.Acquire();
@@ -36,9 +41,14 @@ void ServerThread::Run() {
 
   listening_.Signal();
   while (!quit_.IsSignaled()) {
+    if (pause_.IsSignaled() && !resume_.IsSignaled()) {
+      paused_.Signal();
+      resume_.Wait();
+    }
     server_.WaitForEvents();
+    MaybeNotifyOfHandshakeConfirmation();
   }
-  LOG(INFO) << "shutdown";
+
   server_.Shutdown();
 }
 
@@ -49,6 +59,49 @@ int ServerThread::GetPort() {
     return rc;
 }
 
+void ServerThread::WaitForServerStartup() {
+  listening_.Wait();
+}
+
+void ServerThread::WaitForCryptoHandshakeConfirmed() {
+  confirmed_.Wait();
+}
+
+void ServerThread::Pause() {
+  DCHECK(!pause_.IsSignaled());
+  pause_.Signal();
+  paused_.Wait();
+}
+
+void ServerThread::Resume() {
+  DCHECK(!resume_.IsSignaled());
+  DCHECK(pause_.IsSignaled());
+  resume_.Signal();
+}
+
+void ServerThread::Quit() {
+  if (pause_.IsSignaled() && !resume_.IsSignaled()) {
+    resume_.Signal();
+  }
+  quit_.Signal();
+}
+
+void ServerThread::MaybeNotifyOfHandshakeConfirmation() {
+  if (confirmed_.IsSignaled()) {
+    // Only notify once.
+    return;
+  }
+  QuicDispatcher* dispatcher = QuicServerPeer::GetDispatcher(server());
+  if (dispatcher->session_map().empty()) {
+    // Wait for a session to be created.
+    return;
+  }
+  QuicSession* session = dispatcher->session_map().begin()->second;
+  if (session->IsCryptoHandshakeConfirmed()) {
+    confirmed_.Signal();
+  }
+}
+
 }  // namespace test
 }  // namespace tools
 }  // namespace net
diff --git a/net/tools/quic/test_tools/server_thread.h b/net/tools/quic/test_tools/server_thread.h
index 2011882..ed36c37 100644
--- a/net/tools/quic/test_tools/server_thread.h
+++ b/net/tools/quic/test_tools/server_thread.h
@@ -24,20 +24,49 @@ class ServerThread : public base::SimpleThread {
 
   virtual ~ServerThread();
 
+  // SimpleThread implementation.
   virtual void Run() OVERRIDE;
 
-  int GetPort();
+  // Waits until the server has started and is listening for requests.
+  void WaitForServerStartup();
+
+  // Waits for the handshake to be confirmed for the first session created.
+  void WaitForCryptoHandshakeConfirmed();
+
+  // Pauses execution of the server until Resume() is called.  May only be
+  // called once.
+  void Pause();
 
-  base::WaitableEvent* listening() { return &listening_; }
-  base::WaitableEvent* quit() { return &quit_; }
+  // Resumes execution of the server after Pause() has been called.  May only
+  // be called once.
+  void Resume();
+
+  // Stops the server from executing and shuts it down, destroying all
+  // server objects.
+  void Quit();
+
+  // Returns the underlying server.  Care must be taken to avoid data races
+  // when accessing the server.  It is always safe to access the server
+  // after calling Pause() and before calling Resume().
   QuicServer* server() { return &server_; }
 
+  // Returns the port that the server is listening on.
+  int GetPort();
+
  private:
-  base::WaitableEvent listening_;
-  base::WaitableEvent quit_;
-  base::Lock port_lock_;
+  void MaybeNotifyOfHandshakeConfirmation();
+
+  base::WaitableEvent listening_;  // Notified when the server is listening.
+  base::WaitableEvent confirmed_;  // Notified when the first handshake is
+                                   // confirmed.
+  base::WaitableEvent pause_;      // Notified when the server should pause.
+  base::WaitableEvent paused_;     // Notitied when the server has paused
+  base::WaitableEvent resume_;     // Notified when the server should resume.
+  base::WaitableEvent quit_;       // Notified when the server should quit.
+
   tools::QuicServer server_;
   IPEndPoint address_;
+  base::Lock port_lock_;
   int port_;
 
   DISALLOW_COPY_AND_ASSIGN(ServerThread);
diff --git a/tools/valgrind/tsan/suppressions.txt b/tools/valgrind/tsan/suppressions.txt
index 35d38bc..7e25d0b 100644
--- a/tools/valgrind/tsan/suppressions.txt
+++ b/tools/valgrind/tsan/suppressions.txt
@@ -1139,82 +1139,3 @@
    fun:MessageLoopHelper::TimerExpired
    fun:base::internal::RunnableAdapter::Run
 }
-{
-   bug_313733_a
-   ThreadSanitizer:Race
-   fun:base::TimeDelta::InMicroseconds
-   fun:net::QuicTime::Delta::IsZero
-   fun:net::TcpCubicSender::SmoothedRtt
-   fun:net::QuicCongestionManager::SmoothedRtt
-   fun:net::tools::test::::EndToEndTest_InitialRTT_Test::TestBody
-   fun:testing::internal::HandleSehExceptionsInMethodIfSupported
-}
-{
-   bug_313733_b
-   ThreadSanitizer:Race
-   fun:base::TimeDelta::operator=
-   fun:net::QuicTime::Delta::operator=
-   fun:net::TcpCubicSender::AckAccounting
-   fun:net::TcpCubicSender::OnIncomingAck
-   fun:net::QuicCongestionManager::OnIncomingAckFrame
-   fun:net::QuicConnection::ProcessAckFrame
-   fun:net::QuicConnection::OnPacketComplete
-   fun:net::QuicFramer::ProcessDataPacket
-   fun:net::QuicFramer::ProcessPacket
-   fun:net::QuicConnection::ProcessUdpPacket
-   fun:net::tools::QuicDispatcher::ProcessPacket
-   fun:net::tools::QuicServer::MaybeDispatchPacket
-   fun:net::tools::QuicServer::ReadAndDispatchSinglePacket
-   fun:net::tools::QuicServer::OnEvent
-   fun:net::EpollServer::CallReadyListCallbacks
-   fun:net::EpollServer::WaitForEventsAndCallHandleEvents
-   fun:net::EpollServer::WaitForEventsAndExecuteCallbacks
-   fun:net::tools::QuicServer::WaitForEvents
-   fun:net::tools::test::ServerThread::Run
-   fun:base::SimpleThread::ThreadMain
-   fun:base::::ThreadFunc
-}
-{
-   bug_313733_c
-   ThreadSanitizer:Race
-   fun:net::QuicCryptoServerStream::OnHandshakeMessage
-   fun:net::CryptoFramer::Process
-   fun:net::CryptoFramer::ProcessInput
-   fun:net::QuicCryptoStream::ProcessData
-   fun:net::ReliableQuicStream::ProcessRawData
-   fun:net::QuicStreamSequencer::OnStreamFrame
-   fun:net::ReliableQuicStream::OnStreamFrame
-   fun:net::QuicSession::OnStreamFrames
-   fun:net::VisitorShim::OnStreamFrames
-   fun:net::QuicConnection::OnPacketComplete
-   fun:net::QuicFramer::ProcessDataPacket
-   fun:net::QuicFramer::ProcessPacket
-   fun:net::QuicConnection::ProcessUdpPacket
-   fun:net::tools::QuicDispatcher::ProcessPacket
-   fun:net::tools::QuicServer::MaybeDispatchPacket
-   fun:net::tools::QuicServer::ReadAndDispatchSinglePacket
-   fun:net::tools::QuicServer::OnEvent
-   fun:net::EpollServer::CallReadyListCallbacks
-   fun:net::EpollServer::WaitForEventsAndCallHandleEvents
-   fun:net::EpollServer::WaitForEventsAndExecuteCallbacks
-   fun:net::tools::QuicServer::WaitForEvents
-   fun:net::tools::test::ServerThread::Run
-   fun:base::SimpleThread::ThreadMain
-   fun:base::::ThreadFunc
-}
-{
-   bug_313733_d
-   ThreadSanitizer:Race
-   fun:net::QuicCryptoStream::handshake_confirmed
-   fun:net::QuicSession::IsCryptoHandshakeConfirmed
-   ...
-   fun:testing::internal::HandleSehExceptionsInMethodIfSupported
-}
-{
-  bug_313733_e
-  ThreadSanitizer:Race
-  fun:base::TimeDelta::operator=
-  fun:net::QuicTime::Delta::operator=
-  ...
-  fun:base::::ThreadFunc
-}