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

net/quic/quic_network_transaction_unittest.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 <vector>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
@@ skipping 216 matching lines @@
   }
 
   scoped_ptr<QuicEncryptedPacket> ConstructConnectionClosePacket(
       QuicPacketNumber num) {
     return maker_.MakeConnectionClosePacket(num);
   }
 
   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(
       QuicPacketNumber num,
       QuicTime::Delta delta_time_largest_observed,
       QuicPacketNumber largest_received,
       QuicPacketNumber least_unacked,
       QuicErrorCode quic_error,
       std::string& quic_error_details) {
     return maker_.MakeAckAndConnectionClosePacket(
         num, false, delta_time_largest_observed, largest_received,
         least_unacked, quic_error, quic_error_details);
   }
 
   scoped_ptr<QuicEncryptedPacket> ConstructRstPacket(
       QuicPacketNumber num,
       bool include_version,
       QuicStreamId stream_id,
       QuicRstStreamErrorCode error_code) {
     return maker_.MakeRstPacket(num, include_version, stream_id, error_code);
   }
 
   SpdyHeaderBlock GetRequestHeaders(const std::string& method,
                                     const std::string& scheme,
                                     const std::string& path) {
     return maker_.GetRequestHeaders(method, scheme, path);
   }
 
   SpdyHeaderBlock GetResponseHeaders(const std::string& status) {
     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,
       bool should_include_version,
       bool fin,
       QuicStreamOffset offset,
       base::StringPiece data) {
     return maker_.MakeDataPacket(packet_number, stream_id,
                                  should_include_version, fin, offset, data);
   }
 
   scoped_ptr<QuicEncryptedPacket> ConstructRequestHeadersPacket(
       QuicPacketNumber packet_number,
       QuicStreamId stream_id,
       bool should_include_version,
       bool fin,
+      const SpdyHeaderBlock& headers,
+      QuicStreamOffset* offset) {
+    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,
+      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) {
-    SpdyPriority priority =
-        ConvertRequestPriorityToQuicPriority(DEFAULT_PRIORITY);
-    return maker_.MakeRequestHeadersPacket(packet_number, stream_id,
-                                           should_include_version, fin,
-                                           priority, 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(
       QuicPacketNumber packet_number,
       QuicStreamId stream_id,
       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() {
     CreateSessionWithFactory(&socket_factory_, false);
   }
 
   void CreateSessionWithNextProtos() {
     CreateSessionWithFactory(&socket_factory_, true);
   }
 
@@ skipping 123 matching lines @@
     EXPECT_EQ(QUIC, alternative_service_vector[0].protocol);
   }
 
   void AddHangingNonAlternateProtocolSocketData() {
     MockConnect hanging_connect(SYNCHRONOUS, ERR_IO_PENDING);
     hanging_data_.set_connect_data(hanging_connect);
     socket_factory_.AddSocketDataProvider(&hanging_data_);
     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_;
   MockClientSocketFactory socket_factory_;
   ProofVerifyDetailsChromium verify_details_;
   MockCryptoClientStreamFactory crypto_client_stream_factory_;
   MockHostResolver host_resolver_;
   MockCertVerifier cert_verifier_;
   TransportSecurityState transport_security_state_;
   scoped_ptr<CTVerifier> cert_transparency_verifier_;
@@ skipping 309 matching lines @@
   socket_factory_.AddSocketDataProvider(&http_data);
   socket_factory_.AddSSLSocketDataProvider(&ssl_data_);
 
   CreateSessionWithNextProtos();
 
   SendRequestAndExpectHttpResponse("hello world");
   SendRequestAndExpectHttpResponse("hello world");
 }
 
 // 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)};
 
   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;
+  // 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
 
   mock_quic_data.AddSocketDataToFactory(&socket_factory_);
 
   AddHangingNonAlternateProtocolSocketData();
   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"),
       MockRead(kQuicAlternativeServiceDifferentPortHeader),
       MockRead("hello world"),
       MockRead(SYNCHRONOUS, ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ),
       MockRead(ASYNC, OK)};
 
@@ skipping 1054 matching lines @@
   request_.url = GURL("https://www.example.org:443");
   AddHangingNonAlternateProtocolSocketData();
   CreateSessionWithNextProtos();
   AddQuicAlternateProtocolMapping(MockCryptoClientStream::CONFIRM_HANDSHAKE);
   SendRequestAndExpectQuicResponse("hello!");
   EXPECT_TRUE(rtt_observer_.rtt_notification_received());
 }
 
 }  // namespace test
 }  // namespace net