Skip to content

Commit

Permalink
Notify NQE of TCP RTT values
Browse files Browse the repository at this point in the history
This change passes SocketPerformanceWatcherFactory (SPWF)
from HttpNetworkSession params
-> ClientSocketPoolManagerImpl
-> TransportClientSocketPool
-> TransportConnectJobFactory
-> TransportConnectJob::TransportConnectJob()

TransportConnectJob creates SocketPerformanceWatcher (SPW)
from SPWF, and passes it to the
CreateTransportClientSocket. From there it is passed to
TCPClientSocket, which passes it to TCPSocketPosix.

A new SPW is created for every TCPSocket. If a TCPSocket
object is reused for a different connection (by first
disconnecting and then reconnecting), then SPW is reset.

Currently, RTT is only reported for POSIX TCP Sockets
(net/socket/tcp_socket_posix.h).

BUG=498380, 502419, 502423

TBR=pfeldman@chromium.org, frankf@chromium.org
(for chrome/browser/devtools/device/* and
chrome/test/chromedriver/net/* since the changes are
mechanical).

Review URL: https://codereview.chromium.org/1376473003

Cr-Commit-Position: refs/heads/master@{#387128}
  • Loading branch information
tarunban authored and Commit bot committed Apr 13, 2016
1 parent f6d59b7 commit 7b403bc
Show file tree
Hide file tree
Showing 59 changed files with 662 additions and 248 deletions.
2 changes: 1 addition & 1 deletion blimp/net/tcp_client_transport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void TCPClientTransport::Connect(const net::CompletionCallback& callback) {

connect_callback_ = callback;
socket_ = socket_factory_->CreateTransportClientSocket(
net::AddressList(ip_endpoint_), net_log_, net::NetLog::Source());
net::AddressList(ip_endpoint_), nullptr, net_log_, net::NetLog::Source());
net::CompletionCallback completion_callback = base::Bind(
&TCPClientTransport::OnTCPConnectComplete, base::Unretained(this));

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/devtools/device/adb/adb_client_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ void AdbClientSocket::Connect(const net::CompletionCallback& callback) {

net::AddressList address_list =
net::AddressList::CreateFromIPAddress(ip_address, port_);
socket_.reset(new net::TCPClientSocket(address_list, NULL,
socket_.reset(new net::TCPClientSocket(address_list, NULL, NULL,
net::NetLog::Source()));
int result = socket_->Connect(callback);
if (result != net::ERR_IO_PENDING)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class SocketTunnel : public base::NonThreadSafe {
return;
}

host_socket_.reset(new net::TCPClientSocket(address_list_, nullptr,
host_socket_.reset(new net::TCPClientSocket(address_list_, nullptr, nullptr,
net::NetLog::Source()));
result = host_socket_->Connect(base::Bind(&SocketTunnel::OnConnected,
base::Unretained(this)));
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/devtools/device/tcp_device_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ class ResolveHostAndOpenSocket final {
delete this;
return;
}
std::unique_ptr<net::StreamSocket> socket(
new net::TCPClientSocket(address_list_, NULL, net::NetLog::Source()));
std::unique_ptr<net::StreamSocket> socket(new net::TCPClientSocket(
address_list_, NULL, NULL, net::NetLog::Source()));
socket->Connect(
base::Bind(&RunSocketCallback, callback_, base::Passed(&socket)));
delete this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ class MockTCPClientSocket : public net::TCPClientSocket {
};

MockTCPClientSocket::MockTCPClientSocket()
: TCPClientSocket(net::AddressList(), nullptr, net::NetLog::Source()) {}
: TCPClientSocket(net::AddressList(),
nullptr,
nullptr,
net::NetLog::Source()) {}
MockTCPClientSocket::~MockTCPClientSocket() {}

} // namespace extensions
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/extensions/api/socket/tcp_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ namespace extensions {
class MockTCPSocket : public net::TCPClientSocket {
public:
explicit MockTCPSocket(const net::AddressList& address_list)
: net::TCPClientSocket(address_list, NULL, net::NetLog::Source()) {
}
: net::TCPClientSocket(address_list, NULL, NULL, net::NetLog::Source()) {}

MOCK_METHOD3(Read, int(net::IOBuffer* buf, int buf_len,
const net::CompletionCallback& callback));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class MockSSLClientSocket : public net::SSLClientSocket {
class MockTCPSocket : public net::TCPClientSocket {
public:
explicit MockTCPSocket(const net::AddressList& address_list)
: net::TCPClientSocket(address_list, NULL, net::NetLog::Source()) {}
: net::TCPClientSocket(address_list, NULL, NULL, net::NetLog::Source()) {}

MOCK_METHOD3(Read,
int(net::IOBuffer* buf,
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/chromedriver/net/adb_client_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ AdbClientSocket::~AdbClientSocket() {
void AdbClientSocket::Connect(const net::CompletionCallback& callback) {
net::AddressList address_list = net::AddressList::CreateFromIPAddress(
net::IPAddress::IPv4Localhost(), port_);
socket_.reset(new net::TCPClientSocket(address_list, NULL,
socket_.reset(new net::TCPClientSocket(address_list, NULL, NULL,
net::NetLog::Source()));
int result = socket_->Connect(callback);
if (result != net::ERR_IO_PENDING)
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/chromedriver/net/websocket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ void WebSocket::Connect(const net::CompletionCallback& callback) {
}

net::NetLog::Source source;
socket_.reset(new net::TCPClientSocket(addresses, NULL, source));
socket_.reset(new net::TCPClientSocket(addresses, NULL, NULL, source));

state_ = CONNECTING;
connect_callback_ = callback;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ void PepperTCPServerSocketMessageFilter::DoListen(

state_ = STATE_LISTEN_IN_PROGRESS;

socket_.reset(new net::TCPSocket(NULL, net::NetLog::Source()));
socket_.reset(new net::TCPSocket(NULL, NULL, net::NetLog::Source()));
int net_result = net::OK;
do {
net::IPEndPoint ip_end_point(net::IPAddress(address), port);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ PepperTCPSocketMessageFilter::PepperTCPSocketMessageFilter(
rcvbuf_size_(0),
sndbuf_size_(0),
address_index_(0),
socket_(new net::TCPSocket(NULL, net::NetLog::Source())),
socket_(new net::TCPSocket(NULL, NULL, net::NetLog::Source())),
ssl_context_helper_(host->ssl_context_helper()),
pending_accept_(false),
pending_read_on_unthrottle_(false),
Expand Down Expand Up @@ -860,7 +860,7 @@ void PepperTCPSocketMessageFilter::OnConnectCompleted(
// We have to recreate |socket_| because it doesn't allow a second connect
// attempt. We won't lose any state such as bound address or set options,
// because in the private or v1.0 API, connect must be the first operation.
socket_.reset(new net::TCPSocket(NULL, net::NetLog::Source()));
socket_.reset(new net::TCPSocket(NULL, NULL, net::NetLog::Source()));

if (address_index_ + 1 < address_list_.size()) {
DCHECK_EQ(version_, ppapi::TCP_SOCKET_VERSION_PRIVATE);
Expand Down
2 changes: 1 addition & 1 deletion device/bluetooth/bluetooth_socket_net.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ void BluetoothSocketNet::ResetData() {
}

void BluetoothSocketNet::ResetTCPSocket() {
tcp_socket_.reset(new net::TCPSocket(NULL, net::NetLog::Source()));
tcp_socket_.reset(new net::TCPSocket(NULL, NULL, net::NetLog::Source()));
}

void BluetoothSocketNet::SetTCPSocket(
Expand Down
4 changes: 2 additions & 2 deletions device/bluetooth/bluetooth_socket_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ void BluetoothSocketWin::DoConnect(
}

std::unique_ptr<net::TCPSocket> scoped_socket(
new net::TCPSocket(NULL, net::NetLog::Source()));
new net::TCPSocket(NULL, NULL, net::NetLog::Source()));
net::EnsureWinsockInit();
SOCKET socket_fd = socket(AF_BTH, SOCK_STREAM, BTHPROTO_RFCOMM);
SOCKADDR_BTH sa;
Expand Down Expand Up @@ -261,7 +261,7 @@ void BluetoothSocketWin::DoListen(
// TCPSocket methods that involve address could not be called. So bind()
// is called on |socket_fd| directly.
std::unique_ptr<net::TCPSocket> scoped_socket(
new net::TCPSocket(NULL, net::NetLog::Source()));
new net::TCPSocket(NULL, NULL, net::NetLog::Source()));
scoped_socket->AdoptListenSocket(socket_fd);

SOCKADDR_BTH sa;
Expand Down
2 changes: 1 addition & 1 deletion extensions/browser/api/cast_channel/cast_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ bool CastSocketImpl::audio_only() const {
scoped_ptr<net::TCPClientSocket> CastSocketImpl::CreateTcpSocket() {
net::AddressList addresses(ip_endpoint_);
return scoped_ptr<net::TCPClientSocket>(
new net::TCPClientSocket(addresses, net_log_, net_log_source_));
new net::TCPClientSocket(addresses, nullptr, net_log_, net_log_source_));
// Options cannot be set on the TCPClientSocket yet, because the
// underlying platform socket will not be created until Bind()
// or Connect() is called.
Expand Down
10 changes: 8 additions & 2 deletions extensions/browser/api/cast_channel/cast_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,18 @@ CastMessage CreateTestMessage() {
class MockTCPSocket : public net::TCPClientSocket {
public:
explicit MockTCPSocket(const net::MockConnect& connect_data)
: TCPClientSocket(net::AddressList(), nullptr, net::NetLog::Source()),
: TCPClientSocket(net::AddressList(),
nullptr,
nullptr,
net::NetLog::Source()),
connect_data_(connect_data),
do_nothing_(false) {}

explicit MockTCPSocket(bool do_nothing)
: TCPClientSocket(net::AddressList(), nullptr, net::NetLog::Source()) {
: TCPClientSocket(net::AddressList(),
nullptr,
nullptr,
net::NetLog::Source()) {
CHECK(do_nothing);
do_nothing_ = do_nothing;
}
Expand Down
2 changes: 1 addition & 1 deletion extensions/browser/api/socket/tcp_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void TCPSocket::Connect(const net::AddressList& address,
int result = net::ERR_CONNECTION_FAILED;
if (!is_connected_) {
socket_.reset(
new net::TCPClientSocket(address, NULL, net::NetLog::Source()));
new net::TCPClientSocket(address, NULL, NULL, net::NetLog::Source()));
result = socket_->Connect(
base::Bind(&TCPSocket::OnConnectComplete, base::Unretained(this)));
}
Expand Down
2 changes: 1 addition & 1 deletion google_apis/gcm/base/socket_stream_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ void GCMSocketStreamTest::WaitForData(int msg_size) {

void GCMSocketStreamTest::OpenConnection() {
socket_ = socket_factory_.CreateTransportClientSocket(
address_list_, NULL, net::NetLog::Source());
address_list_, NULL, NULL, net::NetLog::Source());
socket_->Connect(
base::Bind(&GCMSocketStreamTest::ConnectCallback,
base::Unretained(this)));
Expand Down
2 changes: 1 addition & 1 deletion google_apis/gcm/engine/connection_handler_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ net::StreamSocket* GCMConnectionHandlerImplTest::BuildSocket(
socket_factory_.AddSocketDataProvider(data_provider_.get());

socket_ = socket_factory_.CreateTransportClientSocket(
address_list_, NULL, net::NetLog::Source());
address_list_, NULL, NULL, net::NetLog::Source());
socket_->Connect(net::CompletionCallback());

run_loop_.reset(new base::RunLoop());
Expand Down
2 changes: 1 addition & 1 deletion jingle/glue/chrome_async_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ class MockXmppClientSocketFactory : public ResolvingClientSocketFactory {
scoped_ptr<net::StreamSocket> CreateTransportClientSocket(
const net::HostPortPair& host_and_port) override {
return mock_client_socket_factory_->CreateTransportClientSocket(
address_list_, NULL, net::NetLog::Source());
address_list_, NULL, NULL, net::NetLog::Source());
}

scoped_ptr<net::SSLClientSocket> CreateSSLClientSocket(
Expand Down
2 changes: 1 addition & 1 deletion jingle/glue/fake_ssl_client_socket_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class FakeSSLClientSocketTest : public testing::Test {

scoped_ptr<net::StreamSocket> MakeClientSocket() {
return mock_client_socket_factory_.CreateTransportClientSocket(
net::AddressList(), NULL, net::NetLog::Source());
net::AddressList(), NULL, NULL, net::NetLog::Source());
}

void SetData(const net::MockConnect& mock_connect,
Expand Down
56 changes: 56 additions & 0 deletions net/base/network_quality_estimator_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "net/base/network_quality_estimator.h"

#include <stddef.h>
#include <stdint.h>

#include <limits>
Expand Down Expand Up @@ -1122,4 +1123,59 @@ TEST(NetworkQualityEstimatorTest, TestObservers) {
rtt_observer.observations().at(3).rtt_ms);
}

// TestTCPSocketRTT requires kernel support for tcp_info struct, and so it is
// enabled only on certain platforms.
#if defined(TCP_INFO) || defined(OS_LINUX)
#define MAYBE_TestTCPSocketRTT TestTCPSocketRTT
#else
#define MAYBE_TestTCPSocketRTT DISABLED_TestTCPSocketRTT
#endif
// Tests that the TCP socket notifies the Network Quality Estimator of TCP RTTs,
// which in turn notifies registered RTT observers.
TEST(NetworkQualityEstimatorTest, MAYBE_TestTCPSocketRTT) {
TestRTTObserver rtt_observer;
std::map<std::string, std::string> variation_params;
TestNetworkQualityEstimator estimator(variation_params);
estimator.AddRTTObserver(&rtt_observer);

TestDelegate test_delegate;
TestURLRequestContext context(true);
context.set_network_quality_estimator(&estimator);

scoped_ptr<HttpNetworkSession::Params> params(new HttpNetworkSession::Params);
// |estimator| should be notified of TCP RTT observations.
params->socket_performance_watcher_factory =
estimator.GetSocketPerformanceWatcherFactory();
context.set_http_network_session_params(std::move(params));
context.Init();

EXPECT_EQ(0U, rtt_observer.observations().size());

// Send two requests. Verify that the completion of each request generates at
// least one TCP RTT observation.
for (size_t i = 0; i < 2; ++i) {
size_t before_count_tcp_rtt_observations = 0;
for (const auto& observation : rtt_observer.observations()) {
if (observation.source == NetworkQualityEstimator::TCP)
++before_count_tcp_rtt_observations;
}

scoped_ptr<URLRequest> request(context.CreateRequest(
estimator.GetEchoURL(), DEFAULT_PRIORITY, &test_delegate));
request->Start();
base::RunLoop().Run();

size_t after_count_tcp_rtt_observations = 0;
for (const auto& observation : rtt_observer.observations()) {
if (observation.source == NetworkQualityEstimator::TCP)
++after_count_tcp_rtt_observations;
}
// At least one notification should be received per socket performance
// watcher.
EXPECT_LE(1U, after_count_tcp_rtt_observations -
before_count_tcp_rtt_observations)
<< i;
}
}

} // namespace net
2 changes: 2 additions & 0 deletions net/dns/address_sorter_posix_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/macros.h"
#include "net/base/ip_address.h"
#include "net/base/net_errors.h"
#include "net/base/socket_performance_watcher.h"
#include "net/base/test_completion_callback.h"
#include "net/socket/client_socket_factory.h"
#include "net/socket/ssl_client_socket.h"
Expand Down Expand Up @@ -108,6 +109,7 @@ class TestSocketFactory : public ClientSocketFactory {
}
scoped_ptr<StreamSocket> CreateTransportClientSocket(
const AddressList&,
scoped_ptr<SocketPerformanceWatcher>,
NetLog*,
const NetLog::Source&) override {
NOTIMPLEMENTED();
Expand Down
2 changes: 2 additions & 0 deletions net/dns/dns_session_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/rand_util.h"
#include "base/stl_util.h"
#include "net/base/ip_address.h"
#include "net/base/socket_performance_watcher.h"
#include "net/dns/dns_protocol.h"
#include "net/dns/dns_socket_pool.h"
#include "net/log/net_log.h"
Expand All @@ -36,6 +37,7 @@ class TestClientSocketFactory : public ClientSocketFactory {

scoped_ptr<StreamSocket> CreateTransportClientSocket(
const AddressList& addresses,
scoped_ptr<SocketPerformanceWatcher>,
NetLog*,
const NetLog::Source&) override {
NOTIMPLEMENTED();
Expand Down
5 changes: 2 additions & 3 deletions net/dns/dns_socket_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ scoped_ptr<StreamSocket> DnsSocketPool::CreateTCPSocket(
const NetLog::Source& source) {
DCHECK_LT(server_index, nameservers_->size());

return scoped_ptr<StreamSocket>(
socket_factory_->CreateTransportClientSocket(
AddressList((*nameservers_)[server_index]), net_log_, source));
return scoped_ptr<StreamSocket>(socket_factory_->CreateTransportClientSocket(
AddressList((*nameservers_)[server_index]), NULL, net_log_, source));
}

scoped_ptr<DatagramClientSocket> DnsSocketPool::CreateConnectedSocket(
Expand Down
4 changes: 2 additions & 2 deletions net/ftp/ftp_network_transaction.cc
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ int FtpNetworkTransaction::DoCtrlResolveHostComplete(int result) {
int FtpNetworkTransaction::DoCtrlConnect() {
next_state_ = STATE_CTRL_CONNECT_COMPLETE;
ctrl_socket_ = socket_factory_->CreateTransportClientSocket(
addresses_, net_log_.net_log(), net_log_.source());
addresses_, NULL, net_log_.net_log(), net_log_.source());
net_log_.AddEvent(
NetLog::TYPE_FTP_CONTROL_CONNECTION,
ctrl_socket_->NetLog().source().ToEventParametersCallback());
Expand Down Expand Up @@ -1232,7 +1232,7 @@ int FtpNetworkTransaction::DoDataConnect() {
data_address = AddressList::CreateFromIPAddress(
ip_endpoint.address(), data_connection_port_);
data_socket_ = socket_factory_->CreateTransportClientSocket(
data_address, net_log_.net_log(), net_log_.source());
data_address, NULL, net_log_.net_log(), net_log_.source());
net_log_.AddEvent(
NetLog::TYPE_FTP_DATA_CONNECTION,
data_socket_->NetLog().source().ToEventParametersCallback());
Expand Down
3 changes: 2 additions & 1 deletion net/http/http_network_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ ClientSocketPoolManager* CreateSocketPoolManager(
params.net_log,
params.client_socket_factory ? params.client_socket_factory
: ClientSocketFactory::GetDefaultFactory(),
params.host_resolver, params.cert_verifier, params.channel_id_service,
params.socket_performance_watcher_factory, params.host_resolver,
params.cert_verifier, params.channel_id_service,
params.transport_security_state, params.cert_transparency_verifier,
params.ct_policy_enforcer, ssl_session_cache_shard,
params.ssl_config_service, pool_type);
Expand Down
8 changes: 3 additions & 5 deletions net/http/http_network_transaction_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,8 +631,7 @@ template <typename ParentPool>
CaptureGroupNameSocketPool<ParentPool>::CaptureGroupNameSocketPool(
HostResolver* host_resolver,
CertVerifier* /* cert_verifier */)
: ParentPool(0, 0, host_resolver, NULL, NULL) {
}
: ParentPool(0, 0, host_resolver, NULL, NULL, NULL) {}

template <>
CaptureGroupNameHttpProxySocketPool::CaptureGroupNameSocketPool(
Expand Down Expand Up @@ -11674,9 +11673,8 @@ TEST_P(HttpNetworkTransactionTest, MultiRoundAuth) {
TransportClientSocketPool* transport_pool = new TransportClientSocketPool(
50, // Max sockets for pool
1, // Max sockets per group
session_deps_.host_resolver.get(),
session_deps_.socket_factory.get(),
session_deps_.net_log);
session_deps_.host_resolver.get(), session_deps_.socket_factory.get(),
NULL, session_deps_.net_log);
scoped_ptr<MockClientSocketPoolManager> mock_pool_manager(
new MockClientSocketPoolManager);
mock_pool_manager->SetTransportSocketPool(transport_pool);
Expand Down
4 changes: 2 additions & 2 deletions net/http/http_stream_factory_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,8 @@ template <typename ParentPool>
CapturePreconnectsSocketPool<ParentPool>::CapturePreconnectsSocketPool(
HostResolver* host_resolver,
CertVerifier* /* cert_verifier */)
: ParentPool(0, 0, host_resolver, nullptr, nullptr), last_num_streams_(-1) {
}
: ParentPool(0, 0, host_resolver, nullptr, nullptr, nullptr),
last_num_streams_(-1) {}

template <>
CapturePreconnectsHttpProxySocketPool::CapturePreconnectsSocketPool(
Expand Down
2 changes: 1 addition & 1 deletion net/server/http_server_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class TestHttpClient {
int ConnectAndWait(const IPEndPoint& address) {
AddressList addresses(address);
NetLog::Source source;
socket_.reset(new TCPClientSocket(addresses, NULL, source));
socket_.reset(new TCPClientSocket(addresses, NULL, NULL, source));

base::RunLoop run_loop;
connect_result_ = socket_->Connect(base::Bind(&TestHttpClient::OnConnect,
Expand Down
Loading

0 comments on commit 7b403bc

Please sign in to comment.