Makes migration on write error asynchronous to avoid reentrancy issues

net/quic/chromium/quic_stream_factory_test.cc

Index: net/quic/chromium/quic_stream_factory_test.cc
diff --git a/net/quic/chromium/quic_stream_factory_test.cc b/net/quic/chromium/quic_stream_factory_test.cc
index 2ca8dbe9f779857174a7f32dec655cf4b8cc3dba..97413e245f6ea790a6c32c65dca242990f1ce466 100644
--- a/net/quic/chromium/quic_stream_factory_test.cc
+++ b/net/quic/chromium/quic_stream_factory_test.cc
@@ -507,6 +507,17 @@ class QuicStreamFactoryTestBase {
     EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
   }
 
+  // Helper methods for tests of connection migration on write error.

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

These methods all test some specific condition, right? In that case I'd probably
start them with a prefix like 

TestMigrationOn..

  TestMigrationOnWriteErrorNonMigratableStream
  TestMigrationOnWriteErrorMigrationDisabled
  TestMigrationOnMultipleWriteErrors
  TestMigrationOnNetworkDisconnected
  TestMigrationOnNetworkMadeDefault

The other way to do this, by the way, is to extend QuicStreamFactoryTestBase for
each of these and make it a TEST_P with appropriate Params. You approach is fine
for now. Just something to think about down the road.

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

Changed to TestMigrationOn. I thought about TEST_P, but this file already runs
~1200 tests, and these combinations do not make sense for most tests, so I used
the poor man's method.

+  void MigrateSessionOnWriteErrorNonMigratableStream(IoMode mode);

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

I assume |mode| here is the write error io mode? perhaps write_error_mode?

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

Done.

+  void MigrateSessionOnWriteErrorMigrationDisabled(IoMode mode);
+  void MigrateSessionOnWriteError(IoMode mode);
+  void MigrateSessionOnWriteErrorNoNewNetwork(IoMode mode);
+  void MigrateSessionOnMultipleWriteErrors(IoMode mode1, IoMode mode2);

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

Perhaps first_write_error_mode and second_write_error_mode?

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

Done.

+  void MigrateSessionOnWriteErrorWithNotificationQueued(bool disconnected);
+  void MigrateSessionOnNotificationWithWriteErrorQueued(bool disconnected);
+  void OnNetworkDisconnected(bool async_write_before);
+  void OnNetworkMadeDefault(bool async_write_before);
+
   MockHostResolver host_resolver_;
   scoped_refptr<SSLConfigService> ssl_config_service_;
   MockClientSocketFactory socket_factory_;
@@ -1460,17 +1471,30 @@ TEST_P(QuicStreamFactoryTest, OnIPAddressChanged) {
   EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeNetworkMadeDefault) {
+TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultWithSynchronousWriteBefore) {
+  OnNetworkMadeDefault(/*async_write_before=*/false);
+}
+
+TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultWithAsyncWriteBefore) {
+  OnNetworkMadeDefault(/*async_write_before=*/true);
+}

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

nit: Can you move the new tests below the helper method. Not that it really
matters. (here and elsewhere)

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

Done.

+
+void QuicStreamFactoryTestBase::OnNetworkMadeDefault(bool async_write_before) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
+  int packet_number = 1;
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(
-      ConstructGetRequestPacket(1, kClientDataStreamId1, true, true));
+  socket_data.AddWrite(ConstructGetRequestPacket(
+      packet_number++, kClientDataStreamId1, true, true));
+  if (async_write_before) {
+    socket_data.AddWrite(ASYNC, OK);

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

If this is going to be a write of a ping packet, can you make this actually be
an AddWrite with a PingPacket? (or does that cause problems 'cause the write
never actually completes?

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

Right exactly, I don't know how to make it expect a PING packet on an ASYNC
write.

+    packet_number++;
+  }
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -1501,16 +1525,21 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeNetworkMadeDefault) {
   EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
                                     callback_.callback()));
 
+  // Do an async write to leave writer blocked.
+  if (async_write_before)
+    session->connection()->SendPing();
+
   // Set up second socket data provider that is used after migration.
   // The response to the earlier request is read on this new socket.
   MockQuicData socket_data1;
   socket_data1.AddWrite(
-      client_maker_.MakePingPacket(2, /*include_version=*/true));
+      client_maker_.MakePingPacket(packet_number++, /*include_version=*/true));
   socket_data1.AddRead(
       ConstructOkResponsePacket(1, kClientDataStreamId1, false, false));
   socket_data1.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data1.AddWrite(client_maker_.MakeAckAndRstPacket(
-      3, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1, 1, true));
+      packet_number++, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1,
+      1, true));
   socket_data1.AddSocketDataToFactory(&socket_factory_);
 
   // Trigger connection migration. This should cause a PING frame
@@ -1567,17 +1596,30 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeNetworkMadeDefault) {
   EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnected) {
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedWithSynchronousWriteBefore) {
+  OnNetworkDisconnected(/*async_write_before=*/false);
+}
+
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedWithAsyncWriteBefore) {
+  OnNetworkDisconnected(/*async_write_before=*/true);
+}
+
+void QuicStreamFactoryTestBase::OnNetworkDisconnected(bool async_write_before) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
+  int packet_number = 1;
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(
-      ConstructGetRequestPacket(1, kClientDataStreamId1, true, true));
+  socket_data.AddWrite(ConstructGetRequestPacket(
+      packet_number++, kClientDataStreamId1, true, true));
+  if (async_write_before) {
+    socket_data.AddWrite(ASYNC, OK);
+    packet_number++;
+  }
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -1608,14 +1650,21 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnected) {
   EXPECT_EQ(OK, stream->SendRequest(request_headers, &response_info,
                                     callback_.callback()));
 
+  // Do an async write to leave writer blocked.
+  if (async_write_before)
+    session->connection()->SendPing();
+
+  // Set up second socket data provider that is used after migration.
+  // The response to the earlier request is read on this new socket.
   MockQuicData socket_data1;
   socket_data1.AddWrite(
-      client_maker_.MakePingPacket(2, /*include_version=*/true));
+      client_maker_.MakePingPacket(packet_number++, /*include_version=*/true));
   socket_data1.AddRead(
       ConstructOkResponsePacket(1, kClientDataStreamId1, false, false));
   socket_data1.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
   socket_data1.AddWrite(client_maker_.MakeAckAndRstPacket(
-      3, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1, 1, true));
+      packet_number++, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1,
+      1, true));
   socket_data1.AddSocketDataToFactory(&socket_factory_);
 
   // Trigger connection migration. This should cause a PING frame
@@ -1659,7 +1708,7 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnected) {
   EXPECT_TRUE(socket_data2.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNoNetworks) {
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedNoNetworks) {
   NetworkChangeNotifier::NetworkList no_networks(0);
   InitializeConnectionMigrationTest(no_networks);
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1704,7 +1753,7 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNoNetworks) {
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNoNewNetwork) {
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedNoNewNetwork) {
   InitializeConnectionMigrationTest({kDefaultNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
@@ -1748,8 +1797,7 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNoNewNetwork) {
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest,
-       OnNetworkChangeNetworkMadeDefaultNonMigratableStream) {
+TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultNonMigratableStream) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1797,8 +1845,7 @@ TEST_P(QuicStreamFactoryTest,
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest,
-       OnNetworkChangeNetworkMadeDefaultConnectionMigrationDisabled) {
+TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultConnectionMigrationDisabled) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1849,7 +1896,7 @@ TEST_P(QuicStreamFactoryTest,
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNonMigratableStream) {
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedNonMigratableStream) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1896,7 +1943,7 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNonMigratableStream) {
 }
 
 TEST_P(QuicStreamFactoryTest,
-       OnNetworkChangeDisconnectedConnectionMigrationDisabled) {
+       OnNetworkDisconnectedConnectionMigrationDisabled) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1945,7 +1992,7 @@ TEST_P(QuicStreamFactoryTest,
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeNetworkMadeDefaultNoOpenStreams) {
+TEST_P(QuicStreamFactoryTest, OnNetworkMadeDefaultNoOpenStreams) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -1982,7 +2029,7 @@ TEST_P(QuicStreamFactoryTest, OnNetworkChangeNetworkMadeDefaultNoOpenStreams) {
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, OnNetworkChangeDisconnectedNoOpenStreams) {
+TEST_P(QuicStreamFactoryTest, OnNetworkDisconnectedNoOpenStreams) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -2412,7 +2459,15 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionEarlyConnectionMigrationDisabled) {
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteError) {
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorSynchronous) {
+  MigrateSessionOnWriteError(SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorAsync) {
+  MigrateSessionOnWriteError(ASYNC);
+}
+
+void QuicStreamFactoryTestBase::MigrateSessionOnWriteError(IoMode mode) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -2421,7 +2476,7 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteError) {
 
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddWrite(mode, ERR_ADDRESS_UNREACHABLE);
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -2466,8 +2521,8 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteError) {
   EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
                                     callback_.callback()));
 
-  // Run the message loop so that data queued in the new socket is read by the
-  // packet reader.
+  // Run the message loop so that the migration attempt is executed and
+  // data queued in the new socket is read by the packet reader.
   base::RunLoop().RunUntilIdle();
 
   // The session should now be marked as going away. Ensure that
@@ -2489,14 +2544,24 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteError) {
   EXPECT_TRUE(socket_data1.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNoNewNetwork) {
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorNoNewNetworkSynchronous) {
+  MigrateSessionOnWriteErrorNoNewNetwork(SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNoNewNetworkAsync) {
+  MigrateSessionOnWriteErrorNoNewNetwork(ASYNC);
+}
+
+void QuicStreamFactoryTestBase::MigrateSessionOnWriteErrorNoNewNetwork(
+    IoMode mode) {
   InitializeConnectionMigrationTest({kDefaultNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
   crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
 
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddWrite(mode, ERR_ADDRESS_UNREACHABLE);
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -2525,18 +2590,31 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNoNewNetwork) {
   // a connection migration attempt.
   HttpResponseInfo response;
   HttpRequestHeaders request_headers;
-  EXPECT_EQ(
-      ERR_QUIC_PROTOCOL_ERROR,
-      stream->SendRequest(request_headers, &response, callback_.callback()));
-
-  // Migration fails, and session is marked as going away.
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
+  // Run message loop to execute migration attempt.
+  base::RunLoop().RunUntilIdle();
+  // Migration fails, and session is closed and deleted.
+  EXPECT_FALSE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
   EXPECT_FALSE(HasActiveSession(host_port_pair_));
 
   EXPECT_TRUE(socket_data.AllReadDataConsumed());
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNonMigratableStream) {
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorNonMigratableStreamSynchronous) {
+  MigrateSessionOnWriteErrorNonMigratableStream(SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorNonMigratableStreamAsync) {
+  MigrateSessionOnWriteErrorNonMigratableStream(ASYNC);
+}
+
+void QuicStreamFactoryTestBase::MigrateSessionOnWriteErrorNonMigratableStream(
+    IoMode mode) {
+  DVLOG(1) << "Mode: " << ((mode == SYNCHRONOUS) ? "SYNCHRONOUS" : "ASYNC");
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -2544,7 +2622,7 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNonMigratableStream) {
 
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddWrite(mode, ERR_ADDRESS_UNREACHABLE);
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -2574,18 +2652,32 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorNonMigratableStream) {
   // a connection migration attempt.
   HttpResponseInfo response;
   HttpRequestHeaders request_headers;
-  EXPECT_EQ(
-      ERR_QUIC_PROTOCOL_ERROR,
-      stream->SendRequest(request_headers, &response, callback_.callback()));
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
 
-  // Migration fails, and session is marked as going away.
+  // Run message loop to execute migration attempt.
+  base::RunLoop().RunUntilIdle();
+
+  // Migration fails, and session is closed and deleted.
+  EXPECT_FALSE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
   EXPECT_FALSE(HasActiveSession(host_port_pair_));
 
   EXPECT_TRUE(socket_data.AllReadDataConsumed());
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
 }
 
-TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorMigrationDisabled) {
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorMigrationDisabledSynchronous) {
+  MigrateSessionOnWriteErrorMigrationDisabled(SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorMigrationDisabledAsync) {
+  MigrateSessionOnWriteErrorMigrationDisabled(ASYNC);
+}
+
+void QuicStreamFactoryTestBase::MigrateSessionOnWriteErrorMigrationDisabled(
+    IoMode mode) {
   InitializeConnectionMigrationTest(
       {kDefaultNetworkForTests, kNewNetworkForTests});
   ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
@@ -2593,7 +2685,7 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorMigrationDisabled) {
 
   MockQuicData socket_data;
   socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
-  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddWrite(mode, ERR_ADDRESS_UNREACHABLE);
   socket_data.AddSocketDataToFactory(&socket_factory_);
 
   // Create request and QuicHttpStream.
@@ -2626,15 +2718,295 @@ TEST_P(QuicStreamFactoryTest, MigrateSessionOnWriteErrorMigrationDisabled) {
   // a connection migration attempt.
   HttpResponseInfo response;
   HttpRequestHeaders request_headers;
-  EXPECT_EQ(
-      ERR_QUIC_PROTOCOL_ERROR,
-      stream->SendRequest(request_headers, &response, callback_.callback()));
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
+  // Run message loop to execute migration attempt.
+  base::RunLoop().RunUntilIdle();
+  // Migration fails, and session is closed and deleted.
+  EXPECT_FALSE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_FALSE(HasActiveSession(host_port_pair_));
+  EXPECT_TRUE(socket_data.AllReadDataConsumed());
+  EXPECT_TRUE(socket_data.AllWriteDataConsumed());
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnMultipleWriteErrorsSyncSync) {
+  MigrateSessionOnMultipleWriteErrors(SYNCHRONOUS, SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnMultipleWriteErrorsSyncAsync) {
+  MigrateSessionOnMultipleWriteErrors(SYNCHRONOUS, ASYNC);
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnMultipleWriteErrorsAsyncSync) {
+  MigrateSessionOnMultipleWriteErrors(ASYNC, SYNCHRONOUS);
+}
+
+TEST_P(QuicStreamFactoryTest, MigrateSessionOnMultipleWriteErrorsAsyncAsync) {
+  MigrateSessionOnMultipleWriteErrors(ASYNC, ASYNC);
+}
+
+void QuicStreamFactoryTestBase::MigrateSessionOnMultipleWriteErrors(
+    IoMode mode1,
+    IoMode mode2) {
+  const int kMaxReadersPerQuicSession = 5;
+  InitializeConnectionMigrationTest(
+      {kDefaultNetworkForTests, kNewNetworkForTests});
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+
+  // Set up kMaxReadersPerQuicSession socket data providers, since migration
+  // will
+  // cause kMaxReadersPerQuicSession write failures as the session hops
+  // repeatedly
+  // between the two networks.

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

nit wrapping is off

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

Ah -- sent comment > 80 columns through git cl format. Fixed.

+  MockQuicData socket_data[kMaxReadersPerQuicSession + 1];
+  for (int i = 0; i <= kMaxReadersPerQuicSession; ++i) {
+    // The last socket is created but never used.
+    if (i < kMaxReadersPerQuicSession) {
+      socket_data[i].AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+      socket_data[i].AddWrite((i % 2 == 0) ? mode1 : mode2, ERR_FAILED);
+    }
+    socket_data[i].AddSocketDataToFactory(&socket_factory_);
+  }
+
+  // Create request and QuicHttpStream.
+  QuicStreamRequest request(factory_.get());
+  EXPECT_EQ(ERR_IO_PENDING,
+            request.Request(host_port_pair_, privacy_mode_,
+                            /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                            callback_.callback()));
+  EXPECT_EQ(OK, callback_.WaitForResult());
+  std::unique_ptr<QuicHttpStream> stream = request.CreateStream();
+  EXPECT_TRUE(stream.get());
+
+  // Cause QUIC stream to be created.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.org/");
+  EXPECT_EQ(OK, stream->InitializeStream(&request_info, DEFAULT_PRIORITY,
+                                         net_log_, CompletionCallback()));
+
+  // Ensure that session is alive and active.
+  QuicChromiumClientSession* session = GetActiveSession(host_port_pair_);
+  EXPECT_TRUE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_TRUE(HasActiveSession(host_port_pair_));
+
+  // Send GET request on stream. This should cause a write error, which triggers
+  // a connection migration attempt.
+  HttpResponseInfo response;
+  HttpRequestHeaders request_headers;
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
+
+  // Run the message loop so that data queued in the new socket is read by the
+  // packet reader.
+  base::RunLoop().RunUntilIdle();
+
+  // The connection should be closed because of a write error after migration.
+  EXPECT_FALSE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_FALSE(HasActiveSession(host_port_pair_));
+
+  //  EXPECT_EQ(ERR_QUIC_PROTOCOL_ERROR, callback_.WaitForResult());
+  //  EXPECT_EQ(ERR_QUIC_PROTOCOL_ERROR,
+  //            stream->ReadResponseHeaders(callback_.callback()));

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

Should this be commented back in?

(it'd be great to have ERR_NETWORK_SOMETHING instead of PROTOCOL_ERROR if it's
trivial)

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

Fixed test to be more sensible. I don't think this should be
ERR_NETWORK_SOMETHING since it's basically a write error. At the moment, all
write errors are collapsed into QUIC_PROTOCOL_ERROR. 

+
+  stream.reset();
+  for (int i = 0; i <= kMaxReadersPerQuicSession; ++i) {
+    DLOG(INFO) << "Socket number: " << i;
+    EXPECT_TRUE(socket_data[i].AllReadDataConsumed());
+    EXPECT_TRUE(socket_data[i].AllWriteDataConsumed());
+  }
+}
+
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorWithNetworkDisconnectedQueued) {
+  MigrateSessionOnWriteErrorWithNotificationQueued(/*disconnected=*/true);
+}
+
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnWriteErrorWithNetworkMadeDefaultQueued) {
+  MigrateSessionOnWriteErrorWithNotificationQueued(/*disconnected=*/false);
+}
+
+void QuicStreamFactoryTestBase::
+    MigrateSessionOnWriteErrorWithNotificationQueued(bool disconnected) {
+  InitializeConnectionMigrationTest(
+      {kDefaultNetworkForTests, kNewNetworkForTests});
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+
+  MockQuicData socket_data;
+  socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddSocketDataToFactory(&socket_factory_);
+
+  // Create request and QuicHttpStream.
+  QuicStreamRequest request(factory_.get());
+  EXPECT_EQ(ERR_IO_PENDING,
+            request.Request(host_port_pair_, privacy_mode_,
+                            /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                            callback_.callback()));
+  EXPECT_EQ(OK, callback_.WaitForResult());
+  std::unique_ptr<QuicHttpStream> stream = request.CreateStream();
+  EXPECT_TRUE(stream.get());
+
+  // Cause QUIC stream to be created.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.org/");
+  EXPECT_EQ(OK, stream->InitializeStream(&request_info, DEFAULT_PRIORITY,
+                                         net_log_, CompletionCallback()));
+
+  // Ensure that session is alive and active.
+  QuicChromiumClientSession* session = GetActiveSession(host_port_pair_);
+  EXPECT_TRUE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_TRUE(HasActiveSession(host_port_pair_));
+
+  // Set up second socket data provider that is used after
+  // migration. The request is rewritten to this new socket, and the
+  // response to the request is read on this new socket.
+  MockQuicData socket_data1;
+  socket_data1.AddWrite(
+      ConstructGetRequestPacket(1, kClientDataStreamId1, true, true));
+  socket_data1.AddRead(
+      ConstructOkResponsePacket(1, kClientDataStreamId1, false, false));
+  socket_data1.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data1.AddWrite(client_maker_.MakeAckAndRstPacket(
+      2, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1, 1, true));
+  socket_data1.AddSocketDataToFactory(&socket_factory_);
+
+  // First queue a network change notification in the message loop.
+  if (disconnected) {
+    scoped_mock_network_change_notifier_->mock_network_change_notifier()
+        ->QueueNetworkDisconnected(kDefaultNetworkForTests);
+  } else {
+    scoped_mock_network_change_notifier_->mock_network_change_notifier()
+        ->QueueNetworkMadeDefault(kNewNetworkForTests);
+  }
+  // Send GET request on stream. This should cause a write error,
+  // which triggers a connection migration attempt. This will queue a
+  // migration attempt behind the notification in the message loop.
+  HttpResponseInfo response;
+  HttpRequestHeaders request_headers;
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
+
+  base::RunLoop().RunUntilIdle();
+  // The session should now be marked as going away. Ensure that
+  // while it is still alive, it is no longer active.
+  EXPECT_TRUE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_FALSE(HasActiveSession(host_port_pair_));
+  EXPECT_EQ(1u, session->GetNumActiveStreams());
+
+  // Verify that response headers on the migrated socket were delivered to the
+  // stream.
+  EXPECT_EQ(OK, stream->ReadResponseHeaders(callback_.callback()));
+  EXPECT_EQ(200, response.headers->response_code());
+
+  stream.reset();
+
+  EXPECT_TRUE(socket_data.AllReadDataConsumed());
+  EXPECT_TRUE(socket_data.AllWriteDataConsumed());
+  EXPECT_TRUE(socket_data1.AllReadDataConsumed());
+  EXPECT_TRUE(socket_data1.AllWriteDataConsumed());
+}
+
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnNetworkDisconnectedWithWriteErrorQueued) {
+  MigrateSessionOnNotificationWithWriteErrorQueued(/*disconnected=*/true);
+}
 
-  // Migration fails, and session is marked as going away.
+TEST_P(QuicStreamFactoryTest,
+       MigrateSessionOnNetworkMadeDefaultWithWriteErrorQueued) {
+  MigrateSessionOnNotificationWithWriteErrorQueued(/*disconnected=*/true);
+}
+
+void QuicStreamFactoryTestBase::
+    MigrateSessionOnNotificationWithWriteErrorQueued(bool disconnected) {
+  InitializeConnectionMigrationTest(
+      {kDefaultNetworkForTests, kNewNetworkForTests});
+  ProofVerifyDetailsChromium verify_details = DefaultProofVerifyDetails();
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+  crypto_client_stream_factory_.AddProofVerifyDetails(&verify_details);
+
+  MockQuicData socket_data;
+  socket_data.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data.AddWrite(SYNCHRONOUS, ERR_ADDRESS_UNREACHABLE);
+  socket_data.AddSocketDataToFactory(&socket_factory_);
+
+  // Create request and QuicHttpStream.
+  QuicStreamRequest request(factory_.get());
+  EXPECT_EQ(ERR_IO_PENDING,
+            request.Request(host_port_pair_, privacy_mode_,
+                            /*cert_verify_flags=*/0, url_, "GET", net_log_,
+                            callback_.callback()));
+  EXPECT_EQ(OK, callback_.WaitForResult());
+  std::unique_ptr<QuicHttpStream> stream = request.CreateStream();
+  EXPECT_TRUE(stream.get());
+
+  // Cause QUIC stream to be created.
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.example.org/");
+  EXPECT_EQ(OK, stream->InitializeStream(&request_info, DEFAULT_PRIORITY,
+                                         net_log_, CompletionCallback()));
+
+  // Ensure that session is alive and active.
+  QuicChromiumClientSession* session = GetActiveSession(host_port_pair_);
+  EXPECT_TRUE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
+  EXPECT_TRUE(HasActiveSession(host_port_pair_));
+
+  // Set up second socket data provider that is used after
+  // migration. The request is rewritten to this new socket, and the
+  // response to the request is read on this new socket.
+  MockQuicData socket_data1;
+  socket_data1.AddWrite(
+      ConstructGetRequestPacket(1, kClientDataStreamId1, true, true));
+  socket_data1.AddRead(
+      ConstructOkResponsePacket(1, kClientDataStreamId1, false, false));
+  socket_data1.AddRead(SYNCHRONOUS, ERR_IO_PENDING);
+  socket_data1.AddWrite(client_maker_.MakeAckAndRstPacket(
+      2, false, kClientDataStreamId1, QUIC_STREAM_CANCELLED, 1, 1, 1, true));
+  socket_data1.AddSocketDataToFactory(&socket_factory_);
+
+  // Send GET request on stream. This should cause a write error,
+  // which triggers a connection migration attempt. This will queue a
+  // migration attempt in the message loop.
+  HttpResponseInfo response;
+  HttpRequestHeaders request_headers;
+  EXPECT_EQ(OK, stream->SendRequest(request_headers, &response,
+                                    callback_.callback()));
+
+  // Now queue a network change notification in the message loop behind
+  // the migration attempt.
+  if (disconnected) {
+    scoped_mock_network_change_notifier_->mock_network_change_notifier()
+        ->QueueNetworkDisconnected(kDefaultNetworkForTests);
+  } else {
+    scoped_mock_network_change_notifier_->mock_network_change_notifier()
+        ->QueueNetworkMadeDefault(kNewNetworkForTests);
+  }
+
+  base::RunLoop().RunUntilIdle();
+  // The session should now be marked as going away. Ensure that
+  // while it is still alive, it is no longer active.
+  EXPECT_TRUE(QuicStreamFactoryPeer::IsLiveSession(factory_.get(), session));
   EXPECT_FALSE(HasActiveSession(host_port_pair_));
+  EXPECT_EQ(1u, session->GetNumActiveStreams());
+
+  // Verify that response headers on the migrated socket were delivered to the
+  // stream.
+  EXPECT_EQ(OK, stream->ReadResponseHeaders(callback_.callback()));
+  EXPECT_EQ(200, response.headers->response_code());
+
+  stream.reset();
 
   EXPECT_TRUE(socket_data.AllReadDataConsumed());
   EXPECT_TRUE(socket_data.AllWriteDataConsumed());
+  EXPECT_TRUE(socket_data1.AllReadDataConsumed());
+  EXPECT_TRUE(socket_data1.AllWriteDataConsumed());
 }
 
 TEST_P(QuicStreamFactoryTest, MigrateSessionEarlyToBadSocket) {