Change the interface of GetAlternativeServicesFor, always return the best Alt-Svc entry.

net/quic/quic_network_transaction_unittest.cc

Index: net/quic/quic_network_transaction_unittest.cc
diff --git a/net/quic/quic_network_transaction_unittest.cc b/net/quic/quic_network_transaction_unittest.cc
index fa5503a031989af238f80aea12e56f482e5ec4d2..da2d4d46303a5196c059ca4705b840f1d0eeb039 100644
--- a/net/quic/quic_network_transaction_unittest.cc
+++ b/net/quic/quic_network_transaction_unittest.cc
@@ -234,7 +234,25 @@ class QuicNetworkTransactionTest
   scoped_ptr<QuicEncryptedPacket> ConstructAckPacket(
       QuicPacketNumber largest_received,
       QuicPacketNumber least_unacked) {
-    return maker_.MakeAckPacket(2, largest_received, least_unacked, true);
+    return maker_.MakeAckPacket(2, largest_received, least_unacked,
+                                least_unacked, true);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructAckPacket(
+      QuicPacketNumber largest_received,
+      QuicPacketNumber least_unacked,
+      QuicTestPacketMaker* maker) {
+    return maker->MakeAckPacket(2, largest_received, least_unacked,
+                                least_unacked, true);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructAckAndConnectionClosePacket(
+      QuicPacketNumber packet_number,
+      QuicPacketNumber largest_received,
+      QuicPacketNumber ack_least_unacked,
+      QuicPacketNumber stop_least_unacked) {
+    return maker_.MakeAckPacket(packet_number, largest_received,
+                                ack_least_unacked, stop_least_unacked, true);
   }
 
   scoped_ptr<QuicEncryptedPacket> ConstructAckAndConnectionClosePacket(
@@ -267,6 +285,11 @@ class QuicNetworkTransactionTest
     return maker_.GetResponseHeaders(status);
   }
 
+  SpdyHeaderBlock GetResponseHeaders(const std::string& status,
+                                     const std::string& alt_svc) {
+    return maker_.GetResponseHeaders(status, alt_svc);
+  }
+
   scoped_ptr<QuicEncryptedPacket> ConstructDataPacket(
       QuicPacketNumber packet_number,
       QuicStreamId stream_id,
@@ -283,12 +306,50 @@ class QuicNetworkTransactionTest
       QuicStreamId stream_id,
       bool should_include_version,
       bool fin,
-      const SpdyHeaderBlock& headers) {
+      const SpdyHeaderBlock& headers,
+      QuicStreamOffset* offset) {
     SpdyPriority priority =
         ConvertRequestPriorityToQuicPriority(DEFAULT_PRIORITY);
-    return maker_.MakeRequestHeadersPacket(packet_number, stream_id,
-                                           should_include_version, fin,
-                                           priority, headers);
+    return maker_.MakeRequestHeadersPacketWithOffsetTracking(
+        packet_number, stream_id, should_include_version, fin, priority,
+        headers, offset);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructRequestHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicStreamOffset* offset,
+      QuicTestPacketMaker* maker) {
+    SpdyPriority priority =
+        ConvertRequestPriorityToQuicPriority(DEFAULT_PRIORITY);
+    return maker->MakeRequestHeadersPacketWithOffsetTracking(
+        packet_number, stream_id, should_include_version, fin, priority,
+        headers, offset);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructRequestHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers) {
+    return ConstructRequestHeadersPacket(packet_number, stream_id,
+                                         should_include_version, fin, headers,
+                                         nullptr, &maker_);
+  }
+  scoped_ptr<QuicEncryptedPacket> ConstructRequestHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicTestPacketMaker* maker) {
+    return ConstructRequestHeadersPacket(packet_number, stream_id,
+                                         should_include_version, fin, headers,
+                                         nullptr, maker);
   }
 
   scoped_ptr<QuicEncryptedPacket> ConstructResponseHeadersPacket(
@@ -297,8 +358,44 @@ class QuicNetworkTransactionTest
       bool should_include_version,
       bool fin,
       const SpdyHeaderBlock& headers) {
-    return maker_.MakeResponseHeadersPacket(
-        packet_number, stream_id, should_include_version, fin, headers);
+    return ConstructResponseHeadersPacket(packet_number, stream_id,
+                                          should_include_version, fin, headers,
+                                          nullptr, &maker_);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructResponseHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicTestPacketMaker* maker) {
+    return ConstructResponseHeadersPacket(packet_number, stream_id,
+                                          should_include_version, fin, headers,
+                                          nullptr, maker);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructResponseHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicStreamOffset* offset) {
+    return maker_.MakeResponseHeadersPacketWithOffsetTracking(
+        packet_number, stream_id, should_include_version, fin, headers, offset);
+  }
+
+  scoped_ptr<QuicEncryptedPacket> ConstructResponseHeadersPacket(
+      QuicPacketNumber packet_number,
+      QuicStreamId stream_id,
+      bool should_include_version,
+      bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicStreamOffset* offset,
+      QuicTestPacketMaker* maker) {
+    return maker->MakeResponseHeadersPacketWithOffsetTracking(
+        packet_number, stream_id, should_include_version, fin, headers, offset);
   }
 
   void CreateSession() {
@@ -442,6 +539,14 @@ class QuicNetworkTransactionTest
     socket_factory_.AddSSLSocketDataProvider(&ssl_data_);
   }
 
+  void AddNewHangingNonAlternateProtocolSocketData() {
+    MockConnect hanging_connect(SYNCHRONOUS, ERR_IO_PENDING);
+    StaticSocketDataProvider* hanging_data = new StaticSocketDataProvider();

[Ryan Hamilton, 2015/12/30 21:51:42]

I think this leaks memory, doesn't it?

[Zhongyi Shi, 2016/01/05 22:54:20]

Ahhh, yes. I fix it with passing the hanging_data from the args so that the
lifetime of hanging_data is the same with the tests.

[Ryan Hamilton, 2016/01/05 23:51:58]

Instead of requiring the caller to pass in a new DataProviders, how about making
hanging_data_ a vector and each time this method is called, pushing back onto
that vector?

[Zhongyi Shi, 2016/01/06 02:31:40]

The StaticSocketDataProvider is DISALLOW_COPY_AND_ASSIGN. So probably better to
keep it as it is?

[Ryan Hamilton, 2016/01/06 17:06:23]

That's true, but you can do 

std::vector<scoped_ptr<StaticSocketDataProvider>> hanging_data_;

and then and a new heap-allocated StaticSocketDataProvider.

+    hanging_data->set_connect_data(hanging_connect);
+    socket_factory_.AddSocketDataProvider(hanging_data);
+    socket_factory_.AddSSLSocketDataProvider(&ssl_data_);
+  }
+
   MockClock* clock_;  // Owned by QuicStreamFactory after CreateSession.
   QuicTestPacketMaker maker_;
   scoped_ptr<HttpNetworkSession> session_;
@@ -771,14 +876,13 @@ TEST_P(QuicNetworkTransactionTest,
 }
 
 // When multiple alternative services are advertised,
-// HttpStreamFactoryImpl::RequestStreamInternal() only passes the first one to
-// Job.  This is what the following test verifies.
-// TODO(bnc): Update this test when multiple alternative services are handled
-// properly.
-TEST_P(QuicNetworkTransactionTest, UseFirstAlternativeServiceForQuic) {
+// HttpStreamFactoryImpl::RequestStreamInternal() should select the alternative
+// service which uses existing QUIC session if available. If no existing QUIC
+// session can be used, use the first alternative service from the list.
+TEST_P(QuicNetworkTransactionTest, UseExistingAlternativeServiceForQuic) {
   MockRead http_reads[] = {
       MockRead("HTTP/1.1 200 OK\r\n"),
-      MockRead("Alt-Svc: quic=\":443\", quic=\":1234\"\r\n\r\n"),
+      MockRead("Alt-Svc: quic=\"foo.example.com:443\", quic=\":1234\"\r\n\r\n"),
       MockRead("hello world"),
       MockRead(SYNCHRONOUS, ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ),
       MockRead(ASYNC, OK)};
@@ -788,15 +892,38 @@ TEST_P(QuicNetworkTransactionTest, UseFirstAlternativeServiceForQuic) {
   socket_factory_.AddSocketDataProvider(&http_data);
   socket_factory_.AddSSLSocketDataProvider(&ssl_data_);
 
+  QuicStreamOffset request_header_offset = 0;
+  QuicStreamOffset response_header_offset = 0;
+  // First QUIC request data.
+  // Open a session to mail.example.org:443 using the first entry of the

[Ryan Hamilton, 2015/12/30 21:51:42]

This actually opens a QUIC session to foo.example.com, right?

[Zhongyi Shi, 2016/01/05 22:54:20]

Yes. Comment updated.

+  // alternative service list.
   MockQuicData mock_quic_data;
-  mock_quic_data.AddWrite(
-      ConstructRequestHeadersPacket(1, kClientDataStreamId1, true, true,
-                                    GetRequestHeaders("GET", "https", "/")));
+  mock_quic_data.AddWrite(ConstructRequestHeadersPacket(
+      1, kClientDataStreamId1, true, true,
+      GetRequestHeaders("GET", "https", "/"), &request_header_offset));
+
+  std::string alt_svc_list =
+      "quic=\"mail.example.com:1234\", quic=\"foo.example.com:443\", "
+      "quic=\"bar.example.com:8080\"";
   mock_quic_data.AddRead(ConstructResponseHeadersPacket(
-      1, kClientDataStreamId1, false, false, GetResponseHeaders("200 OK")));
+      1, kClientDataStreamId1, false, false,
+      GetResponseHeaders("200 OK", alt_svc_list), &response_header_offset));
   mock_quic_data.AddRead(
       ConstructDataPacket(2, kClientDataStreamId1, false, true, 0, "hello!"));
   mock_quic_data.AddWrite(ConstructAckPacket(2, 1));
+
+  // Second QUIC request data.
+  // Connection pooling, using existing session, no need to include version
+  // as version negotiation has been completed.
+  mock_quic_data.AddWrite(ConstructRequestHeadersPacket(
+      3, kClientDataStreamId2, false, true,
+      GetRequestHeaders("GET", "https", "/"), &request_header_offset));
+  mock_quic_data.AddRead(ConstructResponseHeadersPacket(
+      3, kClientDataStreamId2, false, false, GetResponseHeaders("200 OK"),
+      &response_header_offset));
+  mock_quic_data.AddRead(
+      ConstructDataPacket(4, kClientDataStreamId2, false, true, 0, "hello!"));
+  mock_quic_data.AddWrite(ConstructAckAndConnectionClosePacket(4, 4, 3, 1));
   mock_quic_data.AddRead(ASYNC, ERR_IO_PENDING);  // No more data to read
   mock_quic_data.AddRead(ASYNC, 0);               // EOF
 
@@ -806,9 +933,97 @@ TEST_P(QuicNetworkTransactionTest, UseFirstAlternativeServiceForQuic) {
   CreateSessionWithNextProtos();
 
   SendRequestAndExpectHttpResponse("hello world");
+
+  SendRequestAndExpectQuicResponseOnPort("hello!", 443);
   SendRequestAndExpectQuicResponseOnPort("hello!", 443);
 }
 
+// When multiple alternative services that has existing QUIC session.
+// HttpStreamFactoryImpl::RequestStreamInternal() should select the first
+// alternative service which uses existing QUIC session.
+TEST_P(QuicNetworkTransactionTest, UseFirstExistingAlternativeServiceForQuic) {
+  MockRead http_reads[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"),
+      MockRead("Alt-Svc: quic=\"foo.example.com:443\", quic=\":1234\"\r\n\r\n"),
+      MockRead("hello world"),
+      MockRead(SYNCHRONOUS, ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ),
+      MockRead(ASYNC, OK)};
+
+  StaticSocketDataProvider http_data(http_reads, arraysize(http_reads), nullptr,
+                                     0);
+  socket_factory_.AddSocketDataProvider(&http_data);
+  socket_factory_.AddSSLSocketDataProvider(&ssl_data_);
+
+  QuicStreamOffset request_header_offset = 0;
+  QuicStreamOffset response_header_offset = 0;
+
+  QuicTestPacketMaker maker(GetParam(), 0, clock_, kDefaultServerHostName);
+
+  MockQuicData mock_quic_data;
+  MockQuicData mock_quic_data2;
+  // First QUIC request data.
+  // Open a session to foo.example.com:443.
+  mock_quic_data.AddWrite(ConstructRequestHeadersPacket(
+      1, kClientDataStreamId1, true, true,
+      GetRequestHeaders("GET", "https", "/"), &request_header_offset));
+
+  std::string alt_svc_list =
+      "quic=\"bar.example.com:443\", quic=\"frog.example.com:443\", "

[Ryan Hamilton, 2015/12/30 21:51:42]

I think each alternative should have a different port, so that we can ensure
that the right alternative was used, via the port checks below.

[Zhongyi Shi, 2016/01/05 22:54:20]

Done.

+      "quic=\"mail.example.com:8080\"";
+  // Response header from the server resets the alt_svc list for the origin.
+  mock_quic_data.AddRead(ConstructResponseHeadersPacket(
+      1, kClientDataStreamId1, false, false,
+      GetResponseHeaders("200 OK", alt_svc_list), &response_header_offset));
+  mock_quic_data.AddRead(ConstructDataPacket(2, kClientDataStreamId1, false,
+                                             true, 0, "hello from foo!"));
+  mock_quic_data.AddWrite(ConstructAckPacket(2, 1));
+
+  // Second QUIC request data.
+  // No connection pooling. Open a new session to bar.example.com:443.

[Ryan Hamilton, 2015/12/30 21:51:42]

Can you expand on "No connection pooling". I think the idea is that the existing
quic session to foo.example.com is no longer a viable alt-svc.

[Zhongyi Shi, 2016/01/05 22:54:20]

Done.

+  mock_quic_data2.AddWrite(ConstructRequestHeadersPacket(
+      1, kClientDataStreamId1, true, true,
+      GetRequestHeaders("GET", "https", "/"), &maker));
+  alt_svc_list =
+      "quic=\"foo.example.com:443\", quic=\"mail.example.com:1234\", "
+      "quic=\"bar.example.com:443\"";
+  // Response header from the server resets the alt_svc list for the origin.
+  mock_quic_data2.AddRead(ConstructResponseHeadersPacket(
+      1, kClientDataStreamId1, false, false,
+      GetResponseHeaders("200 OK", alt_svc_list), &maker));
+  mock_quic_data2.AddRead(ConstructDataPacket(2, kClientDataStreamId1, false,
+                                              true, 0, "hello from bar!"));
+  mock_quic_data2.AddWrite(ConstructAckPacket(2, 1, &maker));
+  mock_quic_data2.AddRead(ASYNC, ERR_IO_PENDING);  // No more data to read
+  mock_quic_data2.AddRead(ASYNC, 0);               // EOF
+
+  // Third QUIC request data.
+  // Connection pooling, using the first existing session to foo.example.com
+  mock_quic_data.AddWrite(ConstructRequestHeadersPacket(
+      3, kClientDataStreamId2, false, true,
+      GetRequestHeaders("GET", "https", "/"), &request_header_offset));
+  mock_quic_data.AddRead(ConstructResponseHeadersPacket(
+      3, kClientDataStreamId2, false, false, GetResponseHeaders("200 OK"),
+      &response_header_offset));
+  mock_quic_data.AddRead(ConstructDataPacket(4, kClientDataStreamId2, false,
+                                             true, 0, "hello from foo!"));
+  mock_quic_data.AddWrite(ConstructAckAndConnectionClosePacket(4, 4, 3, 1));
+  mock_quic_data.AddRead(ASYNC, ERR_IO_PENDING);  // No more data to read
+  mock_quic_data.AddRead(ASYNC, 0);               // EOF
+
+  mock_quic_data.AddSocketDataToFactory(&socket_factory_);
+  AddHangingNonAlternateProtocolSocketData();
+
+  mock_quic_data2.AddSocketDataToFactory(&socket_factory_);
+  AddNewHangingNonAlternateProtocolSocketData();
+
+  CreateSessionWithNextProtos();
+
+  SendRequestAndExpectHttpResponse("hello world");
+  SendRequestAndExpectQuicResponseOnPort("hello from foo!", 443);
+  SendRequestAndExpectQuicResponseOnPort("hello from bar!", 443);
+  SendRequestAndExpectQuicResponseOnPort("hello from foo!", 443);

[Ryan Hamilton, 2015/12/30 21:51:42]

Overall this test look good. I wonder, though, if we should also have tests in
which the requests are to different origins. For example we could make a request
to www.example.org which makes a QUIC session to www.example.org. Then we could
make a requests to mail.example.org which has www.example.org (and itself)
listed as an alternative.

This is very similar to what you've done, it just happens slightly differently?

We should probably also have a test where a server specifies an alternative
which is not valid. For example if www.example.org said that www.example.com was
an alternative, and the connection to www.example.org did not have an acceptable
cert, then we should create a new session.

+}
+
 TEST_P(QuicNetworkTransactionTest, AlternativeServiceDifferentPort) {
   MockRead http_reads[] = {
       MockRead("HTTP/1.1 200 OK\r\n"),