Consistently suggest ephemeral port for QUIC client

net/quic/quic_stream_factory.cc

Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index 40f294821850e0a6fbffa5c9dc5a1d0fbb58d13c..5d64aeaf8d95aac7225721050205e45798536c75 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ -21,6 +22,7 @@
 #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"
@@ -256,7 +258,8 @@ QuicStreamFactory::QuicStreamFactory(
       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),
@@ -447,12 +450,33 @@ QuicClientSession* QuicStreamFactory::CreateSession(
     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),
           net_log.net_log(), net_log.source()));
   socket->Connect(addr);
 
+  DCHECK(port_suggester->call_count() >= 1);
+  IPEndPoint address;  // For debugging only.
+  // This DCHECK will try to validate that our port_suggester was used, and not
+  // just consulted.  Non-Mock Chromium tries using the port_suggester up to 10
+  // times before giving up, and accepting an OS selected port, in which case we
+  // *can't* validate the source port.
+  // Since the Mock tests don't really use the suggestions, we'll allow Mocks to
+  // signal this non-use by requesting 3 or more suggestions, and then we won't
+  // validate the source port.
+  // Bottom line: More than 2 suggestions, and we won't validate the port.
+  // Common use cases result in only one suggestion being requested, and it is
+  // immediately used as the source port, so this DCHECK is very meaningful.
+  DCHECK(ERR_SOCKET_NOT_CONNECTED == socket->GetLocalAddress(&address) ||
+         port_suggester->call_count() > 2 ||

[jar (doing other things), 2013/12/08 21:12:58]

The polarity of the first clause (line 474) was wrong, which mostly neutered the
DCHECK.

When I corrected the polarity, then some tests in quic_factory_stream_test.cc do
indeed fail... <yeah!>

...and the proposed technique of calling the PRNG several times in the Mock
socket, allows this DCHECK to be fine.  

[Ryan Hamilton, 2013/12/08 22:54:15]

Yay for tests :>

+         address.port() == port_suggester->previous_suggestion());
+  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
+                       port_suggester->call_count());
+
   // 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