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

components/cronet/android/cronet_url_request_context_adapter.cc

Index: components/cronet/android/cronet_url_request_context_adapter.cc
diff --git a/components/cronet/android/cronet_url_request_context_adapter.cc b/components/cronet/android/cronet_url_request_context_adapter.cc
index e8e65b4f1015fadeeceb35b90f27fd5f11111618..6e48c5b0b19b1dee3ab97a6930cbd16304edbda4 100644
--- a/components/cronet/android/cronet_url_request_context_adapter.cc
+++ b/components/cronet/android/cronet_url_request_context_adapter.cc
@@ -64,6 +64,7 @@
 #include "net/nqe/network_quality_estimator_params.h"
 #include "net/proxy/proxy_config_service_android.h"
 #include "net/proxy/proxy_service.h"
+#include "net/quic/core/quic_versions.h"
 #include "net/sdch/sdch_owner.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/url_request/url_request_context.h"
@@ -759,8 +760,17 @@ void CronetURLRequestContextAdapter::InitializeOnNetworkThread(
       url::SchemeHostPort quic_server("https", canon_host, quic_hint.port);
       net::AlternativeService alternative_service(
           net::kProtoQUIC, "", static_cast<uint16_t>(quic_hint.alternate_port));
-      context_->http_server_properties()->SetAlternativeService(
-          quic_server, alternative_service, base::Time::Max());
+      QuicVersionVector advertised_versions;
+      HttpNetworkSession::Params* params = context_->GetNetworkSessionParams();
+      if (params) {

[xunjieli, 2017/06/16 14:43:56]

|params| should be always non-null, right?
Can you make this into a DCHECK?

DCHECK(params);
context_->>http_server_properties()->SetQuicAlternativeService(
  quic_server, alternative_service, base::Time::Max(),
  params->advertised_versions);

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

Done.

+        advertised_versions = params->quic_supported_versions;
+      } else {
+        advertised_versions = CurrentSupportedVersions();
+      }
+
+      context_->http_server_properties()->SetQuicAlternativeService(

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

I believe this should be called with an empty QuicVersionVector, because we have
no information about what versions the server support.

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

Setting versions here doesn't have behavioral difference comparing to to setting
an empty version list. And it sticks to the API more closely.

When adding support for multiple QUIC versions, we are expecting always have the
advertised versions(and supported) persisted in server properties. The only
exception that version list is missing in server properties for quic servers
happens when we are restarting and updating Chrome from an older version(doesn't
support multi version) to a new version that supports multi version. But in
general, we always want to set the advertised versions when
SetAlternativeService.

There's not much behavior difference in setting an empty version list vs the
versions supported by the current network stack. In the current world, where the
multi version isn't a thing. We always try previously persisted alt svc with the
QUIC version supported by the current running instance of net stack. We never
persist version for alt svc, and if a Chrome update happens, we read the server
properties from the disk, try with a different supported version by the new
running instance of net stack, without realizing the version has been changed.

Additionally, if version list is missing(which happens when updating from a
Chrome not supporting multi version to supporting), we will treat the alt svc as
supporting the current versions the net stack supports in the QuicStreamFactory
layer.

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

What happens if one version of Chrome supports QUIC 37, so that is persisted to
disk, then after update, Chrome supports 38 but not 37, so the entry is
disregarded?  Because of this scenario, I still think it would be better to
persist without an empty version vector if we have no information about the
version.

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

Discussed offline. I have confirmed with xunjieli@: the supported versions will
not change dynamically. It's changed either with a library update or a app
config update (CronetEngine will pick up new configurations). Both require app
restart to take effect, where this method will be invoked again, and those
alternative services will be set again with new versions, thus this will not
break the Alt Svc usage.

[Zhongyi Shi, 2017/06/26 22:23:24]

As I realized version advertisement is optional. I felt it would be more
consistent to leave the version field empty. Which will not set up anything in
disk so that the server is kind of in a state supporting ANY QUIC version.

+          quic_server, alternative_service, base::Time::Max(),
+          advertised_versions);
     }
   }