Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_chromium_client_session_test.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/chromium/quic_chromium_client_session.h"
 
 #include "base/base64.h"
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/rand_util.h"
+#include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "net/base/test_completion_callback.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/http/transport_security_state.h"
 #include "net/log/test_net_log.h"
 #include "net/quic/chromium/crypto/proof_verifier_chromium.h"
 #include "net/quic/chromium/quic_chromium_alarm_factory.h"
 #include "net/quic/chromium/quic_chromium_client_session_peer.h"
 #include "net/quic/chromium/quic_chromium_connection_helper.h"
 #include "net/quic/chromium/quic_chromium_packet_reader.h"
@@ skipping 398 matching lines @@
   std::unique_ptr<QuicChromiumPacketReader> new_reader(
       new QuicChromiumPacketReader(new_socket.get(), &clock_, session_.get(),
                                    kQuicYieldAfterPacketsRead,
                                    QuicTime::Delta::FromMilliseconds(
                                        kQuicYieldAfterDurationMilliseconds),
                                    bound_net_log_.bound()));
   std::unique_ptr<QuicChromiumPacketWriter> new_writer(
       CreateQuicChromiumPacketWriter(new_socket.get(), session_.get()));
 
   // Migrate session.
-  EXPECT_TRUE(session_->MigrateToSocket(std::move(new_socket),
-                                        std::move(new_reader),
-                                        std::move(new_writer), nullptr));
+  EXPECT_TRUE(session_->MigrateToSocket(
+      std::move(new_socket), std::move(new_reader), std::move(new_writer)));
+  // Spin message loop to complete migration.
+  base::RunLoop().RunUntilIdle();
 
   // Write data to session.
   QuicChromiumClientStream* stream =
       session_->CreateOutgoingDynamicStream(kDefaultPriority);
   struct iovec iov[1];
   iov[0].iov_base = data;
   iov[0].iov_len = 4;
   session_->WritevData(stream, stream->id(),
                        QuicIOVector(iov, arraysize(iov), 4), 0, false, nullptr);
 
@@ skipping 27 matching lines @@
         new QuicChromiumPacketReader(new_socket.get(), &clock_, session_.get(),
                                      kQuicYieldAfterPacketsRead,
                                      QuicTime::Delta::FromMilliseconds(
                                          kQuicYieldAfterDurationMilliseconds),
                                      bound_net_log_.bound()));
     std::unique_ptr<QuicChromiumPacketWriter> new_writer(
         CreateQuicChromiumPacketWriter(new_socket.get(), session_.get()));
 
     // Migrate session.
     if (i < kMaxReadersPerQuicSession - 1) {
-      EXPECT_TRUE(session_->MigrateToSocket(std::move(new_socket),
-                                            std::move(new_reader),
-                                            std::move(new_writer), nullptr));
+      EXPECT_TRUE(session_->MigrateToSocket(
+          std::move(new_socket), std::move(new_reader), std::move(new_writer)));
+      // Spin message loop to complete migration.
+      base::RunLoop().RunUntilIdle();
       EXPECT_TRUE(socket_data.AllReadDataConsumed());
       EXPECT_TRUE(socket_data.AllWriteDataConsumed());
     } else {
       // Max readers exceeded.
-      EXPECT_FALSE(session_->MigrateToSocket(std::move(new_socket),
-                                             std::move(new_reader),
-                                             std::move(new_writer), nullptr));
-
+      EXPECT_FALSE(session_->MigrateToSocket(
+          std::move(new_socket), std::move(new_reader), std::move(new_writer)));
       EXPECT_FALSE(socket_data.AllReadDataConsumed());
       EXPECT_FALSE(socket_data.AllWriteDataConsumed());
     }
   }
 }
 
 TEST_P(QuicChromiumClientSessionTest, MigrateToSocketReadError) {
   std::unique_ptr<QuicEncryptedPacket> client_ping(
       client_maker_.MakePingPacket(1, /*include_version=*/false));
   std::unique_ptr<QuicEncryptedPacket> server_ping(
@@ skipping 30 matching lines @@
   std::unique_ptr<QuicChromiumPacketReader> new_reader(
       new QuicChromiumPacketReader(new_socket.get(), &clock_, session_.get(),
                                    kQuicYieldAfterPacketsRead,
                                    QuicTime::Delta::FromMilliseconds(
                                        kQuicYieldAfterDurationMilliseconds),
                                    bound_net_log_.bound()));
   std::unique_ptr<QuicChromiumPacketWriter> new_writer(
       CreateQuicChromiumPacketWriter(new_socket.get(), session_.get()));
 
   // Store old socket and migrate session.
-  EXPECT_TRUE(session_->MigrateToSocket(std::move(new_socket),
-                                        std::move(new_reader),
-                                        std::move(new_writer), nullptr));
+  EXPECT_TRUE(session_->MigrateToSocket(
+      std::move(new_socket), std::move(new_reader), std::move(new_writer)));
+  // Spin message loop to complete migration.
+  base::RunLoop().RunUntilIdle();
 
   // Read error on old socket does not impact session.
   EXPECT_TRUE(socket_data_->IsPaused());
   socket_data_->Resume();
   EXPECT_TRUE(session_->connection()->connected());
   EXPECT_TRUE(new_socket_data.IsPaused());
   new_socket_data.Resume();
 
   // Read error on new socket causes session close.
   EXPECT_TRUE(new_socket_data.IsPaused());
   EXPECT_TRUE(session_->connection()->connected());
   new_socket_data.Resume();
   EXPECT_FALSE(session_->connection()->connected());
 
   EXPECT_TRUE(socket_data_->AllReadDataConsumed());
   EXPECT_TRUE(socket_data_->AllWriteDataConsumed());
   EXPECT_TRUE(new_socket_data.AllReadDataConsumed());
   EXPECT_TRUE(new_socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicChromiumClientSessionTest, MigrateToSocketWriteError) {

[Ryan Hamilton, 2016/09/12 01:32:57]

Do we not need this test 'cause the real tests are in the stream factory test?

[Jana, 2016/09/12 21:08:02]

Right, exactly.

-  Initialize();
-  CompleteCryptoHandshake();
-
-  std::unique_ptr<QuicEncryptedPacket> ping(
-      client_maker_.MakePingPacket(1, /*include_version=*/true));
-  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING, 0)};
-  MockWrite writes[] = {MockWrite(SYNCHRONOUS, ping->data(), ping->length(), 1),
-                        MockWrite(SYNCHRONOUS, ERR_FAILED, 2)};
-  SequencedSocketData socket_data(reads, arraysize(reads), writes,
-                                  arraysize(writes));
-  socket_factory_.AddSocketDataProvider(&socket_data);
-
-  // Create connected socket.
-  std::unique_ptr<DatagramClientSocket> new_socket =
-      socket_factory_.CreateDatagramClientSocket(DatagramSocket::DEFAULT_BIND,
-                                                 base::Bind(&base::RandInt),
-                                                 &net_log_, NetLog::Source());
-  EXPECT_THAT(new_socket->Connect(kIpEndPoint), IsOk());
-
-  // Create reader and writer.
-  std::unique_ptr<QuicChromiumPacketReader> new_reader(
-      new QuicChromiumPacketReader(new_socket.get(), &clock_, session_.get(),
-                                   kQuicYieldAfterPacketsRead,
-                                   QuicTime::Delta::FromMilliseconds(
-                                       kQuicYieldAfterDurationMilliseconds),
-                                   bound_net_log_.bound()));
-  std::unique_ptr<QuicChromiumPacketWriter> new_writer(
-      CreateQuicChromiumPacketWriter(new_socket.get(), session_.get()));
-
-  // Migrate session.
-  EXPECT_TRUE(session_->MigrateToSocket(std::move(new_socket),
-                                        std::move(new_reader),
-                                        std::move(new_writer), nullptr));
-
-  // Write error on new socket causes session close.
-  EXPECT_TRUE(session_->connection()->connected());
-  session_->connection()->SendPing();
-  EXPECT_FALSE(session_->connection()->connected());
-
-  EXPECT_TRUE(socket_data.AllReadDataConsumed());
-  EXPECT_TRUE(socket_data.AllWriteDataConsumed());
-}
-
 }  // namespace
 }  // namespace test
 }  // namespace net