Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(104)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: net/http/http_stream_factory_impl_unittest.cc

Issue 2784143003: Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established (Closed)
Patch Set: Address comments Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « net/http/http_stream_factory_impl_request.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: net/http/http_stream_factory_impl_unittest.cc
diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc
index 7a3b9f50f3f9b670cf38cf2d9ac91b142b8a65b9..9c4cc0e98e6959f81508ae88f544a02c3b1409f6 100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -179,8 +179,7 @@ class MockHttpStreamFactoryImplForPreconnect : public HttpStreamFactoryImpl {
class StreamRequestWaiter : public HttpStreamRequest::Delegate {
public:
- StreamRequestWaiter()
- : waiting_for_stream_(false), stream_done_(false), error_status_(OK) {}
+ StreamRequestWaiter() : stream_done_(false), error_status_(OK) {}
// HttpStreamRequest::Delegate
@@ -188,8 +187,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
const ProxyInfo& used_proxy_info,
HttpStream* stream) override {
stream_done_ = true;
- if (waiting_for_stream_)
- loop_.Quit();
+ loop_.Quit();
stream_.reset(stream);
used_ssl_config_ = used_ssl_config;
used_proxy_info_ = used_proxy_info;
@@ -200,8 +198,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
const ProxyInfo& used_proxy_info,
WebSocketHandshakeStreamBase* stream) override {
stream_done_ = true;
- if (waiting_for_stream_)
- loop_.Quit();
+ loop_.Quit();
websocket_stream_.reset(stream);
used_ssl_config_ = used_ssl_config;
used_proxy_info_ = used_proxy_info;
@@ -212,8 +209,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
const ProxyInfo& used_proxy_info,
BidirectionalStreamImpl* stream) override {
stream_done_ = true;
- if (waiting_for_stream_)
- loop_.Quit();
+ loop_.Quit();
bidirectional_stream_impl_.reset(stream);
used_ssl_config_ = used_ssl_config;
used_proxy_info_ = used_proxy_info;
@@ -221,8 +217,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
void OnStreamFailed(int status, const SSLConfig& used_ssl_config) override {
stream_done_ = true;
- if (waiting_for_stream_)
- loop_.Quit();
+ loop_.Quit();
used_ssl_config_ = used_ssl_config;
error_status_ = status;
}
@@ -246,13 +241,7 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
void OnQuicBroken() override {}
- void WaitForStream() {
- while (!stream_done_) {
- waiting_for_stream_ = true;
- loop_.Run();
- waiting_for_stream_ = false;
- }
- }
+ void WaitForStream() { loop_.Run(); }
const SSLConfig& used_ssl_config() const {
return used_ssl_config_;
@@ -278,7 +267,6 @@ class StreamRequestWaiter : public HttpStreamRequest::Delegate {
int error_status() const { return error_status_; }
private:
- bool waiting_for_stream_;
bool stream_done_;
base::RunLoop loop_;
std::unique_ptr<HttpStream> stream_;
@@ -966,7 +954,6 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyMarkedAsBad) {
request_info.url = GURL("http://www.google.com");
SSLConfig ssl_config;
- StreamRequestWaiter waiter;
EXPECT_EQ(set_alternative_proxy_server,
test_proxy_delegate.alternative_proxy_server().is_quic());
@@ -977,6 +964,7 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyMarkedAsBad) {
// |socket_data_direct_first_request|. The second request should consume
// data from |socket_data_direct_second_request|.
for (size_t i = 0; i < 2; ++i) {
+ StreamRequestWaiter waiter;
std::unique_ptr<HttpStreamRequest> request(
session->http_stream_factory()->RequestStream(
request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
@@ -1072,7 +1060,6 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyNotMarkedAsBad) {
request_info.url = GURL("http://www.google.com");
SSLConfig ssl_config;
- StreamRequestWaiter waiter;
EXPECT_TRUE(test_proxy_delegate.alternative_proxy_server().is_quic());
@@ -1080,6 +1067,7 @@ TEST_F(HttpStreamFactoryTest, WithQUICAlternativeProxyNotMarkedAsBad) {
// |socket_data_proxy_main_job| and |socket_data_https_first|.
// The second request should consume data from |socket_data_https_second|.
for (size_t i = 0; i < 2; ++i) {
+ StreamRequestWaiter waiter;
std::unique_ptr<HttpStreamRequest> request(
session->http_stream_factory()->RequestStream(
request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
@@ -1498,30 +1486,31 @@ TEST_F(HttpStreamFactoryTest, PrivacyModeUsesDifferentSocketPoolGroup) {
request_info.privacy_mode = PRIVACY_MODE_DISABLED;
SSLConfig ssl_config;
- StreamRequestWaiter waiter;
-
+ StreamRequestWaiter waiter1;
std::unique_ptr<HttpStreamRequest> request1(
session->http_stream_factory()->RequestStream(
- request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+ request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter1,
/* enable_ip_based_pooling = */ true, NetLogWithSource()));
- waiter.WaitForStream();
+ waiter1.WaitForStream();
EXPECT_EQ(GetSocketPoolGroupCount(ssl_pool), 1);
+ StreamRequestWaiter waiter2;
std::unique_ptr<HttpStreamRequest> request2(
session->http_stream_factory()->RequestStream(
- request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+ request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter2,
/* enable_ip_based_pooling = */ true, NetLogWithSource()));
- waiter.WaitForStream();
+ waiter2.WaitForStream();
EXPECT_EQ(GetSocketPoolGroupCount(ssl_pool), 1);
+ StreamRequestWaiter waiter3;
request_info.privacy_mode = PRIVACY_MODE_ENABLED;
std::unique_ptr<HttpStreamRequest> request3(
session->http_stream_factory()->RequestStream(
- request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter,
+ request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter3,
/* enable_ip_based_pooling = */ true, NetLogWithSource()));
- waiter.WaitForStream();
+ waiter3.WaitForStream();
EXPECT_EQ(GetSocketPoolGroupCount(ssl_pool), 2);
}
@@ -2143,6 +2132,59 @@ TEST_F(HttpStreamFactoryTest, NewSpdySessionCloseIdleH2Sockets) {
EXPECT_EQ(1, GetSpdySessionCount(session.get()));
}
+// Regression test for crbug.com/706974.
+TEST_F(HttpStreamFactoryTest, TwoSpdyConnects) {
+ SpdySessionDependencies session_deps(ProxyService::CreateDirect());
+
+ MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+ std::vector<std::unique_ptr<SequencedSocketData>> providers;
+ SSLSocketDataProvider ssl_socket_data(ASYNC, OK);
+ ssl_socket_data.next_proto = kProtoHTTP2;
+ for (int i = 0; i < 2; i++) {
+ auto provider = base::MakeUnique<SequencedSocketData>(
+ reads, arraysize(reads), nullptr, 0);
+ provider->set_connect_data(MockConnect(ASYNC, OK));
+ session_deps.socket_factory->AddSocketDataProvider(provider.get());
+ providers.push_back(std::move(provider));
+ session_deps.socket_factory->AddSSLSocketDataProvider(&ssl_socket_data);
+ }
+
+ std::unique_ptr<HttpNetworkSession> session(
+ SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+ HostPortPair host_port_pair("www.google.com", 443);
+
+ // Request two streams at once and make sure they use the same connection.
+ HttpRequestInfo request_info;
+ request_info.method = "GET";
+ request_info.url = GURL("https://www.google.com");
+ request_info.load_flags = 0;
+
+ SSLConfig ssl_config;
+ StreamRequestWaiter waiter1;
+ StreamRequestWaiter waiter2;
+ std::unique_ptr<HttpStreamRequest> request1(
+ session->http_stream_factory()->RequestStream(
+ request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter1,
+ /* enable_ip_based_pooling = */ true, NetLogWithSource()));
+ std::unique_ptr<HttpStreamRequest> request2(
+ session->http_stream_factory()->RequestStream(
+ request_info, DEFAULT_PRIORITY, ssl_config, ssl_config, &waiter2,
+ /* enable_ip_based_pooling = */ true, NetLogWithSource()));
+ waiter1.WaitForStream();
+ waiter2.WaitForStream();
+ EXPECT_TRUE(waiter1.stream_done());
+ EXPECT_TRUE(waiter2.stream_done());
+ ASSERT_NE(nullptr, waiter1.stream());
+ ASSERT_NE(nullptr, waiter2.stream());
+ ASSERT_NE(waiter1.stream(), waiter2.stream());
+
+ // Establishing the SpdySession will close the extra H2 socket.
+ EXPECT_EQ(0, session->GetSSLSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL)
+ ->IdleSocketCount());
+ EXPECT_EQ(1, GetSpdySessionCount(session.get()));
+}
+
TEST_F(HttpStreamFactoryTest, RequestBidirectionalStreamImpl) {
SpdySessionDependencies session_deps(ProxyService::CreateDirect());
« no previous file with comments | « net/http/http_stream_factory_impl_request.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698