Respect QuicAllowed policy for new streams

net/http/http_network_session.h

 // 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.
 
 #ifndef NET_HTTP_HTTP_NETWORK_SESSION_H_
 #define NET_HTTP_HTTP_NETWORK_SESSION_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 52 matching lines @@
 class SSLClientSocketPool;
 class SSLConfigService;
 class TransportClientSocketPool;
 class TransportSecurityState;
 
 // This class holds session objects used by HttpNetworkTransaction objects.
 class NET_EXPORT HttpNetworkSession
     : NON_EXPORTED_BASE(public base::NonThreadSafe),
       public base::MemoryCoordinatorClient {
  public:
+  // Shared Dynamic Parameters - holds parameters which can change at run time.
+  // One instance of this struct is intended to be shared accross multiple
+  // HttpNetworkSessions (the default copy constructor of Params just copies the
+  // pointer to SharedParams).
+  struct NET_EXPORT DynamicSharedParams {
+    DynamicSharedParams();
+    DynamicSharedParams(const DynamicSharedParams& other);
+    ~DynamicSharedParams();
+
+    // Enables QUIC for new streams (if QUIC is enabled).
+    // If DynamicSharedParams is missing, assume false.
+    bool enable_quic;

[Ryan Hamilton, 2016/12/20 15:38:12]

nit: quic_enabled might be a better choice since it sounds more like a state
instead of like an action.

[pmarko, 2016/12/20 18:00:37]

I also thought so at first, but then I wanted to make this consistent with
Params::enable_* - seeing as this used to be Params::enable_quic.

What would you prefer -- consistency with Params:: members or the higher clarity
of action/state semantics?

[Ryan Hamilton, 2016/12/21 21:23:58]

Ah, fair enough. Let's remain consistent.

[pmarko, 2016/12/23 19:19:46]

(obsolete - Params has been reverted)

+  };
+
   struct NET_EXPORT Params {
     Params();
     Params(const Params& other);
     ~Params();
 
     ClientSocketFactory* client_socket_factory;
     HostResolver* host_resolver;
     CertVerifier* cert_verifier;
     ChannelIDService* channel_id_service;
     TransportSecurityState* transport_security_state;
     CTVerifier* cert_transparency_verifier;
     CTPolicyEnforcer* ct_policy_enforcer;
     ProxyService* proxy_service;
     SSLConfigService* ssl_config_service;
     HttpAuthHandlerFactory* http_auth_handler_factory;
     HttpServerProperties* http_server_properties;
     NetLog* net_log;
     HostMappingRules* host_mapping_rules;
     SocketPerformanceWatcherFactory* socket_performance_watcher_factory;
     bool ignore_certificate_errors;
     uint16_t testing_fixed_http_port;
     uint16_t testing_fixed_https_port;
     bool enable_tcp_fast_open_for_ssl;
 
+    // Holds parameters which can change after initialization
+    // and can be shared accross multiple HttpNetworkSessions.
+    // By default, the pointer is inherited on copy.
+    // May be nullptr

[Bence, 2016/12/20 14:58:07]

Please append period to last sentence.

[Ryan Hamilton, 2016/12/20 15:38:12]

I'm ramping up on this CL, so sorry for the basic question. Under what
circumstances should this be null?

[pmarko, 2016/12/20 18:00:37]

Bence: Done.

Ryan: Good point! Basically, by introducing this, I've added a burden of
creating and assigning the dynamic_shared_params when using HttpNetworkSession
with QUIC. As I didn't want to touch all existing unit tests which create a
HttpNetworkSession, I've thought of this:
- When dynamic_shared_params==nullptr, use what used to be default if the user
code didn't change enable_quic.
- This means that existing unit tests which don't use QUIC need no changes (this
remains nullptr, and the resulting behavior is enable_quic=false, which is the
same as they had until now).

I've added a comment clarifying this and referencing to comments on
DynamicSharedParams:: members, where the defaults are commented.

[Ryan Hamilton, 2016/12/21 21:23:58]

Hm. I think you're saying that the dynamic params should never be null in
production. If so, I'd strongly prefer a DCHECK(params_.dynamic_shared_params !=
null); in HttpNetworkSession to make sure callers do the right thing. Alas, as
you point out, this means that a bunch of tests will need to change. That's
annoying, but less annoying than accidentally not enabling QUIC.

[pmarko, 2016/12/23 19:19:46]

See my reply in comments thread - you're right that this is not really
consitent, and making it consistent and thus requiring everyone to set this is
too big a change for what I'm trying to achieve in my opinion.

Thus: Changed the logic: No more DynamicSharedParams, instead HttpNetworkSession
now has SetQuicEnabled which is called from outside.

+    DynamicSharedParams* dynamic_shared_params;

[Ryan Hamilton, 2016/12/20 15:38:12]

I'm surprised not to see any cronet code which sets this. Can you confirm that
cronet's QUIC-enabling code still works after this refactor?

[pmarko, 2016/12/20 18:00:37]

Good point! Didn't think of cronet, but it should be fine - I've just checked
that cronet_environment.mm uses net::URLRequestContextBuilder which sets this
(see cronet_environment.mm:295 , url_request_context_config.cc:373, and
url_request_context_builder.cc in this patch).

[Ryan Hamilton, 2016/12/21 21:23:58]

Ah, good point.

[pmarko, 2016/12/23 19:19:46]

(obsolete - Params has been reverted)

+
     // Use SPDY ping frames to test for connection health after idle.
     bool enable_spdy_ping_based_connection_checking;
     bool enable_http2;
     size_t spdy_session_max_recv_window_size;
     size_t spdy_stream_max_recv_window_size;
     // Source of time for SPDY connections.
     SpdySessionPool::TimeFunc time_func;
     // Whether to enable HTTP/2 Alt-Svc entries with hostname different than
     // that of the origin.
     bool enable_http2_alternative_service_with_different_host;
     // Whether to enable QUIC Alt-Svc entries with hostname different than that
     // of the origin.
     bool enable_quic_alternative_service_with_different_host;
 
-    // Enables QUIC support.
-    bool enable_quic;
     // Disable QUIC if a connection times out with open streams.
     bool disable_quic_on_timeout_with_open_streams;
     // Disables QUIC's 0-RTT behavior.
     bool quic_always_require_handshake_confirmation;
     // Disables QUIC connection pooling.
     bool quic_disable_connection_pooling;
     // If not zero, the task to load QUIC server configs from the disk cache
     // will timeout after this value multiplied by the smoothed RTT for the
     // server.
     float quic_load_server_info_timeout_srtt_multiplier;
@@ skipping 64 matching lines @@
     // If true, configure QUIC sockets to not fragment packets.
     bool quic_do_not_fragment;
 
     ProxyDelegate* proxy_delegate;
     // Enable support for Token Binding.
     bool enable_token_binding;
 
     // Enable HTTP/0.9 for HTTP/HTTPS on ports other than the default one for
     // each protocol.
     bool http_09_on_non_default_ports_enabled;
+
+    // Evaluates if QUIC is enabled for new streams

[Bence, 2016/12/20 14:58:07]

Please append period to this sentence.

[pmarko, 2016/12/20 18:00:37]

Done.

+    bool enable_quic() const;

[Bence, 2016/12/20 14:58:07]

Please move this up to right after the destructor: methods (even accessors)
usually come before members, see
https://google.github.io/styleguide/cppguide.html#Declaration_Order.

[Ryan Hamilton, 2016/12/20 15:38:12]

The style guide prohibits methods on structs. Perhaps you could add an
IsQuicEnabled() method to the session itself?

[pmarko, 2016/12/20 18:00:37]

Bence: Probably obselete now :)

Ryan: Ah, I didn't know that, but it makes sense. Will do.

   };
 
   enum SocketPoolType {
     NORMAL_SOCKET_POOL,
     WEBSOCKET_SOCKET_POOL,
     NUM_SOCKET_POOL_TYPES
   };
 
   explicit HttpNetworkSession(const Params& params);
   ~HttpNetworkSession() override;
@@ skipping 115 matching lines @@
   NextProtoVector next_protos_;
 
   Params params_;
 
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_HTTP_NETWORK_SESSION_H_