Introduce AlternateProtocolInfo.is_broken.
Introduce bool AlternateProtocolInfo.is_broken and remove
ALTERNATE_PROTOCOL_BROKEN from enum AlternateProtocol. This allows
AlternateProtocolInfo to keep track of which protocol is broken. Note that the
port does not identify the protocol: most notably, SPDY and QUIC can use the
same port, but one uses TCP, the other UDP.
This change is essential if we want to keep track of multiple
AlternateProtocolInfo structs for each server, in preparation for alternate
service support.
Note that this change stops writing out broken protocols to prefs in
HttpServerPropertiesManager::UpdatePrefsOnPrefThread(). This is okay, because
HttpServerPropertiesManager::UpdateCacheFromPrefsOnPrefThread() used to refuse
to load broken ones already, see IsAlternateProtocolValid(protocol) in line 436.
BUG=392575
Review URL: https://codereview.chromium.org/701163002
Cr-Commit-Position: refs/heads/master@{#303075}
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index dc02ec5..938f1b0 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -8511,7 +8511,7 @@
const AlternateProtocolInfo alternate =
http_server_properties->GetAlternateProtocol(
HostPortPair::FromURL(request.url));
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, alternate.protocol);
+ EXPECT_TRUE(alternate.is_broken);
}
TEST_P(HttpNetworkTransactionTest,
diff --git a/net/http/http_server_properties.cc b/net/http/http_server_properties.cc
index a9cbd72..4433b4e 100644
--- a/net/http/http_server_properties.cc
+++ b/net/http/http_server_properties.cc
@@ -23,7 +23,6 @@
"npn-h2-14", // HTTP/2 draft 14. Called SPDY4 internally.
"quic"
};
-const char kBrokenAlternateProtocol[] = "Broken";
COMPILE_ASSERT(
arraysize(kAlternateProtocolStrings) == NUM_VALID_ALTERNATE_PROTOCOLS,
@@ -57,8 +56,6 @@
DCHECK(IsAlternateProtocolValid(protocol));
return kAlternateProtocolStrings[
protocol - ALTERNATE_PROTOCOL_MINIMUM_VALID_VERSION];
- case ALTERNATE_PROTOCOL_BROKEN:
- return kBrokenAlternateProtocol;
case UNINITIALIZED_ALTERNATE_PROTOCOL:
return "Uninitialized";
}
@@ -73,8 +70,6 @@
if (str == AlternateProtocolToString(protocol))
return protocol;
}
- if (str == kBrokenAlternateProtocol)
- return ALTERNATE_PROTOCOL_BROKEN;
return UNINITIALIZED_ALTERNATE_PROTOCOL;
}
@@ -101,9 +96,10 @@
}
std::string AlternateProtocolInfo::ToString() const {
- return base::StringPrintf("%d:%s p=%f", port,
+ return base::StringPrintf("%d:%s p=%f%s", port,
AlternateProtocolToString(protocol),
- probability);
+ probability,
+ is_broken ? " (broken)" : "");
}
} // namespace net
diff --git a/net/http/http_server_properties.h b/net/http/http_server_properties.h
index edf598f..337f22d 100644
--- a/net/http/http_server_properties.h
+++ b/net/http/http_server_properties.h
@@ -61,7 +61,6 @@
NPN_SPDY_MAXIMUM_VERSION = NPN_SPDY_4,
QUIC,
ALTERNATE_PROTOCOL_MAXIMUM_VALID_VERSION = QUIC,
- ALTERNATE_PROTOCOL_BROKEN, // The alternate protocol is known to be broken.
UNINITIALIZED_ALTERNATE_PROTOCOL,
};
@@ -88,7 +87,17 @@
double probability)
: port(port),
protocol(protocol),
- probability(probability) {}
+ probability(probability),
+ is_broken(false) {}
+
+ AlternateProtocolInfo(uint16 port,
+ AlternateProtocol protocol,
+ double probability,
+ bool is_broken)
+ : port(port),
+ protocol(protocol),
+ probability(probability),
+ is_broken(is_broken) {}
bool Equals(const AlternateProtocolInfo& other) const {
return port == other.port &&
@@ -101,6 +110,7 @@
uint16 port;
AlternateProtocol protocol;
double probability;
+ bool is_broken;
};
struct NET_EXPORT SupportsQuic {
diff --git a/net/http/http_server_properties_impl.cc b/net/http/http_server_properties_impl.cc
index 103d0e1..65ecd11 100644
--- a/net/http/http_server_properties_impl.cc
+++ b/net/http/http_server_properties_impl.cc
@@ -49,13 +49,12 @@
void HttpServerPropertiesImpl::InitializeAlternateProtocolServers(
AlternateProtocolMap* alternate_protocol_map) {
- // Keep all the ALTERNATE_PROTOCOL_BROKEN ones since those don't
- // get persisted.
+ // Keep all the broken ones since those don't get persisted.
for (AlternateProtocolMap::iterator it = alternate_protocol_map_.begin();
it != alternate_protocol_map_.end();) {
AlternateProtocolMap::iterator old_it = it;
++it;
- if (old_it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
+ if (!old_it->second.is_broken) {
alternate_protocol_map_.Erase(old_it);
}
}
@@ -251,10 +250,6 @@
uint16 alternate_port,
AlternateProtocol alternate_protocol,
double alternate_probability) {
- if (alternate_protocol == ALTERNATE_PROTOCOL_BROKEN) {
- LOG(DFATAL) << "Call SetBrokenAlternateProtocol() instead.";
- return;
- }
AlternateProtocolInfo alternate(alternate_port,
alternate_protocol,
@@ -263,13 +258,12 @@
const AlternateProtocolInfo existing_alternate =
GetAlternateProtocol(server);
- if (existing_alternate.protocol == ALTERNATE_PROTOCOL_BROKEN) {
+ if (existing_alternate.is_broken) {
DVLOG(1) << "Ignore alternate protocol since it's known to be broken.";
return;
}
- if (alternate_protocol != ALTERNATE_PROTOCOL_BROKEN &&
- !existing_alternate.Equals(alternate)) {
+ if (!existing_alternate.Equals(alternate)) {
LOG(WARNING) << "Changing the alternate protocol for: "
<< server.ToString()
<< " from [Port: " << existing_alternate.port
@@ -306,14 +300,11 @@
void HttpServerPropertiesImpl::SetBrokenAlternateProtocol(
const HostPortPair& server) {
AlternateProtocolMap::iterator it = alternate_protocol_map_.Get(server);
- if (it != alternate_protocol_map_.end()) {
- it->second.protocol = ALTERNATE_PROTOCOL_BROKEN;
- } else {
- AlternateProtocolInfo alternate(server.port(),
- ALTERNATE_PROTOCOL_BROKEN,
- 1);
- alternate_protocol_map_.Put(server, alternate);
+ if (it == alternate_protocol_map_.end()) {
+ LOG(DFATAL) << "Trying to mark unknown alternate protocol broken.";
+ return;
}
+ it->second.is_broken = true;
int count = ++broken_alternate_protocol_map_[server];
base::TimeDelta delay =
base::TimeDelta::FromSeconds(kBrokenAlternateProtocolDelaySecs);
diff --git a/net/http/http_server_properties_impl_unittest.cc b/net/http/http_server_properties_impl_unittest.cc
index a23ebbed..1c016b78 100644
--- a/net/http/http_server_properties_impl_unittest.cc
+++ b/net/http/http_server_properties_impl_unittest.cc
@@ -286,19 +286,18 @@
TEST_F(AlternateProtocolServerPropertiesTest, Initialize) {
HostPortPair test_host_port_pair1("foo1", 80);
+ impl_.SetAlternateProtocol(test_host_port_pair1, 443, NPN_SPDY_3, 1);
impl_.SetBrokenAlternateProtocol(test_host_port_pair1);
HostPortPair test_host_port_pair2("foo2", 80);
impl_.SetAlternateProtocol(test_host_port_pair2, 443, NPN_SPDY_3, 1);
AlternateProtocolMap alternate_protocol_map(
AlternateProtocolMap::NO_AUTO_EVICT);
- AlternateProtocolInfo port_alternate_protocol_pair(123, NPN_SPDY_3, 1);
- alternate_protocol_map.Put(test_host_port_pair2,
- port_alternate_protocol_pair);
+ AlternateProtocolInfo alternate(123, NPN_SPDY_3, 1);
+ alternate_protocol_map.Put(test_host_port_pair2, alternate);
HostPortPair test_host_port_pair3("foo3", 80);
- port_alternate_protocol_pair.port = 1234;
- alternate_protocol_map.Put(test_host_port_pair3,
- port_alternate_protocol_pair);
+ alternate.port = 1234;
+ alternate_protocol_map.Put(test_host_port_pair3, alternate);
impl_.InitializeAlternateProtocolServers(&alternate_protocol_map);
// Verify test_host_port_pair3 is the MRU server.
@@ -311,13 +310,11 @@
ASSERT_TRUE(impl_.HasAlternateProtocol(test_host_port_pair1));
ASSERT_TRUE(impl_.HasAlternateProtocol(test_host_port_pair2));
- port_alternate_protocol_pair =
- impl_.GetAlternateProtocol(test_host_port_pair1);
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, port_alternate_protocol_pair.protocol);
- port_alternate_protocol_pair =
- impl_.GetAlternateProtocol(test_host_port_pair2);
- EXPECT_EQ(123, port_alternate_protocol_pair.port);
- EXPECT_EQ(NPN_SPDY_3, port_alternate_protocol_pair.protocol);
+ alternate = impl_.GetAlternateProtocol(test_host_port_pair1);
+ EXPECT_TRUE(alternate.is_broken);
+ alternate = impl_.GetAlternateProtocol(test_host_port_pair2);
+ EXPECT_EQ(123, alternate.port);
+ EXPECT_EQ(NPN_SPDY_3, alternate.protocol);
}
TEST_F(AlternateProtocolServerPropertiesTest, MRUOfHasAlternateProtocol) {
@@ -365,11 +362,12 @@
TEST_F(AlternateProtocolServerPropertiesTest, SetBroken) {
HostPortPair test_host_port_pair("foo", 80);
+ impl_.SetAlternateProtocol(test_host_port_pair, 443, NPN_SPDY_3, 1);
impl_.SetBrokenAlternateProtocol(test_host_port_pair);
ASSERT_TRUE(impl_.HasAlternateProtocol(test_host_port_pair));
AlternateProtocolInfo alternate =
impl_.GetAlternateProtocol(test_host_port_pair);
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, alternate.protocol);
+ EXPECT_TRUE(alternate.is_broken);
impl_.SetAlternateProtocol(
test_host_port_pair,
@@ -377,17 +375,17 @@
NPN_SPDY_3,
1);
alternate = impl_.GetAlternateProtocol(test_host_port_pair);
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, alternate.protocol)
- << "Second attempt should be ignored.";
+ EXPECT_TRUE(alternate.is_broken) << "Second attempt should be ignored.";
}
TEST_F(AlternateProtocolServerPropertiesTest, ClearBroken) {
HostPortPair test_host_port_pair("foo", 80);
+ impl_.SetAlternateProtocol(test_host_port_pair, 443, NPN_SPDY_3, 1);
impl_.SetBrokenAlternateProtocol(test_host_port_pair);
ASSERT_TRUE(impl_.HasAlternateProtocol(test_host_port_pair));
AlternateProtocolInfo alternate =
impl_.GetAlternateProtocol(test_host_port_pair);
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, alternate.protocol);
+ EXPECT_TRUE(alternate.is_broken);
impl_.ClearAlternateProtocol(test_host_port_pair);
EXPECT_FALSE(impl_.HasAlternateProtocol(test_host_port_pair));
}
diff --git a/net/http/http_server_properties_manager.cc b/net/http/http_server_properties_manager.cc
index 6796fb1..794030d 100644
--- a/net/http/http_server_properties_manager.cc
+++ b/net/http/http_server_properties_manager.cc
@@ -746,11 +746,11 @@
}
// Save alternate_protocol.
- if (server_pref.alternate_protocol) {
+ const net::AlternateProtocolInfo* port_alternate_protocol =
+ server_pref.alternate_protocol;
+ if (port_alternate_protocol && !port_alternate_protocol->is_broken) {
base::DictionaryValue* port_alternate_protocol_dict =
new base::DictionaryValue;
- const net::AlternateProtocolInfo* port_alternate_protocol =
- server_pref.alternate_protocol;
port_alternate_protocol_dict->SetInteger("port",
port_alternate_protocol->port);
const char* protocol_str =
diff --git a/net/http/http_stream_factory.cc b/net/http/http_stream_factory.cc
index e86d8f8..29d9b26 100644
--- a/net/http/http_stream_factory.cc
+++ b/net/http/http_stream_factory.cc
@@ -67,14 +67,10 @@
return;
}
- protocol =
- AlternateProtocolFromString(port_protocol_vector[1]);
+ protocol = AlternateProtocolFromString(port_protocol_vector[1]);
+
if (IsAlternateProtocolValid(protocol) &&
!session.IsProtocolEnabled(protocol)) {
- protocol = ALTERNATE_PROTOCOL_BROKEN;
- }
-
- if (protocol == ALTERNATE_PROTOCOL_BROKEN) {
DVLOG(1) << kAlternateProtocolHeader
<< " header has unrecognized protocol: "
<< port_protocol_vector[1];
@@ -94,7 +90,7 @@
const AlternateProtocolInfo existing_alternate =
http_server_properties->GetAlternateProtocol(host_port);
// If we think the alternate protocol is broken, don't change it.
- if (existing_alternate.protocol == ALTERNATE_PROTOCOL_BROKEN)
+ if (existing_alternate.is_broken)
return;
}
diff --git a/net/http/http_stream_factory_impl.cc b/net/http/http_stream_factory_impl.cc
index 689bcf9..8304694 100644
--- a/net/http/http_stream_factory_impl.cc
+++ b/net/http/http_stream_factory_impl.cc
@@ -194,7 +194,7 @@
AlternateProtocolInfo alternate =
http_server_properties.GetAlternateProtocol(origin);
- if (alternate.protocol == ALTERNATE_PROTOCOL_BROKEN) {
+ if (alternate.is_broken) {
HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_BROKEN);
return kNoAlternateProtocol;
}
diff --git a/net/quic/quic_network_transaction_unittest.cc b/net/quic/quic_network_transaction_unittest.cc
index ab698f3..62fe895 100644
--- a/net/quic/quic_network_transaction_unittest.cc
+++ b/net/quic/quic_network_transaction_unittest.cc
@@ -286,7 +286,7 @@
const AlternateProtocolInfo alternate =
session_->http_server_properties()->GetAlternateProtocol(
HostPortPair::FromURL(request_.url));
- EXPECT_EQ(ALTERNATE_PROTOCOL_BROKEN, alternate.protocol);
+ EXPECT_TRUE(alternate.is_broken);
}
void ExpectQuicAlternateProtocolMapping() {
