[Cronet] Write effective experimental options to NetLog

components/cronet/url_request_context_config.cc

Index: components/cronet/url_request_context_config.cc
diff --git a/components/cronet/url_request_context_config.cc b/components/cronet/url_request_context_config.cc
index 00979d384d94b0122da5b8e70dbfe4c7303e08f5..afa8ab76f6f8c21e540359472fc802ff02f10ef3 100644
--- a/components/cronet/url_request_context_config.cc
+++ b/components/cronet/url_request_context_config.cc
@@ -89,13 +89,14 @@ const char kDisableIPv6[] = "disable_ipv6";
 
 const char kSSLKeyLogFile[] = "ssl_key_log_file";
 
-void ParseAndSetExperimentalOptions(
+// Returns the effective experimental options.
+std::unique_ptr<base::DictionaryValue> ParseAndSetExperimentalOptions(
     const std::string& experimental_options,
     net::URLRequestContextBuilder* context_builder,
     net::NetLog* net_log,
     const scoped_refptr<base::SequencedTaskRunner>& file_task_runner) {
   if (experimental_options.empty())
-    return;
+    return nullptr;
 
   DCHECK(net_log);
 
@@ -106,7 +107,7 @@ void ParseAndSetExperimentalOptions(
   if (!options) {
     DCHECK(false) << "Parsing experimental options failed: "
                   << experimental_options;
-    return;
+    return nullptr;
   }
 
   std::unique_ptr<base::DictionaryValue> dict =
@@ -115,152 +116,194 @@ void ParseAndSetExperimentalOptions(
   if (!dict) {
     DCHECK(false) << "Experimental options string is not a dictionary: "
                   << experimental_options;
-    return;
+    return nullptr;
   }
 
-  const base::DictionaryValue* quic_args = nullptr;
-  if (dict->GetDictionary(kQuicFieldTrialName, &quic_args)) {
-    std::string quic_connection_options;
-    if (quic_args->GetString(kQuicConnectionOptions,
-                             &quic_connection_options)) {
-      context_builder->set_quic_connection_options(
-          net::ParseQuicConnectionOptions(quic_connection_options));
-    }
-
-    // TODO(rtenneti): Delete this option after apps stop using it.
-    // Added this for backward compatibility.
-    bool quic_store_server_configs_in_properties = false;
-    if (quic_args->GetBoolean(kQuicStoreServerConfigsInProperties,
-                              &quic_store_server_configs_in_properties)) {
-      context_builder->set_quic_max_server_configs_stored_in_properties(
-          net::kMaxQuicServersToPersist);
-    }
-
-    int quic_max_server_configs_stored_in_properties = 0;
-    if (quic_args->GetInteger(kQuicMaxServerConfigsStoredInProperties,
-                              &quic_max_server_configs_stored_in_properties)) {
-      context_builder->set_quic_max_server_configs_stored_in_properties(
-          static_cast<size_t>(quic_max_server_configs_stored_in_properties));
-    }
+  bool async_dns_enable = false;
+  bool stale_dns_enable = false;
+  bool host_resolver_rules_enable = false;
+  bool disable_ipv6 = false;
+  StaleHostResolver::StaleOptions stale_dns_options;
+  std::string host_resolver_rules_string;
+  for (base::DictionaryValue::Iterator it(*dict.get()); !it.IsAtEnd();
+       it.Advance()) {
+    if (it.key() == kQuicFieldTrialName) {
+      const base::DictionaryValue* quic_args = nullptr;
+      if (!it.value().GetAsDictionary(&quic_args)) {
+        LOG(ERROR) << "Quic config params \"" << it.value()
+                   << "\" is not a dictionary value";
+        dict->Remove(it.key(), nullptr);
+        continue;
+      }
+      std::string quic_connection_options;
+      if (quic_args->GetString(kQuicConnectionOptions,
+                               &quic_connection_options)) {
+        context_builder->set_quic_connection_options(
+            net::ParseQuicConnectionOptions(quic_connection_options));
+      }
 
-    bool quic_delay_tcp_race = false;
-    if (quic_args->GetBoolean(kQuicDelayTcpRace, &quic_delay_tcp_race)) {
-      context_builder->set_quic_delay_tcp_race(quic_delay_tcp_race);
-    }
+      // TODO(rtenneti): Delete this option after apps stop using it.
+      // Added this for backward compatibility.
+      bool quic_store_server_configs_in_properties = false;
+      if (quic_args->GetBoolean(kQuicStoreServerConfigsInProperties,
+                                &quic_store_server_configs_in_properties)) {
+        context_builder->set_quic_max_server_configs_stored_in_properties(
+            net::kMaxQuicServersToPersist);
+      }
 
-    int quic_idle_connection_timeout_seconds = 0;
-    if (quic_args->GetInteger(kQuicIdleConnectionTimeoutSeconds,
-                              &quic_idle_connection_timeout_seconds)) {
-      context_builder->set_quic_idle_connection_timeout_seconds(
-          quic_idle_connection_timeout_seconds);
-    }
+      int quic_max_server_configs_stored_in_properties = 0;
+      if (quic_args->GetInteger(
+              kQuicMaxServerConfigsStoredInProperties,
+              &quic_max_server_configs_stored_in_properties)) {
+        context_builder->set_quic_max_server_configs_stored_in_properties(
+            static_cast<size_t>(quic_max_server_configs_stored_in_properties));
+      }
 
-    std::string quic_host_whitelist;
-    if (quic_args->GetString(kQuicHostWhitelist, &quic_host_whitelist)) {
-      std::unordered_set<std::string> hosts;
-      for (const std::string& host :
-           base::SplitString(quic_host_whitelist, ",", base::TRIM_WHITESPACE,
-                             base::SPLIT_WANT_ALL)) {
-        hosts.insert(host);
+      bool quic_delay_tcp_race = false;
+      if (quic_args->GetBoolean(kQuicDelayTcpRace, &quic_delay_tcp_race)) {
+        context_builder->set_quic_delay_tcp_race(quic_delay_tcp_race);
       }
-      context_builder->set_quic_host_whitelist(hosts);
-    }
 
-    bool quic_close_sessions_on_ip_change = false;
-    if (quic_args->GetBoolean(kQuicCloseSessionsOnIpChange,
-                              &quic_close_sessions_on_ip_change)) {
-      context_builder->set_quic_close_sessions_on_ip_change(
-          quic_close_sessions_on_ip_change);
-    }
+      int quic_idle_connection_timeout_seconds = 0;
+      if (quic_args->GetInteger(kQuicIdleConnectionTimeoutSeconds,
+                                &quic_idle_connection_timeout_seconds)) {
+        context_builder->set_quic_idle_connection_timeout_seconds(
+            quic_idle_connection_timeout_seconds);
+      }
 
-    bool quic_migrate_sessions_on_network_change = false;
-    if (quic_args->GetBoolean(kQuicMigrateSessionsOnNetworkChange,
-                              &quic_migrate_sessions_on_network_change)) {
-      context_builder->set_quic_migrate_sessions_on_network_change(
-          quic_migrate_sessions_on_network_change);
-    }
+      std::string quic_host_whitelist;
+      if (quic_args->GetString(kQuicHostWhitelist, &quic_host_whitelist)) {
+        std::unordered_set<std::string> hosts;
+        for (const std::string& host :
+             base::SplitString(quic_host_whitelist, ",", base::TRIM_WHITESPACE,
+                               base::SPLIT_WANT_ALL)) {
+          hosts.insert(host);
+        }
+        context_builder->set_quic_host_whitelist(hosts);
+      }
 
-    bool quic_prefer_aes = false;
-    if (quic_args->GetBoolean(kQuicPreferAes, &quic_prefer_aes)) {
-      context_builder->set_quic_prefer_aes(quic_prefer_aes);
-    }
+      bool quic_close_sessions_on_ip_change = false;
+      if (quic_args->GetBoolean(kQuicCloseSessionsOnIpChange,
+                                &quic_close_sessions_on_ip_change)) {
+        context_builder->set_quic_close_sessions_on_ip_change(
+            quic_close_sessions_on_ip_change);
+      }
 
-    std::string quic_user_agent_id;
-    if (quic_args->GetString(kQuicUserAgentId, &quic_user_agent_id)) {
-      context_builder->set_quic_user_agent_id(quic_user_agent_id);
-    }
+      bool quic_migrate_sessions_on_network_change = false;
+      if (quic_args->GetBoolean(kQuicMigrateSessionsOnNetworkChange,
+                                &quic_migrate_sessions_on_network_change)) {
+        context_builder->set_quic_migrate_sessions_on_network_change(
+            quic_migrate_sessions_on_network_change);
+      }
 
-    bool quic_migrate_sessions_early = false;
-    if (quic_args->GetBoolean(kQuicMigrateSessionsEarly,
-                              &quic_migrate_sessions_early)) {
-      context_builder->set_quic_migrate_sessions_early(
-          quic_migrate_sessions_early);
-    }
+      bool quic_prefer_aes = false;
+      if (quic_args->GetBoolean(kQuicPreferAes, &quic_prefer_aes)) {
+        context_builder->set_quic_prefer_aes(quic_prefer_aes);
+      }
 
-    bool quic_disable_bidirectional_streams = false;
-    if (quic_args->GetBoolean(kQuicDisableBidirectionalStreams,
-                              &quic_disable_bidirectional_streams)) {
-      context_builder->set_quic_disable_bidirectional_streams(
-          quic_disable_bidirectional_streams);
-    }
+      std::string quic_user_agent_id;
+      if (quic_args->GetString(kQuicUserAgentId, &quic_user_agent_id)) {
+        context_builder->set_quic_user_agent_id(quic_user_agent_id);
+      }
 
-    bool quic_race_cert_verification = false;
-    if (quic_args->GetBoolean(kQuicRaceCertVerification,
-                              &quic_race_cert_verification)) {
-      context_builder->set_quic_race_cert_verification(
-          quic_race_cert_verification);
-    }
-  }
+      bool quic_migrate_sessions_early = false;
+      if (quic_args->GetBoolean(kQuicMigrateSessionsEarly,
+                                &quic_migrate_sessions_early)) {
+        context_builder->set_quic_migrate_sessions_early(
+            quic_migrate_sessions_early);
+      }
 
-  bool async_dns_enable = false;
-  bool stale_dns_enable = false;
-  bool host_resolver_rules_enable = false;
-  bool disable_ipv6 = false;
-  StaleHostResolver::StaleOptions stale_dns_options;
-  std::string host_resolver_rules_string;
+      bool quic_disable_bidirectional_streams = false;
+      if (quic_args->GetBoolean(kQuicDisableBidirectionalStreams,
+                                &quic_disable_bidirectional_streams)) {
+        context_builder->set_quic_disable_bidirectional_streams(
+            quic_disable_bidirectional_streams);
+      }
 
-  const base::DictionaryValue* async_dns_args = nullptr;
-  if (dict->GetDictionary(kAsyncDnsFieldTrialName, &async_dns_args))
-    async_dns_args->GetBoolean(kAsyncDnsEnable, &async_dns_enable);
-
-  const base::DictionaryValue* stale_dns_args = nullptr;
-  if (dict->GetDictionary(kStaleDnsFieldTrialName, &stale_dns_args)) {
-    if (stale_dns_args->GetBoolean(kStaleDnsEnable, &stale_dns_enable) &&
-        stale_dns_enable) {
-      int delay;
-      if (stale_dns_args->GetInteger(kStaleDnsDelayMs, &delay))
-        stale_dns_options.delay = base::TimeDelta::FromMilliseconds(delay);
-      int max_expired_time_ms;
-      if (stale_dns_args->GetInteger(kStaleDnsMaxExpiredTimeMs,
-                                     &max_expired_time_ms)) {
-        stale_dns_options.max_expired_time =
-            base::TimeDelta::FromMilliseconds(max_expired_time_ms);
+      bool quic_race_cert_verification = false;
+      if (quic_args->GetBoolean(kQuicRaceCertVerification,
+                                &quic_race_cert_verification)) {
+        context_builder->set_quic_race_cert_verification(
+            quic_race_cert_verification);
+      }
+    } else if (it.key() == kAsyncDnsFieldTrialName) {
+      const base::DictionaryValue* async_dns_args = nullptr;
+      if (!it.value().GetAsDictionary(&async_dns_args)) {
+        LOG(ERROR) << "\"" << it.key() << "\" config params \"" << it.value()
+                   << "\" is not a dictionary value";
+        dict->Remove(it.key(), nullptr);
+        continue;
       }
-      int max_stale_uses;
-      if (stale_dns_args->GetInteger(kStaleDnsMaxStaleUses, &max_stale_uses))
-        stale_dns_options.max_stale_uses = max_stale_uses;
-      bool allow_other_network;
-      if (stale_dns_args->GetBoolean(kStaleDnsAllowOtherNetwork,
-                                     &allow_other_network)) {
-        stale_dns_options.allow_other_network = allow_other_network;
+      async_dns_args->GetBoolean(kAsyncDnsEnable, &async_dns_enable);
+    } else if (it.key() == kStaleDnsFieldTrialName) {
+      const base::DictionaryValue* stale_dns_args = nullptr;
+      if (!it.value().GetAsDictionary(&stale_dns_args)) {
+        LOG(ERROR) << "\"" << it.key() << "\" config params \"" << it.value()
+                   << "\" is not a dictionary value";
+        dict->Remove(it.key(), nullptr);
+        continue;
       }
+      if (stale_dns_args->GetBoolean(kStaleDnsEnable, &stale_dns_enable) &&
+          stale_dns_enable) {
+        int delay;
+        if (stale_dns_args->GetInteger(kStaleDnsDelayMs, &delay))
+          stale_dns_options.delay = base::TimeDelta::FromMilliseconds(delay);
+        int max_expired_time_ms;
+        if (stale_dns_args->GetInteger(kStaleDnsMaxExpiredTimeMs,
+                                       &max_expired_time_ms)) {
+          stale_dns_options.max_expired_time =
+              base::TimeDelta::FromMilliseconds(max_expired_time_ms);
+        }
+        int max_stale_uses;
+        if (stale_dns_args->GetInteger(kStaleDnsMaxStaleUses, &max_stale_uses))
+          stale_dns_options.max_stale_uses = max_stale_uses;
+        bool allow_other_network;
+        if (stale_dns_args->GetBoolean(kStaleDnsAllowOtherNetwork,
+                                       &allow_other_network)) {
+          stale_dns_options.allow_other_network = allow_other_network;
+        }
+      }
+    } else if (it.key() == kHostResolverRulesFieldTrialName) {
+      const base::DictionaryValue* host_resolver_rules_args = nullptr;
+      if (!it.value().GetAsDictionary(&host_resolver_rules_args)) {
+        LOG(ERROR) << "\"" << it.key() << "\" config params \"" << it.value()
+                   << "\" is not a dictionary value";
+        dict->Remove(it.key(), nullptr);
+        continue;
+      }
+      host_resolver_rules_enable = host_resolver_rules_args->GetString(
+          kHostResolverRules, &host_resolver_rules_string);
+    } else if (it.key() == kDisableIPv6) {
+      if (!it.value().GetAsBoolean(&disable_ipv6)) {
+        LOG(ERROR) << "\"" << it.key() << "\" config params \"" << it.value()
+                   << "\" is not a bool";
+        dict->Remove(it.key(), nullptr);
+        continue;
+      }
+    } else if (it.key() == kSSLKeyLogFile) {
+      std::string ssl_key_log_file_string;
+      if (it.value().GetAsString(&ssl_key_log_file_string)) {
+        DCHECK(file_task_runner);
+        base::FilePath ssl_key_log_file(ssl_key_log_file_string);
+        if (!ssl_key_log_file.empty() && file_task_runner) {
+          // SetSSLKeyLogFile is only safe to call before any SSLClientSockets
+          // are created. This should not be used if there are multiple
+          // CronetEngine.
+          // TODO(xunjieli): Expose this as a stable API after crbug.com/458365
+          // is resolved.
+          net::SSLClientSocket::SetSSLKeyLogFile(ssl_key_log_file,
+                                                 file_task_runner);
+        }
+      }
+    } else {
+      LOG(ERROR) << "Unrecognized Cronet experimental option \"" << it.key()

[kapishnikov, 2017/03/14 14:37:09]

Should we change the error to warning? It could be possible that some
experimental options will be removed in the future, so the new implementation
doesn't have them anymore.

[xunjieli, 2017/03/14 19:03:06]

This doesn't introduce crash. I think this one is an error (comparable to
Log.e() in Android) that clients should take care of. 

[kapishnikov, 2017/03/14 22:39:07]

There can be a scenario when the implementation is updated independently from
the client code. The implementation may not be necessary packaged inside the app
bundle but downloaded from somewhere else and updated independently after the
release of the app. If the implementation is updated to a version that does not
have the experimental option anymore, the app will suddenly start logging the
error but there is nothing that the developer can do. That is why I think that
the logging with the warning level might be better than the error one.

[xunjieli, 2017/03/14 23:10:46]

Done.

+                 << "\" with params \"" << it.value();
+      dict->Remove(it.key(), nullptr);
     }
   }
-
-  const base::DictionaryValue* host_resolver_rules_args = nullptr;
-  if (dict->GetDictionary(kHostResolverRulesFieldTrialName,
-                          &host_resolver_rules_args)) {
-    host_resolver_rules_enable = host_resolver_rules_args->GetString(
-        kHostResolverRules, &host_resolver_rules_string);
-  }
-
-  dict->GetBoolean(kDisableIPv6, &disable_ipv6);
-
   if (async_dns_enable || stale_dns_enable || host_resolver_rules_enable ||
       disable_ipv6) {
-    if (net_log == nullptr) {
-      CHECK(false) << "All DNS-related experiments require NetLog.";
-    }
+    CHECK(net_log) << "All DNS-related experiments require NetLog.";
     std::unique_ptr<net::HostResolver> host_resolver;
     if (stale_dns_enable) {
       DCHECK(!disable_ipv6);
@@ -282,20 +325,7 @@ void ParseAndSetExperimentalOptions(
     }
     context_builder->set_host_resolver(std::move(host_resolver));
   }
-
-  std::string ssl_key_log_file_string;
-  if (dict->GetString(kSSLKeyLogFile, &ssl_key_log_file_string)) {
-    DCHECK(file_task_runner);
-    base::FilePath ssl_key_log_file(ssl_key_log_file_string);
-    if (!ssl_key_log_file.empty() && file_task_runner) {
-      // SetSSLKeyLogFile is only safe to call before any SSLClientSockets are
-      // created. This should not be used if there are multiple CronetEngine.
-      // TODO(xunjieli): Expose this as a stable API after crbug.com/458365 is
-      // resolved.
-      net::SSLClientSocket::SetSSLKeyLogFile(ssl_key_log_file,
-                                             file_task_runner);
-    }
-  }
+  return dict;
 }
 
 }  // namespace
@@ -387,8 +417,8 @@ void URLRequestContextConfig::ConfigureURLRequestContextBuilder(
   if (enable_quic)
     context_builder->set_quic_user_agent_id(quic_user_agent_id);
 
-  ParseAndSetExperimentalOptions(experimental_options, context_builder, net_log,
-                                 file_task_runner);
+  effective_experimental_options = ParseAndSetExperimentalOptions(
+      experimental_options, context_builder, net_log, file_task_runner);
 
   std::unique_ptr<net::CertVerifier> cert_verifier;
   if (mock_cert_verifier) {