Consistently suggest ephemeral port for QUIC client

net/quic/quic_stream_factory.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 "net/quic/quic_stream_factory.h"
 
 #include <set>
 
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_verifier.h"
 #include "net/dns/host_resolver.h"
 #include "net/dns/single_request_host_resolver.h"
 #include "net/http/http_server_properties.h"
 #include "net/quic/congestion_control/tcp_receiver.h"
 #include "net/quic/crypto/proof_verifier_chromium.h"
 #include "net/quic/crypto/quic_random.h"
+#include "net/quic/port_suggester.h"
 #include "net/quic/quic_client_session.h"
 #include "net/quic/quic_clock.h"
 #include "net/quic/quic_connection.h"
 #include "net/quic/quic_connection_helper.h"
 #include "net/quic/quic_crypto_client_stream_factory.h"
 #include "net/quic/quic_default_packet_writer.h"
 #include "net/quic/quic_http_stream.h"
 #include "net/quic/quic_protocol.h"
 #include "net/socket/client_socket_factory.h"
 
@@ skipping 215 matching lines @@
     QuicClock* clock,
     size_t max_packet_length)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
       client_socket_factory_(client_socket_factory),
       http_server_properties_(http_server_properties),
       quic_crypto_client_stream_factory_(quic_crypto_client_stream_factory),
       random_generator_(random_generator),
       clock_(clock),
       max_packet_length_(max_packet_length),
-      weak_factory_(this) {
+      weak_factory_(this),
+      port_entropy_(random_generator_->RandUint64()) {
   config_.SetDefaults();
   config_.set_idle_connection_state_lifetime(
       QuicTime::Delta::FromSeconds(30),
       QuicTime::Delta::FromSeconds(30));
   cannoncial_suffixes_.push_back(string(".c.youtube.com"));
   cannoncial_suffixes_.push_back(string(".googlevideo.com"));
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
   CloseAllSessions(ERR_ABORTED);
@@ skipping 170 matching lines @@
 }
 
 QuicClientSession* QuicStreamFactory::CreateSession(
     const HostPortProxyPair& host_port_proxy_pair,
     bool is_https,
     CertVerifier* cert_verifier,
     const AddressList& address_list,
     const BoundNetLog& net_log) {
   QuicGuid guid = random_generator_->RandUint64();
   IPEndPoint addr = *address_list.begin();
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(host_port_proxy_pair.first, port_entropy_);
   scoped_ptr<DatagramClientSocket> socket(
       client_socket_factory_->CreateDatagramClientSocket(
-          DatagramSocket::DEFAULT_BIND, base::Bind(&base::RandInt),
+          DatagramSocket::RANDOM_BIND,
+          base::Bind(&PortSuggester::SuggestPort, port_suggester),

[Ryan Hamilton, 2013/12/07 01:46:14]

It would be super great if we had a test that verified we actually use the same
port when connecting to the same server.  Alas, that doesn't look trivial. :/ 
It's tempting to add a unit test of QuicStreamFactory, but that use the
MockSocket infrastructure, which doesn't call the rand_cb.  You could probably
tweak a test to use a real socket factory, but that'd be a a bunch of awkward. 
Probably the easiest thing would be to tweak the QuicServer to expose the
peer_address in some fashion, but even that's a bunch of yak-shaving, so perhaps
it's not worth it.

[jar (doing other things), 2013/12/07 03:12:18]

I added some code to DCHECK() that the port_suggester was at least being called
(recall that there was a bug previously where that didn't happen), and also to
verify that the last suggestion was actually used (I'm counting on not being
blocked from using the suggestions).

The only missing link would be if we're accidentally calling to seed_
differently when we are going to the same server (since the unit tests verify
that if you seed identically... you get identical suggestions).

[Ryan Hamilton, 2013/12/07 04:52:22]

If you reverted the changes to this file, would any tests break?

           net_log.net_log(), net_log.source()));
   socket->Connect(addr);
 
   // We should adaptively set this buffer size, but for now, we'll use a size
   // that is more than large enough for a full receive window, and yet
   // does not consume "too much" memory.  If we see bursty packet loss, we may
   // revisit this setting and test for its impact.
   const int32 kSocketBufferSize(TcpReceiver::kReceiveWindowTCP);
   socket->SetReceiveBufferSize(kSocketBufferSize);
   // Set a buffer large enough to contain the initial CWND's worth of packet
@@ skipping 97 matching lines @@
   // Copy the CachedState for the canonical server from canonical_crypto_config
   // as the initial CachedState for the server_hostname in crypto_config.
   crypto_config->InitializeFrom(server_hostname,
                                 canonical_host_port_proxy_pair.first.host(),
                                 canonical_crypto_config);
   // Update canonical version to point at the "most recent" crypto_config.
   canonical_hostname_to_origin_map_[canonical_host_port] = host_port_proxy_pair;
 }
 
 }  // namespace net