Add and persist a new field in AlternativeServiceInfo to list QUIC verisons advertised

net/http/http_stream_factory_impl_job_controller_unittest.cc

Index: net/http/http_stream_factory_impl_job_controller_unittest.cc
diff --git a/net/http/http_stream_factory_impl_job_controller_unittest.cc b/net/http/http_stream_factory_impl_job_controller_unittest.cc
index 4160bc897bdfdc85d1042fcc16644583e74107a6..5fba751336c209e683e505651b11e9228e1e5346 100644
--- a/net/http/http_stream_factory_impl_job_controller_unittest.cc
+++ b/net/http/http_stream_factory_impl_job_controller_unittest.cc
@@ -152,10 +152,20 @@ class JobControllerPeer {
       HttpStreamFactoryImpl::JobController* job_controller) {
     return job_controller->main_job_is_blocked_;
   }
+
   static bool main_job_is_resumed(
       HttpStreamFactoryImpl::JobController* job_controller) {
     return job_controller->main_job_is_resumed_;
   }
+
+  static AlternativeServiceInfo GetAlternativeServiceInfoFor(
+      HttpStreamFactoryImpl::JobController* job_controller,
+      const HttpRequestInfo& request_info,
+      HttpStreamRequest::Delegate* delegate,
+      HttpStreamRequest::StreamType stream_type) {
+    return job_controller->GetAlternativeServiceInfoFor(request_info, delegate,
+                                                        stream_type);
+  }
 };
 
 class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
@@ -254,8 +264,14 @@ class HttpStreamFactoryImplJobControllerTest : public ::testing::Test {
     HostPortPair host_port_pair = HostPortPair::FromURL(request_info.url);
     url::SchemeHostPort server(request_info.url);
     base::Time expiration = base::Time::Now() + base::TimeDelta::FromDays(1);
-    session_->http_server_properties()->SetAlternativeService(
-        server, alternative_service, expiration);
+    if (alternative_service.protocol == kProtoQUIC) {
+      session_->http_server_properties()->SetQuicAlternativeService(
+          server, alternative_service, expiration,
+          session_->params().quic_supported_versions);
+    } else {
+      session_->http_server_properties()->SetHttp2AlternativeService(
+          server, alternative_service, expiration);
+    }
   }
 
   void VerifyBrokenAlternateProtocolMapping(const HttpRequestInfo& request_info,
@@ -1708,4 +1724,55 @@ TEST_P(HttpStreamFactoryImplJobControllerPreconnectTest,
   EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
 }
 
+// Test that GetAlternativeServiceInfoFor will include a list of advertised

[Bence, 2017/06/16 14:43:40]

Since GetAlternativeServiceInfoFor() only has one call site, which immediately
extracts .alternative_service and throws out the rest of the Info struct, maybe
it could be changed back to GetAlternativeServiceFor().  After all, the caller
really should not worry about what versions are supported, only whether there is
overlap with the client-supported versions.

[Zhongyi Shi, 2017/06/20 23:23:37]

Whoops. I probably should give some more context here. I agree with you that
currently GetAlternativeServiceInfoFor doesn't nothing as it drops the version
list. However, in the next CL after this one is landed, the version list will be
taken by the JobController via AlternativeServiceInfo, then passed to
QuicStreamFactory so that a "best" QuicVersion will be picked based on the
advertised versions and the supported versions, and QuicStreamFactory will then
pass the selected version to QuicChomiumClientSession so that we will use
dynamic QUIC versions based on the server advertisement. 

[Bence, 2017/06/21 12:39:25]

Oh, I totally forgot about that.  Sorry.

Optionally, GetAlternativeServiceFor() could do the version matching, and return
the single best Alternative Service.  This has an advantage in case there are
two QUIC AltSvc entries for the same origin with different HostPortPairs and
with different versions: GetAlternativeServiceFor() could pick the one with the
version that is supported, but if the version matching logic lives in
QuicStreamFactory, then since GetAlternativeServiceInfoFor() only returns a
single entry, it might not return the best one.  Of course this is an edge case
which should be very rare.

[Zhongyi Shi, 2017/06/21 21:01:33]

Currently, we are returning a single AlternativerServiceInfo, but with all the
advertised versions. In most cases, if we didn't change the client supported
versions, we know at least one of the advertised versions will be found in the
AltSvcInfo. 

The edge case is that we might support for QUIC_VERSION_37 for a while, and
persisted a AlternativeService with QUIC_VERSION_37. And then we update to
support QUIC_VERSION_39. In the old world, we will ignorantly connect to the
server with QUIC_VERSION_39, which will fail later. In the new world, we will
create a QUIC job and until the QuicStreamFactory found no mutually supported
versions, we will could kill the quic job, mark QUIC broken. This is close to
what we were doing. 

+// versions. Returns an empty list if advertised versions are missing in
+// HttpServerProperties.
+TEST_F(HttpStreamFactoryImplJobControllerTest, GetAlternativeServiceInfoFor) {
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("https://www.google.com");
+
+  Initialize(request_info);
+  url::SchemeHostPort server(request_info.url);
+  AlternativeService alternative_service(kProtoQUIC, server.host(), 443);
+  HostPortPair host_port_pair = HostPortPair::FromURL(request_info.url);
+  base::Time expiration = base::Time::Now() + base::TimeDelta::FromDays(1);
+
+  // Set alternative service with no advertised version.
+  session_->http_server_properties()->SetQuicAlternativeService(
+      server, alternative_service, expiration, QuicVersionVector());
+
+  AlternativeServiceInfo alt_svc_info =
+      JobControllerPeer::GetAlternativeServiceInfoFor(
+          job_controller_, request_info, &request_delegate_,
+          HttpStreamRequest::HTTP_STREAM);
+  // Verify that JobController get an empty list of supported QUIC versions.
+  EXPECT_TRUE(alt_svc_info.advertised_versions().empty());
+
+  // Set alternative service for the same server with QUIC_VERSION_39 specified.
+  ASSERT_TRUE(session_->http_server_properties()->SetQuicAlternativeService(
+      server, alternative_service, expiration, {QUIC_VERSION_39}));
+
+  alt_svc_info = JobControllerPeer::GetAlternativeServiceInfoFor(
+      job_controller_, request_info, &request_delegate_,
+      HttpStreamRequest::HTTP_STREAM);
+  EXPECT_EQ(1u, alt_svc_info.advertised_versions().size());
+  // Verify that JobController returns the single version specified in set.
+  EXPECT_EQ(QUIC_VERSION_39, alt_svc_info.advertised_versions()[0]);
+
+  // Set alternative service for the same server with two QUIC versions:
+  // QUIC_VERSION_35, QUIC_VERSION_39.
+  ASSERT_TRUE(session_->http_server_properties()->SetQuicAlternativeService(
+      server, alternative_service, expiration,
+      {QUIC_VERSION_35, QUIC_VERSION_39}));
+
+  alt_svc_info = JobControllerPeer::GetAlternativeServiceInfoFor(
+      job_controller_, request_info, &request_delegate_,
+      HttpStreamRequest::HTTP_STREAM);
+  EXPECT_EQ(2u, alt_svc_info.advertised_versions().size());
+  // Verify that JobController returns the list of versions specified in set.
+  EXPECT_EQ(QUIC_VERSION_35, alt_svc_info.advertised_versions()[0]);
+  EXPECT_EQ(QUIC_VERSION_39, alt_svc_info.advertised_versions()[1]);
+}
+
 }  // namespace net